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

From: Jason Baron
Date: Mon Jul 11 2016 - 15:52:32 EST

On 07/11/2016 03:23 PM, Andrew Morton wrote:
On Mon, 11 Jul 2016 09:18:21 -0400 Jason Baron <jbaron@xxxxxxxxxx> wrote:

On 07/08/2016 05:41 PM, Andrew Morton wrote:
On Wed, 6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@xxxxxxxxxx> 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.


+#define dd_key_init(key, init) key = (init)


+#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.


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

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) \
: }
: +
: +#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)


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()


#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?