[ClusterLabs Developers] libqb: Re: 633f262 logging: Remove linker 'magic' and just use statics for logging callsites (#322)

[Andrew Beekhof]

> On 17 Jan 2019, at 2:59 am, Ken Gaillot <kgaillot at redhat.com> wrote:
> 
> I'm not familiar with the reasoning for the current setup, but
> pacemaker's crm_crit(), crm_error(), etc. use qb_logt(), while
> crm_debug() and crm_trace() (which won't be used in ordinary runs) do
> something similar to what you propose.
> 
> Pacemaker has about 1,700 logging calls that would be affected (not
> counting another 2,000 debug/trace). Presumably that means Pacemaker
> currently has about +16KB of memory overhead and binary size for
> debug/trace logging static pointers, and that would almost double using
> them for all logs. Not a big deal today? Or meaningful in an embedded
> context?
> 
> Not sure if that overhead vs runtime trade-off is the original
> motivation or not, but that's the first thing that comes to mind.

I believe my interest was the ability to turn them on dynamically in a running program (yes, i used it plenty back in the day) and have the overhead be minimal for the normal case when they weren't in use.

> 
> On Wed, 2019-01-16 at 16:20 +0100, Lars Ellenberg wrote:
>> On Wed, Jan 16, 2019 at 03:44:22PM +0100, Lars Ellenberg wrote:
>>> Back then when this "dynamic" logging was introduced,
>>> I thought the whole point was that "quiet" callsites
>>> are "cheap".
>>> 
>>> So I think you want to
>>> qb_log_callsite_get() only *once* per callsite,
>>> assign that to a static pointer (as you say in the commit message).
>>> And do the actual qb_log_real_() function call
>>> only conditionally based on if (cs->targets). 
>>> 
>>> That way, a disabled trace logging boils down to a
>>> if (cs && cs->targets)
>>>    ;
>>> Much cheaper than what you have now.
>>> 
>>> Or was always calling into both qb_log_callsite_get() and
>>> qb_log_real_()
>>> intentional for some obscure (to me) reason,
>>> even for disabled call sites?
>>> 
>>> Below I also added a test for (cs->tags & QB_LOG_TAG_LIBQB_MSG),
>>> in case someone used qb_util_set_log_function().
>>> If that special tag flag could be folded into cs->targets (e.g. bit
>>> 0),
>>> I'd like it even better.
>>> 
>>> Cheers,
>>>    Lars
>>> 
>>> 
>>> diff --git a/include/qb/qblog.h b/include/qb/qblog.h
>>> index 1943b94..f63f4ad 100644
>> 
>> Oops, that patch was against *before* the 633f262 commit :-)
>> and that's why I did not notice the clash in macro argument "tags"
>> and struct member "tags" when compile testing...
>> I forgot I jumped between checkouts :-(
>> 
>> anyways, the (now even make check testet)
>> patch for *after* 633f262 commit:
>> 
>> diff --git a/include/qb/qblog.h b/include/qb/qblog.h
>> index 31981b8..ae1d25c 100644
>> --- a/include/qb/qblog.h
>> +++ b/include/qb/qblog.h
>> @@ -340,11 +340,17 @@ void qb_log_from_external_source_va(const char
>> *function,
>>  * @param fmt usual printf style format specifiers
>>  * @param args usual printf style args
>>  */
>> -#define qb_logt(priority, tags, fmt, args...) do {	\
>> -	struct qb_log_callsite* descriptor_pt =		\
>> -	qb_log_callsite_get(__func__, __FILE__, fmt,	\
>> -			    priority, __LINE__, tags);	\
>> -	qb_log_real_(descriptor_pt, ##args);		\
>> +#define qb_logt(priority, tags_, fmt, args...) do {		\
>> +	static struct qb_log_callsite* descriptor_pt;		\
>> +	if (!descriptor_pt) {					\
>> +		descriptor_pt =					\
>> +		qb_log_callsite_get(__func__, __FILE__, fmt,	\
>> +				    priority, __LINE__, tags_);	\
>> +	}							\
>> +	if (descriptor_pt && (descriptor_pt->targets ||		\
>> +	    qb_bit_set(descriptor_pt->tags,			\
>> +		    QB_LOG_TAG_LIBQB_MSG_BIT)))			\
>> +		qb_log_real_(descriptor_pt, ##args);		\
>>     } while(0)
>> 
>> _______________________________________________
>> Developers mailing list
>> Developers at clusterlabs.org
>> https://lists.clusterlabs.org/mailman/listinfo/developers
> -- 
> Ken Gaillot <kgaillot at redhat.com>
> 
> _______________________________________________
> Developers mailing list
> Developers at clusterlabs.org
> https://lists.clusterlabs.org/mailman/listinfo/developers