Messages in this thread |  | | Subject | Re: [PATCH v3 7/7] dynamic_debug: add jump label support | From | Jason Baron <> | Date | Mon, 11 Jul 2016 15:52:26 -0400 |
| |
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
|  |