lkml.org 
[lkml]   [2011]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 25/26] dynamic_debug: add pr_fmt_dbg() for dynamic_pr_debug
    On Tue, Sep 27, 2011 at 10:51 PM, Joe Perches <joe@perches.com> wrote:
    > On Tue, 2011-09-27 at 20:54 -0600, Jim Cromie wrote:
    >> Cleanest way is to drop the original synonym, and just use
    >> warn (its shorter), but that creates some churn (havent grepped to see
    >> how much).
    >> I picked what looked like least effort & fewest corner-cases.
    >> ICBW..
    >
    > #ifndef pr_fmt_warning
    > #define pr_fmt_warning pr_fmt_warn
    > #endif
    >

    you've made it conditional, where line 203 is not. why ?
    201 #define pr_warning(fmt, ...) \
    202 printk(KERN_WARNING pr_fmt_warn(fmt), ##__VA_ARGS__)
    203 #define pr_warn pr_warning
    204 #define pr_notice(fmt, ...) \

    Also, though minor, 203 adds pr_warn as the shortcut,
    youve suggested the opposite. I presume this is an oversight.

    >> > What did you think of avoiding all of this and
    >> > having __dynamic_pr_debug move the fmt pointer over
    >> > any initial KBUILD_MODULE ": "
    >> >
    >> > int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
    >> > {
    >> > [...]
    >> >        size_t modsize = strlen(descriptor->modname);
    >> >        if (0 == strncmp(fmt, descriptor->modname, modsize) &&
    >> >            0 == strncmp(fmt + modsize, ": ", 2))
    >> >                fmt += modsize + 2;
    >> >        vprintk(fmt, ...)
    >> > ?
    >>
    >> I was getting to that... ;-)
    >> Im not crazy about it.  It feels like too much ..
    >> Its a runtime workaround for what I think is a
    >> problem in users' (or header's) #defines.
    >
    > I think exactly the opposite myself.
    >
    > I think all of the '#define pr_fmt(fmt) KBUILD_MODNAME ": "'
    > are effectivly useless and I will eventually delete them.
    >
    > The printk subsystem should look up the module name and
    > prefix them to the printk akin to how the dynamic_debug
    > system does.  Except the module name should be a singleton.

    hmm, maybe Ive missed something in your argument.

    For non-dynamic-debug builds, it seems you still want
    the MODNAME in the pr_(crit|warn|info|debug) output,
    you just dont like the hundreds of defines throughout drivers/*

    linux-2.6]$ grep -r pr_fmt drivers/ |wc
    427 2556 32040
    linux-2.6]$ grep -r pr_fmt drivers/ | grep KBUILD_MODNAME |wc
    303 1827 22567

    Your runtime check-and-skip-MODNAME code above
    fixes dynamic-debug so that works around user defines
    doing:
    #define pr_fmt(fmt) KBUILD_MODNAME ": "

    This would let user of a dynamic-debug enabled kernel
    add MODNAME selectively, with +m >$CONTROL


    But ISTM this does the same:

    diff --git a/include/linux/printk.h b/include/linux/printk.h
    index e21de7e..cf17986 100644
    --- a/include/linux/printk.h
    +++ b/include/linux/printk.h
    @@ -148,8 +148,17 @@ static inline void setup_log_buf(int early)

    extern void dump_stack(void) __cold;

    +/* Set pr_fmt default to most common, useful case */
    #ifndef pr_fmt
    -#define pr_fmt(fmt) fmt
    +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
    +#endif
    +/* except for dynamic-debug, where user can enable it, and we dont
    + want to print it 2x
    + */
    +#if defined(CONFIG_DYNAMIC_DEBUG)
    +#ifndef pr_fmt_debug
    +#define pr_fmt_debug(fmt) fmt
    +#endif
    #endif

    #ifndef pr_fmt_emerg

    This sets default to work for the 303 common cases,
    allowing the removal of their defines:
    #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

    It also allows existing non-standard uses to work as is:
    drivers/sh/intc/dynamic.c:#define pr_fmt(fmt) "intc: " fmt

    Your suggested pr_fmt_warning, etc allow severity specific
    customizations to override, w/o changing all the others..

    And in dynamic-debug kernels, the default pr_fmt_debug
    is reset so MODNAME isnt printed, except by the user:
    ~]# echo +m > $CONTROL
    IOW, it seems to take care of everything, w/o runtime workarounds.
    With agreement on this, I can fold above patch into 25/26
    (which Ive updated here to also fix pr_*_once, pr_*_ratelimited macros),
    and we can move against those 303 irritants at our leisure (there is
    no flag day)

    thanks
    Jim
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-09-28 19:29    [W:0.027 / U:60.632 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site