Re: [PATCH 1/3] drivers-core: make structured logging play nice with dynamic-debug

[Jim Cromie]

On Mon, Jul 23, 2012 at 11:09 AM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> On Mon, 2012-07-23 at 09:07 -0400, Jason Baron wrote:
>> On Thu, Jul 19, 2012 at 01:46:20PM -0600, Jim Cromie wrote:
>> > commit c4e00daaa96d3a0786f1f4fe6456281c60ef9a16 changed __dev_printk
>> > in a way that broke dynamic-debug's ability to control the dynamic
>> > prefix of dev_dbg(dev,..), but not dev_dbg(NULL,..) or pr_debug(..),
>> > which is why it wasnt noticed sooner.
>> >
>> > When dev==NULL, __dev_printk() just calls printk(), which just works.
>> > But otherwise, it assumed that level was always a string like "<L>"
>> > and just plucked out the 'L', ignoring the rest.  However,
>> > dynamic_emit_prefix() adds "[tid] module:func:line:" to the string,
>> > those additions all got lost.
>> >
>
> I think this is not a really good approach.
>
> 3 depths of %pV can consume quite a lot of stack
> and avoiding this is useful.
>
> I'd much rather improve/centralize the mechanism
> in dynamic_debug.c so that the extra recursion
> depth is avoided.
>
> Something like this:
>
> o Remove KERN_DEBUG from dynamic_emit_prefix
> o Make a function of create_syslog_header to format
>   the header for printk_emit.
> o Call printk_emit in dynamic_dev_dbg and dynamic_netdev_dbg
> o Call printk_emit in __dev_printk
> o Remove now unused EXPORT_SYMBOL(__netdev_printk)
> o Neatening
>
> Thoughts?
>

My patch was a minimal bug-fix aimed at rc-late, but it missed the window.
We now have room for a more comprehensive fix, such as yours.

OTOH, my patch does fix a bug, while yours is a bigger "optimization" patch.
Your commit message could identify the lost prefix as its impetus,
but I think it stands nicely on its own as a refactoring for less stack usage.

I'll try the patch soon

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