[RFC] Refactor kenter/kleave/kdebug macros

[RFC] Refactor kenter/kleave/kdebug macros

[Leon Romanovsky]

Dear David,

During my work on NOMMU system (mm/nommu.c), I saw definition and
usage of kenter/kleave/kdebug macros. These macros are compiled as
empty because of "#if 0" construction.
  45 #if 0
  46 #define kenter(FMT, ...) \
  47         printk(KERN_DEBUG "==> %s("FMT")\n", __func__, ##__VA_ARGS__)
  48 #define kleave(FMT, ...) \
  49         printk(KERN_DEBUG "<== %s()"FMT"\n", __func__, ##__VA_ARGS__)
  50 #define kdebug(FMT, ...) \
  51         printk(KERN_DEBUG "xxx" FMT"yyy\n", ##__VA_ARGS__)
  52 #else
  53 #define kenter(FMT, ...) \
  54         no_printk(KERN_DEBUG "==> %s("FMT")\n", __func__, ##__VA_ARGS__)
  55 #define kleave(FMT, ...) \
  56         no_printk(KERN_DEBUG "<== %s()"FMT"\n", __func__, ##__VA_ARGS__)
  57 #define kdebug(FMT, ...) \
  58         no_printk(KERN_DEBUG FMT"\n", ##__VA_ARGS__)
  59 #endif

This code was changed in 2009 [1] and similar definitions can be found
in 9 other files [2]. The protection of these definitions is slightly
different. There are places with "#if 0" protection and others with
"#if defined(__KDEBUG)" protection. __KDEBUG is supposed to be
inserted by GCC.

My question is how we should handle such duplicated debug print code?
As possible solutions, I see five options:
1. Leave it as is.
2. Move it to general include file (for example linux/printk.h) and
commonize the output to be consistent between different kdebug users.
3. Add CONFIG_*_DEBUG definition for every kdebug user.
4. Move everything to "#if 0" construction.
5. Move everything to "#if defined(__KDEBUG)" construction.

What do you think?

[1]     commit 8feae13110d60cc6287afabc2887366b0eb226c2
        Author: David Howells <dhowells@redhat.com>
        Date:   Thu Jan 8 12:04:47 2009 +0000

[2] List of all files there kdebug was defined:
* arch/mn10300/kernel/mn10300-serial.c
* arch/mn10300/mm/misalignment.c
* fs/cachefiles/internal.h
* fs/afs/internal.h
* fs/fscache/internal.h
* fs/binfmt_elf_fdpic.c
* kernel/cred.c
* mm/nommu.c
* net/rxrpc/ar-internal.h
* security/keys/internal.h

Thank you.

-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[Joe Perches]

On Sat, 2015-05-16 at 20:01 +0300, Leon Romanovsky wrote:
> Dear David,
> 
> During my work on NOMMU system (mm/nommu.c), I saw definition and
> usage of kenter/kleave/kdebug macros. These macros are compiled as
> empty because of "#if 0" construction.
>   45 #if 0
>   46 #define kenter(FMT, ...) \
>   47         printk(KERN_DEBUG "==> %s("FMT")\n", __func__, ##__VA_ARGS__)
>   48 #define kleave(FMT, ...) \
>   49         printk(KERN_DEBUG "<== %s()"FMT"\n", __func__, ##__VA_ARGS__)
>   50 #define kdebug(FMT, ...) \
>   51         printk(KERN_DEBUG "xxx" FMT"yyy\n", ##__VA_ARGS__)
>   52 #else
>   53 #define kenter(FMT, ...) \
>   54         no_printk(KERN_DEBUG "==> %s("FMT")\n", __func__, ##__VA_ARGS__)
>   55 #define kleave(FMT, ...) \
>   56         no_printk(KERN_DEBUG "<== %s()"FMT"\n", __func__, ##__VA_ARGS__)
>   57 #define kdebug(FMT, ...) \
>   58         no_printk(KERN_DEBUG FMT"\n", ##__VA_ARGS__)
>   59 #endif
[]
> My question is how we should handle such duplicated debug print code?
> As possible solutions, I see five options:
> 1. Leave it as is.
> 2. Move it to general include file (for example linux/printk.h) and
> commonize the output to be consistent between different kdebug users.
> 3. Add CONFIG_*_DEBUG definition for every kdebug user.
> 4. Move everything to "#if 0" construction.
> 5. Move everything to "#if defined(__KDEBUG)" construction.

6: delete the macros and uses


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[Leon Romanovsky]

On Sat, May 16, 2015 at 8:09 PM, Joe Perches <joe@perches.com> wrote:
> On Sat, 2015-05-16 at 20:01 +0300, Leon Romanovsky wrote:
[]
>> My question is how we should handle such duplicated debug print code?
>> As possible solutions, I see five options:
>> 1. Leave it as is.
>> 2. Move it to general include file (for example linux/printk.h) and
>> commonize the output to be consistent between different kdebug users.
>> 3. Add CONFIG_*_DEBUG definition for every kdebug user.
>> 4. Move everything to "#if 0" construction.
>> 5. Move everything to "#if defined(__KDEBUG)" construction.
>
> 6: delete the macros and uses
Thank you, It is indeed possible option, since in last six years there
were no attempts to "open" this code.


-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[David Howells]

Leon Romanovsky <leon@leon.nu> wrote:

> During my work on NOMMU system (mm/nommu.c), I saw definition and
> usage of kenter/kleave/kdebug macros. These macros are compiled as
> empty because of "#if 0" construction.

Because you only need them if you're debugging.  They shouldn't, generally, be
turned on upstream.

> This code was changed in 2009 [1] and similar definitions can be found
> in 9 other files [2]. The protection of these definitions is slightly
> different. There are places with "#if 0" protection and others with
> "#if defined(__KDEBUG)" protection. __KDEBUG is supposed to be
> inserted by GCC.

I can turn on all the macros in a file just be #defining __KDEBUG at the top.
When I first did this, pr_xxx() didn't exist.

Note that the macros in afs, cachefiles, fscache and rxrpc are more complex
than a grep tells you.  There are _enter(), _leave() and _debug() macros which
are conditional via a module parameter.  These are trivially individually
enableable during debugging by changing the initial underscore to a 'k'.  They
are otherwise enableable by module parameter (macros are individually
selectable) or enableably by file __KDEBUG.  These are well used.  Note that
just turning them all into pr_devel() would represent a loss of useful
function.

The ones in the keys directory are also very well used, though they aren't
externally selectable.  I've added functionality to the debugging, but haven't
necessarily needed to backport it to earlier variants.

For the mn10300 macros, I would just recommend leaving them as is.

For the nommu macros, you could convert them to pr_devel() - but putting all
the information in the kenter/kleave/kdebug macro into each pr_devel macro
would be more intrusive in the code since you'd have to move the stuff out of
there macro definition into each caller.  You could also reexpress the macros
in terms of pr_devel and get rid of the conditional.  OTOH, there's not that
much in the nommu code, so you could probably slim down a lot of what's
printed.

For the cred macro, just convert to pr_devel() or pr_debug() and make pr_fmt
insert current->comm and current->pid.

> 2. Move it to general include file (for example linux/printk.h) and
> commonize the output to be consistent between different kdebug users.

I would quite like to see kenter() and kleave() be moved to printk.h,
expressed in a similar way to pr_devel() or pr_debug() (and perhaps renamed
pr_enter() and pr_leave()) but separately so they can be enabled separately.
OTOH, possibly they should be enableable by compilation block rather than by
macro set.

The main thing I like out of the ones in afs, cachefiles, fscache and rxrpc is
the ability to just turn on a few across a bunch of files so as not to get
overwhelmed by data.

David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2769 bytes --]

On Mon, May 18, 2015 at 1:31 PM, David Howells <dhowells@redhat.com> wrote:

> I can turn on all the macros in a file just be #defining __KDEBUG at the
> top.
> When I first did this, pr_xxx() didn't exist.
>
> Note that the macros in afs, cachefiles, fscache and rxrpc are more complex
> than a grep tells you.  There are _enter(), _leave() and _debug() macros
> which
> are conditional via a module parameter.  These are trivially individually
> enableable during debugging by changing the initial underscore to a 'k'.
> They
> are otherwise enableable by module parameter (macros are individually
> selectable) or enableably by file __KDEBUG.  These are well used.  Note
> that
> just turning them all into pr_devel() would represent a loss of useful
> function.
>
> The ones in the keys directory are also very well used, though they aren't
> externally selectable.  I've added functionality to the debugging, but
> haven't
> necessarily needed to backport it to earlier variants.
>
> For the mn10300 macros, I would just recommend leaving them as is.
>
> For the nommu macros, you could convert them to pr_devel() - but putting
> all
> the information in the kenter/kleave/kdebug macro into each pr_devel macro
> would be more intrusive in the code since you'd have to move the stuff out
> of
> there macro definition into each caller.  You could also reexpress the
> macros
> in terms of pr_devel and get rid of the conditional.  OTOH, there's not
> that
> much in the nommu code, so you could probably slim down a lot of what's
> printed.
>
> For the cred macro, just convert to pr_devel() or pr_debug() and make
> pr_fmt
> insert current->comm and current->pid.
>
> > 2. Move it to general include file (for example linux/printk.h) and
> > commonize the output to be consistent between different kdebug users.
>
> I would quite like to see kenter() and kleave() be moved to printk.h,
> expressed in a similar way to pr_devel() or pr_debug() (and perhaps renamed
> pr_enter() and pr_leave()) but separately so they can be enabled
> separately.
> OTOH, possibly they should be enableable by compilation block rather than
> by
> macro set.
>
> The main thing I like out of the ones in afs, cachefiles, fscache and
> rxrpc is
> the ability to just turn on a few across a bunch of files so as not to get
> overwhelmed by data.
>
Blind conversion to pr_debug will blow the code because it will be always
compiled in. In current implementation, it replaced by empty functions
which is thrown by compiler.

Additionally, It looks like the output of these macros can be viewed by
ftrace mechanism.

Maybe we should delete them from mm/nommu.c as was pointed by Joe?



>
> David
>



-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

[-- Attachment #2: Type: text/html, Size: 3697 bytes --]

Re: [RFC] Refactor kenter/kleave/kdebug macros

[Leon Romanovsky]

Sorry for reposting.

On Mon, May 18, 2015 at 1:31 PM, David Howells <dhowells@redhat.com> wrote:
>
> I can turn on all the macros in a file just be #defining __KDEBUG at the
> top.
> When I first did this, pr_xxx() didn't exist.
>
> Note that the macros in afs, cachefiles, fscache and rxrpc are more
> complex
> than a grep tells you.  There are _enter(), _leave() and _debug() macros
> which
> are conditional via a module parameter.  These are trivially individually
> enableable during debugging by changing the initial underscore to a 'k'.
> They
> are otherwise enableable by module parameter (macros are individually
> selectable) or enableably by file __KDEBUG.  These are well used.  Note
> that
> just turning them all into pr_devel() would represent a loss of useful
> function.
>
> The ones in the keys directory are also very well used, though they aren't
> externally selectable.  I've added functionality to the debugging, but
> haven't
> necessarily needed to backport it to earlier variants.
>
> For the mn10300 macros, I would just recommend leaving them as is.
>
> For the nommu macros, you could convert them to pr_devel() - but putting
> all
> the information in the kenter/kleave/kdebug macro into each pr_devel macro
> would be more intrusive in the code since you'd have to move the stuff out
> of
> there macro definition into each caller.  You could also reexpress the
> macros
> in terms of pr_devel and get rid of the conditional.  OTOH, there's not
> that
> much in the nommu code, so you could probably slim down a lot of what's
> printed.
>
> For the cred macro, just convert to pr_devel() or pr_debug() and make
> pr_fmt
> insert current->comm and current->pid.
>
> > 2. Move it to general include file (for example linux/printk.h) and
> > commonize the output to be consistent between different kdebug users.
>
> I would quite like to see kenter() and kleave() be moved to printk.h,
> expressed in a similar way to pr_devel() or pr_debug() (and perhaps
> renamed
> pr_enter() and pr_leave()) but separately so they can be enabled
> separately.
> OTOH, possibly they should be enableable by compilation block rather than
> by
> macro set.
>
> The main thing I like out of the ones in afs, cachefiles, fscache and
> rxrpc is
> the ability to just turn on a few across a bunch of files so as not to get
> overwhelmed by data.

 Blind conversion to pr_debug will blow the code because it will be always
 compiled in. In current implementation, it replaced by empty functions which
 is thrown by compiler.

 Additionally, It looks like the output of these macros can be viewed by
 ftrace mechanism.

 Maybe we should delete them from mm/nommu.c as was pointed by Joe?


>
>
> David




 --
 Leon Romanovsky | Independent Linux Consultant
         www.leon.nu | leon@leon.nu



-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[David Howells]

Leon Romanovsky <leon@leon.nu> wrote:

> Blind conversion to pr_debug will blow the code because it will be always
> compiled in.

No, it won't.

> Additionally, It looks like the output of these macros can be viewed by ftrace
> mechanism.

*blink* It can?

> Maybe we should delete them from mm/nommu.c as was pointed by Joe?

Why?

David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[Leon Romanovsky]

On Mon, May 18, 2015 at 4:29 PM, David Howells <dhowells@redhat.com> wrote:
> Leon Romanovsky <leon@leon.nu> wrote:
>
>> Blind conversion to pr_debug will blow the code because it will be always
>> compiled in.
>
> No, it won't.
Sorry, you are right.

>
>> Additionally, It looks like the output of these macros can be viewed by ftrace
>> mechanism.
>
> *blink* It can?
I was under strong impression that "function" and "function_graph"
tracers will give similar kenter/kleave information. Do I miss
anything important, except the difference in output format?

>
>> Maybe we should delete them from mm/nommu.c as was pointed by Joe?
>
> Why?
If ftrace is sufficient to get the debug information, there will no
need to duplicate it.

>
> David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[David Howells]

Leon Romanovsky <leon@leon.nu> wrote:

> >> Additionally, It looks like the output of these macros can be viewed by
> >> ftrace mechanism.
> >
> > *blink* It can?
> I was under strong impression that "function" and "function_graph"
> tracers will give similar kenter/kleave information. Do I miss
> anything important, except the difference in output format?
> 
> >
> >> Maybe we should delete them from mm/nommu.c as was pointed by Joe?
> >
> > Why?
> If ftrace is sufficient to get the debug information, there will no
> need to duplicate it.

It isn't sufficient.  It doesn't store the parameters or the return value, it
doesn't distinguish the return path in a function when there's more than one,
eg.:

		kleave(" = %d [val]", ret);

vs:

	kleave(" = %lx", result);

in do_mmap_pgoff() and it doesn't permit you to retrieve data from where the
argument pointers that you don't have pointed to, eg.:

	kenter("%p{%d}", region, region->vm_usage);

David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] Refactor kenter/kleave/kdebug macros

[Leon Romanovsky]

On Mon, May 18, 2015 at 6:20 PM, David Howells <dhowells@redhat.com> wrote:
> Leon Romanovsky <leon@leon.nu> wrote:
>
>> >> Additionally, It looks like the output of these macros can be viewed by
>> >> ftrace mechanism.
>> >
>> > *blink* It can?
>> I was under strong impression that "function" and "function_graph"
>> tracers will give similar kenter/kleave information. Do I miss
>> anything important, except the difference in output format?
>>
>> >
>> >> Maybe we should delete them from mm/nommu.c as was pointed by Joe?
>> >
>> > Why?
>> If ftrace is sufficient to get the debug information, there will no
>> need to duplicate it.
>
> It isn't sufficient.  It doesn't store the parameters or the return value, it
> doesn't distinguish the return path in a function when there's more than one,
> eg.:
>
>                 kleave(" = %d [val]", ret);
>
> vs:
>
>         kleave(" = %lx", result);
>
> in do_mmap_pgoff() and it doesn't permit you to retrieve data from where the
> argument pointers that you don't have pointed to, eg.:
>
>         kenter("%p{%d}", region, region->vm_usage);
>
> David
Thanks you for explanation, I'll send the patch in near future.


-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>