Message-ID: <1317309888.1854.18.camel@Joe-Laptop>
Date:	Thu, 29 Sep 2011 08:24:48 -0700
From:	Joe Perches <joe@...ches.com>
To:	Jim Cromie <jim.cromie@...il.com>
Cc:	jbaron@...hat.com, bart.vanassche@...il.com, greg@...ah.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 25/26] dynamic_debug: add pr_fmt_dbg() for
 dynamic_pr_debug

On Wed, 2011-09-28 at 11:27 -0600, Jim Cromie wrote:
> On Tue, Sep 27, 2011 at 10:51 PM, Joe Perches <joe@...ches.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.

I think pr_warning should be deprecated.
I think not #defining pr_fmt_warning is just fine.

> >> > 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/*

Yes. I've added most all of them.

It's because the current printk / pr_<level>
subsystem chose to not prefix by default.
Over the last couple of years, I've helped
slowly converted about half of the kernel to
pr_<level> and pr_fmt.

At some point next year or so, enough of the
kernel should be converted so that marking
the last few uses of pr_debug or other
pr_<level> without a preceding pr_fmt should
be be relatively easy to convert to a more
standard KBUILD_MODNAME or other user specified
prefix.

It takes awhile though.

> IOW, it seems to take care of everything, w/o runtime workarounds.

Nope.  Try it with and without DEBUG.

Also, look at the pr_debug uses that include __func__.
These also duplicate output.

I'd prefer a solution that allows deduplication.
Having a dynamic_debug output to a fixed buffer
via vsnprintf, scanning that output for leading
KBUILD_MODNAME/__file__/__func__/__LINE__ prefixes
and then skipping them when enabled would seem
appropriate to me.

cheers, Joe

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

Powered by blists - more mailing lists