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

[Ken Gaillot]

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.

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>