cred: Remove unnecessary kdebug atomic reads
PATCH 1/1 cred: Remove unnecessary kdebug atomic reads
commit e0e817392b9a ("CRED: Add some configurable debugging [try #6]") added the kdebug mechanism to this file back in 2009.
The kdebug macro calls no_printk which always evaluates arguments.
Most of the kdebug uses have an unnecessary call of
atomic_read(&cred->usage)
Make the kdebug macro do nothing by defining it with
do { if (0) no_printk(...); } while (0)when not enabled.
$ size kernel/cred.o* (defconfig x86-64) text data bss dec hex filename 2748 336 8 3092 c14 kernel/cred.o.new 2788 336 8 3132 c3c kernel/cred.o.old
Miscellanea:
o Neaten the #define kdebug macros while there
--- kernel/cred.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
kernel/cred.c +9 -4
@@ -20,11 +20,16 @@#include <linux/cn_proc.h> #if 0 -#define kdebug (FMT , ...) \ - printk("[%-5.5s%5u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__) +#define kdebug (FMT , ...) \ + printk("[%-5.5s%5u] " FMT "\n", \ + current->comm, current->pid, ##__VA_ARGS__)#else -#define kdebug (FMT , ...) \ - no_printk("[%-5.5s%5u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__) +#define kdebug (FMT , ...) \ +do { \ + if (0) \ + no_printk("[%-5.5s%5u] " FMT "\n", \ + current->comm, current->pid, ##__VA_ARGS__); \ +} while (0)#endif static struct kmem_cache *cred_jar;
Quoted 35 lines · On Tue, 25 Aug 2015 09:53:51 -0700 Joe Perches <[email protected]> wrote:
commit
e0e817392b9a("CRED: Add some configurable debugging [try #6]") added the kdebug mechanism to this file back in 2009.The kdebug macro calls
no_printkwhich always evaluates arguments.Most of the kdebug uses have an unnecessary call of
atomic_read(&cred->usage)Make the kdebug macro do nothing by defining it with
do { if (0) no_printk(...); } while (0)when not enabled.
$ size kernel/cred.o* (defconfig x86-64) text data bss dec hex filename 2748 336 8 3092 c14 kernel/cred.o.new 2788 336 8 3132 c3c kernel/cred.o.oldMiscellanea:
o Neaten the #define kdebug macros while there...
kernel/cred.c +3
-3@@ -20,11 +20,16 @@#include <linux/cn_proc.h> #if 0 -#define kdebug (FMT , ...) \ - printk("[%-5.5s%5u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__) +#define kdebug (FMT , ...) \ + printk("[%-5.5s%5u] " FMT "\n", \ + current->comm, current->pid, ##__VA_ARGS__)#else -#define kdebug (FMT , ...) \
kernel/cred.c line 35
Quoted 10 lines
kernel/cred.c +6
-1- no_printk("[%-5.5s%5u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__) +#define kdebug (FMT , ...) \ +do { \ + if (0) \ + no_printk("[%-5.5s%5u] " FMT "\n", \ + current->comm, current->pid, ##__VA_ARGS__); \ +} while (0)#endif static struct kmem_cache *cred_jar;
Did you consider doing this within no_printk()? That would break code which is relying on side-effects in the evaluation of a printk arg but that's pretty weird and I bet there isn't (and won't be) such code.
Quoted 4 lines · Andrew Morton
Quoted 3 lines · On Tue, 25 Aug 2015 09:53:51 -0700 Joe Perches <[email protected]> wrote:
commit
e0e817392b9a("CRED: Add some configurable debugging [try #6]") added the kdebug mechanism to this file back in 2009.
Quoted 8 lines
Quoted 8 lines
The kdebug macro calls
no_printkwhich always evaluates arguments.Most of the kdebug uses have an unnecessary call of
atomic_read(&cred->usage)Make the kdebug macro do nothing by defining it with
do { if (0) no_printk(...); } while (0)when not enabled.
[]
Quoted 1 line
Did you consider doing this within
no_printk()?
Yes.
Quoted 3 lines
That would break code
which is relying on side-effects in the evaluation of a printk arg but that's pretty weird and I bet there isn't (and won't be) such code.
I'll bet you there is more than a little and I don't want to experiment with it unconditionally.
All printks would need to be evaluated for that side-effect.
Safer would be to create a new no_eval_printk macro and convert the no_printk uses over to that as appropriate and possibly create a CONFIG_ option to use no_eval_printk instead of no_printk/printk and let the adventurous find the side-effects.
Maybe a coccinelle script can be written to find all the locations with evaluated non-constant expression arguments with side-effects.
Quoted 27 lines · On Tue, 25 Aug 2015 13:51:06 -0700 Joe Perches <[email protected]> wrote:
Quoted 4 lines · Andrew Morton
Quoted 3 lines · On Tue, 25 Aug 2015 09:53:51 -0700 Joe Perches <[email protected]> wrote:
commit
e0e817392b9a("CRED: Add some configurable debugging [try #6]") added the kdebug mechanism to this file back in 2009.Quoted 8 lines
Quoted 8 lines
The kdebug macro calls
no_printkwhich always evaluates arguments.Most of the kdebug uses have an unnecessary call of
atomic_read(&cred->usage)Make the kdebug macro do nothing by defining it with
do { if (0) no_printk(...); } while (0)when not enabled.
[]
Quoted 1 line
Did you consider doing this within
no_printk()?Yes.
Quoted 3 lines
That would break code
which is relying on side-effects in the evaluation of a printk arg but that's pretty weird and I bet there isn't (and won't be) such code.I'll bet you there is more than a little and I don't want to experiment with it unconditionally.
All printks would need to be evaluated for that side-effect.
Quoted 7 lines
Safer would be to create a new
no_eval_printkmacro and convert theno_printkuses over to that as appropriate and possibly create aCONFIG_option to useno_eval_printkinstead of no_printk/printk and let the adventurous find the side-effects.Maybe a coccinelle script can be written to find all the locations with evaluated non-constant expression arguments with side-effects.
wimp.
That duplicated printk in cred.c is nasty. We could do this?
#if 0
#define __kdebug printk
#else
#define __kdebug if (0) no_printk
#endif
#define kdebug(FMT, ...) \ __kdebug(...)
Quoted 34 lines · Andrew Morton
Quoted 32 lines · On Tue, 25 Aug 2015 13:51:06 -0700 Joe Perches <[email protected]> wrote:
Quoted 4 lines · Andrew Morton
Quoted 3 lines · On Tue, 25 Aug 2015 09:53:51 -0700 Joe Perches <[email protected]> wrote:
commit
e0e817392b9a("CRED: Add some configurable debugging [try #6]") added the kdebug mechanism to this file back in 2009.Quoted 8 lines
Quoted 8 lines
The kdebug macro calls
no_printkwhich always evaluates arguments.Most of the kdebug uses have an unnecessary call of
atomic_read(&cred->usage)Make the kdebug macro do nothing by defining it with
do { if (0) no_printk(...); } while (0)when not enabled.
[]
Quoted 1 line
Did you consider doing this within
no_printk()?Yes.
Quoted 3 lines
That would break code
which is relying on side-effects in the evaluation of a printk arg but that's pretty weird and I bet there isn't (and won't be) such code.I'll bet you there is more than a little and I don't want to experiment with it unconditionally.
All printks would need to be evaluated for that side-effect.
Safer would be to create a new
no_eval_printkmacro and convert theno_printkuses over to that as appropriate and possibly create aCONFIG_option to useno_eval_printkinstead of no_printk/printk and let the adventurous find the side-effects.
Quoted 4 lines
Quoted 2 lines
Maybe a coccinelle script can be written to find all the locations with evaluated non-constant expression arguments with side-effects.
wimp.
twice shy...
Coccinelle isn't very good at calling tree analysis, so it'd be a difficult thing for it to do well anyway.
btw; I seems to recall suggestions around the same thing when no_printk was moved from subsystems to kernel.h by David Howells in 2010.
Quoted 7 lines
That duplicated printk in
cred.cis nasty. We could do this?#if 0
#define__kdebugprintk
#else
#define__kdebugif (0)no_printk
#endif
You could, but what's there is a very common idiom and what you suggest is unsafe for if/else
Quoted 3 lines · Andrew Morton
Did you consider doing this within
no_printk()? That would break code which is relying on side-effects in the evaluation of a printk arg but that's pretty weird and I bet there isn't (and won't be) such code.
The reasons that no_printk() is like it is are (1) so that the side-effects stay intact and (2) you still get format warnings. If someone prints a device register, for example, it *must* work the same whether or not you use printk() or no_printk(). Of course, printing a device register like this is probably not a good idea...
David