Re: [PATCH] printk(): add KERN_CONT where needed

[Kay Sievers]

On Tue, Apr 3, 2012 at 16:32, Joe Perches <joe@xxxxxxxxxxx> wrote:
> On Tue, 2012-04-03 at 12:30 +0200, Kay Sievers wrote:
>> On Tue, Apr 3, 2012 at 05:47, Joe Perches <joe@xxxxxxxxxxx> wrote:
>> > I think you should do it "right" rather than add
>> > trivial markers.
>>
>> The trivial markers _are_ correct. And they really fix things as soon
>> as we start storing machine-readable records with printk(), instead of
>> blindly glueing bytes together with each printk() call, for humans to
>> puzzle with them if things go wrong.
>
> These KERN_CONT changes don't _fix_ things,
> they just make it less likely to cause problems.

They sure changes things, as you can see in the first message in this
thread. This is a real paste of output, not made up stuff.

It will matter as soon as we do not allow the next printk() to get
appended to 'left-over garbage' of the last printk() without a
trailing '\n'. We should not allow that, and the 'risk' of
interleaving and garbled messages should be limited to threads which
_both_ do continuation at the same time. The likeliness of that will
get pretty low.

We should make absolutely sure, it will never affect the thread which
does a proper single and complete line.

> Imagine two threads with printks extended with
> KERN_CONT
>
> Thread 1: Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Thread 2:
> printk(KERN_INFO "info message: ");
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âprintk(KERN_ERR "err message: ");
> printk(KERN_CONT "online\n");
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âprintk(KERN_CONT "offline\n");
>
> Instead of a guarantee of "info message: online" and
> "err message: offline", buffering could still join
> the messages to "err message: online".

Yes, both threads play racy games with continuation, and that makes
them both vulnerable to interleaving. But we must make sure, that
single and complete lines are never affected by that.

I did not claim to address the problem of concurrent continuation line
writers, and this patch has absolutely nothing to do with that
problem. It _does_ fix encountered problems, and it is correct as it
is. Please do not mix other issues you bring up here into it, they
should go into a separate thread, unrelated to this patch.

> I believe the only _guaranteed_ way to correctly
> assemble these messages is to use a initiator with
> a cookie and pass that cookie to assembling printks.
>
> Something like:
>
> Â Â Â Âcookie = multi_printk_start()
> Â Â Â Âmulti_printk(cookie, level fmt, ...);
> Â Â Â Â...
> Â Â Â Âmulti_printk_end(cookie);
>
> Though get_current() might be a reasonable cookie
> so perhaps the multi_ variants aren't needed.

Apart from *possibly* switching to per-cpu buffers for printk() core,
I think the callers should probably just do that on their own, maybe
just on the stack, if the message is small enough.

> git.kernel.org isn't responding right now. ÂI
> can't read the link you sent me privately to
> check if you are using get_current() or some
> other current_thread_info() constuct.

No, it does not change any current behaviour or gets any knowledge
about threads, it just makes sure that full and single line always
ends up in its own record instead of accepting unterminated left-overs
from earlier printk()s.

Thanks,
Kay
--
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/