Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440617 > unrolled thread

Re: [PATCH v3 7/7] dynamic_debug: add jump label support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 7/7] dynamic_debug: add jump label support Jason Baron <jbaron@akamai.com> - 2016-07-11 15:20 +0200
    Re: [PATCH v3 7/7] dynamic_debug: add jump label support Andrew Morton <akpm@linux-foundation.org> - 2016-07-11 21:30 +0200
      Re: [PATCH v3 7/7] dynamic_debug: add jump label support Jason Baron <jbaron@akamai.com> - 2016-07-11 22:00 +0200

#1440617 — Re: [PATCH v3 7/7] dynamic_debug: add jump label support

On 07/08/2016 05:41 PM, Andrew Morton wrote:
> On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> wrote:
>
>> Although dynamic debug is often only used for debug builds, sometimes its
>> enabled for production builds as well. Minimize its impact by using jump
>> labels. This reduces the text section by 7000+ bytes in the kernel image
>> below. It does increase data, but this should only be referenced when
>> changing the direction of the branches, and hence usually not in cache.
>>
>> ...
>>
>> +#ifdef HAVE_JUMP_LABEL
>> +
>> +#define dd_key_init(key, init) key = (init)
>>
>> ...
>>
>> +#else
>> +
>> +#define dd_key_init(key, init)
>> +
> umm, lazy.  One is an lval and returns a value and the other does
> neither.  Lack of parenthesization in the first version doubtless
> exposes various horrors.
>
> Care to do something more robust and conventional here?  Presumably use
> symmetrical do{}while(0) things, neither of which is an lval, both of
> which don't return anything.
>

Hi,

The 'dd_key_init()' macro is being used here to help initialize
the 'key' field in the 'struct _ddebug', and its not being used as a
statement.

In the 'HAVE_JUMP_LABEL' case, we are initializing the 'key' field, while in
the not-'HAVE_JUMP_LABEL' case, the 'key' field is simply not present
in the structure (to conserve space).

Thanks,

-Jason

[toc] | [next] | [standalone]

#1440838

On Mon, 11 Jul 2016 09:18:21 -0400 Jason Baron <jbaron@akamai.com> wrote:

> On 07/08/2016 05:41 PM, Andrew Morton wrote:
> > On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> wrote:
> >
> >> Although dynamic debug is often only used for debug builds, sometimes its
> >> enabled for production builds as well. Minimize its impact by using jump
> >> labels. This reduces the text section by 7000+ bytes in the kernel image
> >> below. It does increase data, but this should only be referenced when
> >> changing the direction of the branches, and hence usually not in cache.
> >>
> >> ...
> >>
> >> +#ifdef HAVE_JUMP_LABEL
> >> +
> >> +#define dd_key_init(key, init) key = (init)
> >>
> >> ...
> >>
> >> +#else
> >> +
> >> +#define dd_key_init(key, init)
> >> +
> > umm, lazy.  One is an lval and returns a value and the other does
> > neither.  Lack of parenthesization in the first version doubtless
> > exposes various horrors.
> >
> > Care to do something more robust and conventional here?  Presumably use
> > symmetrical do{}while(0) things, neither of which is an lval, both of
> > which don't return anything.
> >
> 
> Hi,
> 
> The 'dd_key_init()' macro is being used here to help initialize
> the 'key' field in the 'struct _ddebug', and its not being used as a
> statement.
> 
> In the 'HAVE_JUMP_LABEL' case, we are initializing the 'key' field, while in
> the not-'HAVE_JUMP_LABEL' case, the 'key' field is simply not present
> in the structure (to conserve space).

Well yeah.  And it's doing it wrongly, isn't it?

: @@ -68,13 +78,51 @@ void __dynamic_netdev_dbg(struct _ddebug
:  		.filename = __FILE__,				\
:  		.format = (fmt),				\
:  		.lineno = __LINE__,				\
: -		.flags =  _DPRINTK_FLAGS_DEFAULT,		\
: +		.flags = _DPRINTK_FLAGS_DEFAULT,		\
: +		dd_key_init(key, init)				\
:  	}
:  
: +#ifdef HAVE_JUMP_LABEL
: +
: +#define dd_key_init(key, init) key = (init)

Shouldn't it be ".key = (init)"?

Anyway, it's odd-looking.  I guess something like

#define DD_KEY_INIT(init)	.key = (init)

would be more idiomatic.

[toc] | [prev] | [next] | [standalone]

#1440850

On 07/11/2016 03:23 PM, Andrew Morton wrote:
> On Mon, 11 Jul 2016 09:18:21 -0400 Jason Baron <jbaron@akamai.com> wrote:
>
>> On 07/08/2016 05:41 PM, Andrew Morton wrote:
>>> On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> wrote:
>>>
>>>> Although dynamic debug is often only used for debug builds, sometimes its
>>>> enabled for production builds as well. Minimize its impact by using jump
>>>> labels. This reduces the text section by 7000+ bytes in the kernel image
>>>> below. It does increase data, but this should only be referenced when
>>>> changing the direction of the branches, and hence usually not in cache.
>>>>
>>>> ...
>>>>
>>>> +#ifdef HAVE_JUMP_LABEL
>>>> +
>>>> +#define dd_key_init(key, init) key = (init)
>>>>
>>>> ...
>>>>
>>>> +#else
>>>> +
>>>> +#define dd_key_init(key, init)
>>>> +
>>> umm, lazy.  One is an lval and returns a value and the other does
>>> neither.  Lack of parenthesization in the first version doubtless
>>> exposes various horrors.
>>>
>>> Care to do something more robust and conventional here?  Presumably use
>>> symmetrical do{}while(0) things, neither of which is an lval, both of
>>> which don't return anything.
>>>
>> Hi,
>>
>> The 'dd_key_init()' macro is being used here to help initialize
>> the 'key' field in the 'struct _ddebug', and its not being used as a
>> statement.
>>
>> In the 'HAVE_JUMP_LABEL' case, we are initializing the 'key' field, while in
>> the not-'HAVE_JUMP_LABEL' case, the 'key' field is simply not present
>> in the structure (to conserve space).
> Well yeah.  And it's doing it wrongly, isn't it?
>
> : @@ -68,13 +78,51 @@ void __dynamic_netdev_dbg(struct _ddebug
> :  		.filename = __FILE__,				\
> :  		.format = (fmt),				\
> :  		.lineno = __LINE__,				\
> : -		.flags =  _DPRINTK_FLAGS_DEFAULT,		\
> : +		.flags = _DPRINTK_FLAGS_DEFAULT,		\
> : +		dd_key_init(key, init)				\
> :  	}
> :
> : +#ifdef HAVE_JUMP_LABEL
> : +
> : +#define dd_key_init(key, init) key = (init)
>
> Shouldn't it be ".key = (init)"?
>
> Anyway, it's odd-looking.  I guess something like
>
> #define DD_KEY_INIT(init)	.key = (init)
>
> would be more idiomatic.

Ok, so the 'key' field is a union and so the patch is effectively
calling (after substitution):

dd_key_init(.key.dd_key_true, STATIC_KEY_TRUE_INIT)

and:

dd_key_init(.key.dd_key_true, STATIC_KEY_FALSE_INIT)

So we could have variations such as:

#define dd_key_true_init() .key.dd_key_true = (STATIC_KEY_TRUE_INIT)
#define dd_key_false_init()

and

#define dd_key_true_init()
#define dd_key_false_init() .key.dd_key_false = (STATIC_KEY_FALSE_INIT)

and finally:

#define dd_key_true_init()
#define dd_key_false_init()

and then have both dd_key_true_init()  and dd_key_fase_init() in
the structure definition. It adds a bunch more definitions and I'm
not sure if its more readable, but maybe it would look cleaner?

Thanks,

-Jason

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web