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

Groups > linux.kernel > #1437955 > unrolled thread

[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

  [PATCH v3 7/7] dynamic_debug: add jump label support Jason Baron <jbaron@akamai.com> - 2016-07-06 23:50 +0200
    Re: [PATCH v3 7/7] dynamic_debug: add jump label support Joe Perches <joe@perches.com> - 2016-07-07 01:00 +0200
      Re: [PATCH v3 7/7] dynamic_debug: add jump label support Jason Baron <jbaron@akamai.com> - 2016-07-07 21:20 +0200
    Re: [PATCH v3 7/7] dynamic_debug: add jump label support Andrew Morton <akpm@linux-foundation.org> - 2016-07-08 23:50 +0200
      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

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

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.

   text	   data	    bss	    dec	    hex	filename
8194852	4879776	 925696	14000324	 d5a0c4	vmlinux.pre
8187337	4960224	 925696	14073257	 d6bda9	vmlinux.post

Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 include/linux/dynamic_debug.h | 60 ++++++++++++++++++++++++++++++++++++++-----
 lib/dynamic_debug.c           |  7 +++++
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 4f1bbc68cd1b..546d68057e3b 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -1,6 +1,10 @@
 #ifndef _DYNAMIC_DEBUG_H
 #define _DYNAMIC_DEBUG_H
 
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
+#include <linux/jump_label.h>
+#endif
+
 /*
  * An instance of this structure is created in a special
  * ELF section at every dynamic debug callsite.  At runtime,
@@ -33,6 +37,12 @@ struct _ddebug {
 #define _DPRINTK_FLAGS_DEFAULT 0
 #endif
 	unsigned int flags:8;
+#ifdef HAVE_JUMP_LABEL
+	union {
+		struct static_key_true dd_key_true;
+		struct static_key_false dd_key_false;
+	} key;
+#endif
 } __attribute__((aligned(8)));
 
 
@@ -60,7 +70,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			  const struct net_device *dev,
 			  const char *fmt, ...);
 
-#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+#define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, key, init)	\
 	static struct _ddebug  __aligned(8)			\
 	__attribute__((section("__verbose"))) name = {		\
 		.modname = KBUILD_MODNAME,			\
@@ -68,13 +78,51 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 		.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)
+
+#ifdef DEBUG
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, .key.dd_key_true, \
+					  (STATIC_KEY_TRUE_INIT))
+
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	static_branch_likely(&descriptor.key.dd_key_true)
+#else
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, .key.dd_key_false, \
+					  (STATIC_KEY_FALSE_INIT))
+
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	static_branch_unlikely(&descriptor.key.dd_key_false)
+#endif
+
+#else
+
+#define dd_key_init(key, init)
+
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, 0, 0)
+
+#ifdef DEBUG
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	likely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+#else
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+#endif
+
+#endif
+
 #define dynamic_pr_debug(fmt, ...)				\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),	\
 				   ##__VA_ARGS__);		\
 } while (0)
@@ -82,7 +130,7 @@ do {								\
 #define dynamic_dev_dbg(dev, fmt, ...)				\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		__dynamic_dev_dbg(&descriptor, dev, fmt,	\
 				  ##__VA_ARGS__);		\
 } while (0)
@@ -90,7 +138,7 @@ do {								\
 #define dynamic_netdev_dbg(dev, fmt, ...)			\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		__dynamic_netdev_dbg(&descriptor, dev, fmt,	\
 				     ##__VA_ARGS__);		\
 } while (0)
@@ -100,7 +148,7 @@ do {								\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor,		\
 		__builtin_constant_p(prefix_str) ? prefix_str : "hexdump");\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		print_hex_dump(KERN_DEBUG, prefix_str,		\
 			       prefix_type, rowsize, groupsize,	\
 			       buf, len, ascii);		\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fe42b6ec3f0c..da796e2dc4f5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -188,6 +188,13 @@ static int ddebug_change(const struct ddebug_query *query,
 			newflags = (dp->flags & mask) | flags;
 			if (newflags == dp->flags)
 				continue;
+#ifdef HAVE_JUMP_LABEL
+			if (dp->flags & _DPRINTK_FLAGS_PRINT) {
+				if (!(flags & _DPRINTK_FLAGS_PRINT))
+					static_branch_disable(&dp->key.dd_key_true);
+			} else if (flags & _DPRINTK_FLAGS_PRINT)
+				static_branch_enable(&dp->key.dd_key_true);
+#endif
 			dp->flags = newflags;
 			vpr_info("changed %s:%d [%s]%s =%s\n",
 				 trim_prefix(dp->filename), dp->lineno,
-- 
2.6.1

[toc] | [next] | [standalone]

#1438002

On Wed, 2016-07-06 at 17:42 -0400, Jason Baron 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.
> 
>    text	   data	    bss	    dec	    hex	filename
> 8194852	4879776	 925696	14000324	 d5a0c4	vmlinux.pre
> 8187337	4960224	 925696	14073257	 d6bda9	vmlinux.post

Maybe to get some of that space back on the 32 bit builds
the 8 byte alignment can be relaxed.

> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
[]
> @@ -33,6 +37,12 @@ struct _ddebug {
>  #define _DPRINTK_FLAGS_DEFAULT 0
>  #endif
>  	unsigned int flags:8;
> +#ifdef HAVE_JUMP_LABEL
> +	union {
> +		struct static_key_true dd_key_true;
> +		struct static_key_false dd_key_false;
> +	} key;
> +#endif
>  } __attribute__((aligned(8)));

Couldn't this be:

} __aligned(__alignof__(void *));
 
> @@ -60,7 +70,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
>  			  const struct net_device *dev,
>  			  const char *fmt, ...);
>  
> -#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
> +#define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, key, init)	\
>  	static struct _ddebug  __aligned(8)			\
>  	__attribute__((section("__verbose"))) name = {		\

And this shouldn't need the __aligned as it's already
in the structure definition.

Maybe:

	static struct _ddebug __section(__verbose) name = {

>  		.modname = KBUILD_MODNAME,			\

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

#1438908

On 07/06/2016 06:52 PM, Joe Perches wrote:
> On Wed, 2016-07-06 at 17:42 -0400, Jason Baron 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.
>>
>>    text	   data	    bss	    dec	    hex	filename
>> 8194852	4879776	 925696	14000324	 d5a0c4	vmlinux.pre
>> 8187337	4960224	 925696	14073257	 d6bda9	vmlinux.post
> 
> Maybe to get some of that space back on the 32 bit builds
> the 8 byte alignment can be relaxed.
> 
>> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> []
>> @@ -33,6 +37,12 @@ struct _ddebug {
>>  #define _DPRINTK_FLAGS_DEFAULT 0
>>  #endif
>>  	unsigned int flags:8;
>> +#ifdef HAVE_JUMP_LABEL
>> +	union {
>> +		struct static_key_true dd_key_true;
>> +		struct static_key_false dd_key_false;
>> +	} key;
>> +#endif
>>  } __attribute__((aligned(8)));
> 
> Couldn't this be:
> 
> } __aligned(__alignof__(void *));
>  
>> @@ -60,7 +70,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
>>  			  const struct net_device *dev,
>>  			  const char *fmt, ...);
>>  
>> -#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
>> +#define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, key, init)	\
>>  	static struct _ddebug  __aligned(8)			\
>>  	__attribute__((section("__verbose"))) name = {		\
> 
> And this shouldn't need the __aligned as it's already
> in the structure definition.
> 
> Maybe:
> 
> 	static struct _ddebug __section(__verbose) name = {
> 
>>  		.modname = KBUILD_MODNAME,			\
> 


hmmm...dropping that last bit increases the size of 'data', and
results in an oops on boot, I think b/c the __verbose section
adds gaps that the dynamic debug init code is not expecting. We
could revisit this for 32-bit, but this is somewhat fragile...

I think we could reduce the size of 'struct static_key' by
removing the 'next' field and using the 'entries' when there
are multiple modules associated with a key. This will increase
the memory usage slightly when there are multiple modules
associated with a key, but this is probably not the common case,
and certainly for dynamic debug we only have one patch target
for each key. That should reduce the 'data' increase by around
~20% and would reduce the size for other users. I can try something
along those lines in a re-post...

Thanks,

-Jason

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

#1439808

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.

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

#1440617

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] | [prev] | [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