Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1441539 > unrolled thread

Re: [PATCH v12 0/3] printk: Make printk() completely async

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v12 0/3] printk: Make printk() completely async Petr Mladek <pmladek@suse.com> - 2016-07-12 18:30 +0200
    Re: [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-07-13 09:50 +0200
      Re: [PATCH v12 0/3] printk: Make printk() completely async Petr Mladek <pmladek@suse.com> - 2016-07-13 13:20 +0200
        Re: [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-07-13 15:30 +0200
          Re: [PATCH v12 0/3] printk: Make printk() completely async Petr Mladek <pmladek@suse.com> - 2016-07-13 16:30 +0200

#1441539 — Re: [PATCH v12 0/3] printk: Make printk() completely async

On Wed 2016-06-29 14:08:35, Sergey Senozhatsky wrote:
> Hello,
> 
> the patch to fix the async KERN_CONT printk regression I mentioned
> several days ago in another thread.
> 
> as a separate patch for now, to ease the review. will squash with 0001.
> 
> ============== 8< ==============
> 
> >From 70f65ed55eb82a1b8b74fdbd1a7afc9e77e9b380 Mon Sep 17 00:00:00 2001
> From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Subject: [PATCH] printk: print KERN_CONT buffer in sync printk mode
> 
> One of my boxen has revealed that there is a race condition now between
> pr_cot() output and printk_kthread wake_up() time. Schematically, the CPU
> that triggered the race conditions was doing
> something like this:
> 
> 	for (...) {
> 		for (....) {
> 			...
> 			pr_cont(...);
> 			...
> 		}
> 		pr_cont("\n");
> 	}

I have tried the following:

       int a,b;

       for (a = 0; a < 1000; a++) {
               for (b = 0; b < 10; b++) {
                       pr_cont("item%04d", a*10 + b);
               }
               pr_cont("\n");
       }


> the resulting output was:
> 
> $ dmesg
> 
> 00000001 00000002 00000003 00000004 00000005 00000006 00000007
> 00000008
> 00000009 0000000a 0000000b 0000000c 0000000d 0000000e
> 0000000f 00000010 00000011 00000012 00000013 00000014 00000015
> 0000002b 0000002c 0000002d 0000002e 0000002f 00000030 00000031
> ....

This patch helped a lot but I still see the problem from time to time.
For example,

[ 4913.000290] item8720item8721item8722item8723item8724item8725item8726item8727item8728item8729
[ 4913.000294] item8730item8731item8732item8733item8734item8735item8736item8737item8738item8739
[ 4913.000298] item8740item8741item8742item8743item8744item8745item8746item8747item8748item8749
[ 4913.000310] item8750item8751item8752item8753item8754item8755item8756item8757item8758item8759
[ 4913.000327] item8760item8761item8762item8763item8764item8765item8766item8767item8768item8769
[ 4913.000342] item8770item8771item8772item8773item8774item8775item8776item8777item8778item8779
[ 4913.000356] item8780
[ 4913.000357] item8781
[ 4913.000358] item8782
[ 4913.000358] item8783
[ 4913.000359] item8784
[ 4913.000359] item8785
[ 4913.000359] item8786
[ 4913.000360] item8787
[ 4913.000360] item8788
[ 4913.000360] item8789

[ 4913.000361] item8790
[ 4913.000361] item8791
[ 4913.000361] item8792
[...]


> 
> KERN_CONT output is heavily relying on the fact there will be no other
> CPU(-s)/process(-es) doing any sort (KERN_CONT etc.) of printk-s simultaneously.
> This basically means that pr_cont() CPU is expected to be the one that actually
> does the printing and log_store() of the entire cont buffer:

In particular, the partially flushed cont buffer could not be used for new
messages until the rest of it is flushed to the console. It is because
the number of flushed characters is stored in the struct cont.


> 	pr_cont()
> 		console_unlock()
> 			console_cont_flush()
> 			log_store()	/* cont buffer */
> 	pr_cont()
> 		console_unlock()
> 			console_cont_flush()
> 			log_store()	/* cont buffer */
> 	....
> 
> Async printk breaks this contract, because console_cont_flush() is now deferred.
> Thus pr_cont() will see a not fully flushed cont buffer, which will force it to
> append a new KERN_CONT not to cont buffer, but to buffer log as a separate log
> entry via log_store(). Due to the deferred nature of console_cont_flush() the
> very next pr_cont() may see cont buffer being flushed, so it will keep the
> message in the cont buffer and log_store() it later. So the cont line will split
> across several log entries -- printk_kthread can take some time to wkeup,
> during which pr_cont() will keep splitting cont line:

I guess that the problem was there even before async printk. For
example, we do not keep lockbuf_lock between console_cont_flush()
and printing other lines from the ring buffer. It means that
new lines might appear in the buffer in the meantime.

Note that we call consoles in console_cont_flush() without
lockbuf_lock. So there is relatively long time for the race.

In each case, the async printk made it worse because there
might be longer periods between the printk thread is scheduled
and the rest of the cont buffer is flushed.



Anyway, this solution makes the async printk less helpful.
Any message that does not end with a new line will force
the sync mode and the risk of the soft lookup.


Another solution would be to remember cont.cons outside
of struct cont. Then the cont buffer could be reused
immediately.

The saved position will be used either for the actual
cont buffer or for the very first message in the ring
buffer. The situation might be detected by comparing
console_seq, log_next_seq, and log_first_seq.

if (console_seq == log_next_seq)
   cont buffer still includes the very last message

if (console_seq < log_first_seq)
   the message was lost => no partialy flushed message around

if (console_seq < log_next_seq && cont.cons)
   the current message in the log buffer is partially printed


Well, this looks quite complicated. And I am afraid that
I have missed something.

I am still scratching my head around it.

Best Regards,
Petr

[toc] | [next] | [standalone]

#1442129

Hello,

On (07/12/16 18:28), Petr Mladek wrote:
> I have tried the following:
> 
>        int a,b;
> 
>        for (a = 0; a < 1000; a++) {
>                for (b = 0; b < 10; b++) {
>                        pr_cont("item%04d", a*10 + b);
>                }
>                pr_cont("\n");
>        }
> 
> 
> [ 4913.000298] item8740item8741item8742item8743item8744item8745item8746item8747item8748item8749
> [ 4913.000310] item8750item8751item8752item8753item8754item8755item8756item8757item8758item8759
> [ 4913.000327] item8760item8761item8762item8763item8764item8765item8766item8767item8768item8769
> [ 4913.000342] item8770item8771item8772item8773item8774item8775item8776item8777item8778item8779
> [ 4913.000356] item8780
> [ 4913.000357] item8781
> [ 4913.000358] item8782
> [...]

hm.. so are there any 'concurrent' printk()-s coming from other CPUs
that are not getting printed on the console (because of loglevel restrictions),
but still screw up the cont buffer?.... otherwise, my expectation was that in
this particular case cpu issues a new pr_cont() only after it has printed
the current message via call_console_drivers()->write(). so partially flushed
cont buffer was not really expected to happen. I was wrong, obviously.

> > KERN_CONT output is heavily relying on the fact there will be no other
> > CPU(-s)/process(-es) doing any sort (KERN_CONT etc.) of printk-s simultaneously.
> > This basically means that pr_cont() CPU is expected to be the one that actually
> > does the printing and log_store() of the entire cont buffer:
> 
> In particular, the partially flushed cont buffer could not be used for new
> messages until the rest of it is flushed to the console. It is because
> the number of flushed characters is stored in the struct cont.
> > 	pr_cont()
> > 		console_unlock()
> > 			console_cont_flush()
> > 			log_store()	/* cont buffer */
> > 	pr_cont()
> > 		console_unlock()
> > 			console_cont_flush()
> > 			log_store()	/* cont buffer */
> > 	....
> > 
> > Async printk breaks this contract, because console_cont_flush() is now deferred.
> > Thus pr_cont() will see a not fully flushed cont buffer, which will force it to
> > append a new KERN_CONT not to cont buffer, but to buffer log as a separate log
> > entry via log_store(). Due to the deferred nature of console_cont_flush() the
> > very next pr_cont() may see cont buffer being flushed, so it will keep the
> > message in the cont buffer and log_store() it later. So the cont line will split
> > across several log entries -- printk_kthread can take some time to wkeup,
> > during which pr_cont() will keep splitting cont line:
> 
> I guess that the problem was there even before async printk. For
> example, we do not keep lockbuf_lock between console_cont_flush()
> and printing other lines from the ring buffer. It means that
> new lines might appear in the buffer in the meantime.
> 
> Note that we call consoles in console_cont_flush() without
> lockbuf_lock. So there is relatively long time for the race.
> 
[...]
> Another solution would be to remember cont.cons outside
> of struct cont. Then the cont buffer could be reused
> immediately.

just an idea.
... or try to make KERN_CONT SMP-safe. there are many pr_cont() call
sites. ACPI is one notable example. the others include OOM, some cgroup
related output (or... was it memcg), etc., etc.

so we *may be* can have a per-cpu cont buffer and add new API
pr_cont_begin()/pr_cont_end(), that would disable preemption.


+	pr_cont_begin() /* preempt_disable() */

	for (.....)
		pr_cont("%pS ....);

+	pr_cont_end() /* preempt_enable() */

pr_cont_end() also can flush this CPU's cont buffer and store the log
line. this will probably break very long cont lines (not sure if we
have any of those though). and may be flush_on_panic() would have to
do some extra work iterating each cpu.

> The saved position will be used either for the actual
> cont buffer or for the very first message in the ring
> buffer. The situation might be detected by comparing
> console_seq, log_next_seq, and log_first_seq.

we still can have pr_cont() happening on several cpus simultaneously.
console_seq is getting reset, when we register a new CON_PRINTBUFFER
console.

> if (console_seq == log_next_seq)
>    cont buffer still includes the very last message
> 
> if (console_seq < log_first_seq)
>    the message was lost => no partialy flushed message around
> 
> if (console_seq < log_next_seq && cont.cons)
>    the current message in the log buffer is partially printed
> 
> 
> Well, this looks quite complicated. And I am afraid that
> I have missed something.
> 
> I am still scratching my head around it.

	-ss

[toc] | [prev] | [next] | [standalone]

#1442345

On Wed 2016-07-13 16:42:42, Sergey Senozhatsky wrote:
> Hello,
> 
> On (07/12/16 18:28), Petr Mladek wrote:
> > I have tried the following:
> > 
> >        int a,b;
> > 
> >        for (a = 0; a < 1000; a++) {
> >                for (b = 0; b < 10; b++) {
> >                        pr_cont("item%04d", a*10 + b);
> >                }
> >                pr_cont("\n");
> >        }
> > 
> > 
> > [ 4913.000298] item8740item8741item8742item8743item8744item8745item8746item8747item8748item8749
> > [ 4913.000310] item8750item8751item8752item8753item8754item8755item8756item8757item8758item8759
> > [ 4913.000327] item8760item8761item8762item8763item8764item8765item8766item8767item8768item8769
> > [ 4913.000342] item8770item8771item8772item8773item8774item8775item8776item8777item8778item8779
> > [ 4913.000356] item8780
> > [ 4913.000357] item8781
> > [ 4913.000358] item8782
> > [...]
> 
> hm.. so are there any 'concurrent' printk()-s coming from other CPUs
> that are not getting printed on the console (because of loglevel restrictions),
> but still screw up the cont buffer?.... otherwise, my expectation was that in
> this particular case cpu issues a new pr_cont() only after it has printed
> the current message via call_console_drivers()->write(). so partially flushed
> cont buffer was not really expected to happen. I was wrong, obviously.

To be honest. I am not 100% sure what happens here. One theory is
that the printk kthread is waken because of some previous
non-continuous message. It steals console_lock() and partially flushes
the cont buffer. In this case, the pr_cont() calls are not able to get
console_lock() and basically work in the async mode. The pr_cont()
calls have to store each piece sepatately because the partially
flushed cont buffer is blocked until flushed completely.

Hmm, the strange thing is that I see this problem even when I force
the global synch more by

    echo Y >  /sys/module/printk/parameters/synchronous


I need to dig more into it.

> > Another solution would be to remember cont.cons outside
> > of struct cont. Then the cont buffer could be reused
> > immediately.
> 
> just an idea.
> ... or try to make KERN_CONT SMP-safe. there are many pr_cont() call
> sites. ACPI is one notable example. the others include OOM, some cgroup
> related output (or... was it memcg), etc., etc.
> 
> so we *may be* can have a per-cpu cont buffer and add new API
> pr_cont_begin()/pr_cont_end(), that would disable preemption.
> 
> 
> +	pr_cont_begin() /* preempt_disable() */
> 
> 	for (.....)
> 		pr_cont("%pS ....);
> 
> +	pr_cont_end() /* preempt_enable() */
>
> pr_cont_end() also can flush this CPU's cont buffer and store the log
> line. this will probably break very long cont lines (not sure if we
> have any of those though). and may be flush_on_panic() would have to
> do some extra work iterating each cpu.

It would work but I am a bit scared by the complexity. I think
that we should find a compromise between complexity and
reliability.

> > The saved position will be used either for the actual
> > cont buffer or for the very first message in the ring
> > buffer. The situation might be detected by comparing
> > console_seq, log_next_seq, and log_first_seq.
> 
> we still can have pr_cont() happening on several cpus simultaneously.
> console_seq is getting reset, when we register a new CON_PRINTBUFFER
> console.

I agree that mixing part of lines from different processes/cpus
is not ideal. But it is not much worse than mixing whole lines.
We should do a best effort but we do not need to be perfect.

I continue with scratching my head.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1442458

Hello,

On (07/13/16 13:14), Petr Mladek wrote:
[..]
> > > [ 4913.000310] item8750item8751item8752item8753item8754item8755item8756item8757item8758item8759
> > > [ 4913.000327] item8760item8761item8762item8763item8764item8765item8766item8767item8768item8769
> > > [ 4913.000342] item8770item8771item8772item8773item8774item8775item8776item8777item8778item8779
> > > [ 4913.000356] item8780
> > > [ 4913.000357] item8781
> > > [ 4913.000358] item8782
> > > [...]
> > 
> > hm.. so are there any 'concurrent' printk()-s coming from other CPUs
> > that are not getting printed on the console (because of loglevel restrictions),
> > but still screw up the cont buffer?.... otherwise, my expectation was that in
> > this particular case cpu issues a new pr_cont() only after it has printed
> > the current message via call_console_drivers()->write(). so partially flushed
> > cont buffer was not really expected to happen. I was wrong, obviously.
> 
> To be honest. I am not 100% sure what happens here. One theory is
> that the printk kthread is waken because of some previous
> non-continuous message. It steals console_lock() and partially flushes
> the cont buffer. In this case, the pr_cont() calls are not able to get
> console_lock() and basically work in the async mode. The pr_cont()
> calls have to store each piece sepatately because the partially
> flushed cont buffer is blocked until flushed completely.
> 
> Hmm, the strange thing is that I see this problem even when I force
> the global synch more by
> 
>     echo Y >  /sys/module/printk/parameters/synchronous

oh, even in sync mode. hm...

> > just an idea.
> > ... or try to make KERN_CONT SMP-safe. there are many pr_cont() call
> > sites. ACPI is one notable example. the others include OOM, some cgroup
> > related output (or... was it memcg), etc., etc.
> > 
> > so we *may be* can have a per-cpu cont buffer and add new API
> > pr_cont_begin()/pr_cont_end(), that would disable preemption.
> > 
> > 
> > +	pr_cont_begin() /* preempt_disable() */
> > 
> > 	for (.....)
> > 		pr_cont("%pS ....);
> > 
> > +	pr_cont_end() /* preempt_enable() */
> >
> > pr_cont_end() also can flush this CPU's cont buffer and store the log
> > line. this will probably break very long cont lines (not sure if we
> > have any of those though). and may be flush_on_panic() would have to
> > do some extra work iterating each cpu.
> 
> It would work but I am a bit scared by the complexity. I think
> that we should find a compromise between complexity and
> reliability.

I understand your concerns.
but, realistically, KERN_CONT will still be SMP unsafe and will not do
what people probably expect it to do: e.g. ACPI_ERROR or ACPI_anything.
it does acpi_os_vprintf() -> printk(KERN_CONT).

the whole thing is already complicated, we already have all those len,
cons etc. checks in various places, it also has that owner task, etc.

static struct cont {
	char buf[LOG_LINE_MAX];
	size_t len;			/* length == 0 means unused buffer */
	size_t cons;			/* bytes written to console */
	struct task_struct *owner;	/* task of first print*/
	u64 ts_nsec;			/* time of first print */
	u8 level;			/* log level of first message */
	u8 facility;			/* log facility of first message */
	enum log_flags flags;		/* prefix, newline flags */
	bool flushed:1;			/* buffer sealed and committed */
} cont;

so I think, personally... it's sort of kind of not exactly what people
need (aka 'broken'). so my idea is to forbid concurrent cont buffer usage.
each CPU will have it's own buffer, and touch it with preemption_disabled.
there will be no more partial flushes, cont buffer will be flushed when it's
done -- via pr_cont_end()->log_store()->wake_up(printk_kthread). IOW it will
just add the cont buffer content to log_buf and print it via console_unlock(),
like the rest of the messages. no more console_cont_flush().

> > we still can have pr_cont() happening on several cpus simultaneously.
> > console_seq is getting reset, when we register a new CON_PRINTBUFFER
> > console.
> 
> I agree that mixing part of lines from different processes/cpus
> is not ideal. But it is not much worse than mixing whole lines.
> We should do a best effort but we do not need to be perfect.

sure, thanks! I'll try to think of something other than "throw it away".
but.., well, you know my opinion by now :)

	-ss

[toc] | [prev] | [next] | [standalone]

#1442498

On Wed 2016-07-13 22:24:38, Sergey Senozhatsky wrote:
> Hello,
> 
> On (07/13/16 13:14), Petr Mladek wrote:
> [..]
> > > > [ 4913.000310] item8750item8751item8752item8753item8754item8755item8756item8757item8758item8759
> > > > [ 4913.000327] item8760item8761item8762item8763item8764item8765item8766item8767item8768item8769
> > > > [ 4913.000342] item8770item8771item8772item8773item8774item8775item8776item8777item8778item8779
> > > > [ 4913.000356] item8780
> > > > [ 4913.000357] item8781
> > > > [ 4913.000358] item8782
> > > > [...]
> > > 
> > > hm.. so are there any 'concurrent' printk()-s coming from other CPUs
> > > that are not getting printed on the console (because of loglevel restrictions),
> > > but still screw up the cont buffer?.... otherwise, my expectation was that in
> > > this particular case cpu issues a new pr_cont() only after it has printed
> > > the current message via call_console_drivers()->write(). so partially flushed
> > > cont buffer was not really expected to happen. I was wrong, obviously.
> > 
> > To be honest. I am not 100% sure what happens here. One theory is
> > that the printk kthread is waken because of some previous
> > non-continuous message. It steals console_lock() and partially flushes
> > the cont buffer. In this case, the pr_cont() calls are not able to get
> > console_lock() and basically work in the async mode. The pr_cont()
> > calls have to store each piece sepatately because the partially
> > flushed cont buffer is blocked until flushed completely.
> > 
> > Hmm, the strange thing is that I see this problem even when I force
> > the global synch more by
> > 
> >     echo Y >  /sys/module/printk/parameters/synchronous
> 
> oh, even in sync mode. hm...

It seems that the rsyslogd daemon steps in. I have dumped all
processes that called console_cont_flush() and really flushed
something. It was mostly the process that triggered the test
for-cycle and called console_unlock() from every pr_cont
the syncronous way. But there was the following sinner
from time to time:

   rs:main Q:Reg-771   [001] d... 87486.753353: console_unlock:
 rs:main Q:Reg calling console_cont_flush
rs:main Q:Reg-771   [001] d... 87486.753360: <stack trace>
 => console_unlock
 => do_con_write.part.22
 => con_write
 => do_output_char
 => n_tty_write
 => tty_write
 => __vfs_write
 => vfs_write
 => SyS_write
 => entry_SYSCALL_64_fastpath

It means that some other process does console_unlock() and blocks
using the cont buffer.

I am not sure what is the exact purpose of this syscall. But
it seems reasonable that rsystemlog is active when there is
some activity in the kernel log buffer.


> > > just an idea.
> > > ... or try to make KERN_CONT SMP-safe. there are many pr_cont() call
> > > sites. ACPI is one notable example. the others include OOM, some cgroup
> > > related output (or... was it memcg), etc., etc.
> > > 
> > > so we *may be* can have a per-cpu cont buffer and add new API
> > > pr_cont_begin()/pr_cont_end(), that would disable preemption.
> > > 
> > > 
> > > +	pr_cont_begin() /* preempt_disable() */
> > > 
> > > 	for (.....)
> > > 		pr_cont("%pS ....);
> > > 
> > > +	pr_cont_end() /* preempt_enable() */
> > >
> > > pr_cont_end() also can flush this CPU's cont buffer and store the log
> > > line. this will probably break very long cont lines (not sure if we
> > > have any of those though). and may be flush_on_panic() would have to
> > > do some extra work iterating each cpu.
> > 
> > It would work but I am a bit scared by the complexity. I think
> > that we should find a compromise between complexity and
> > reliability.
> 
> I understand your concerns.
> but, realistically, KERN_CONT will still be SMP unsafe and will not do
> what people probably expect it to do: e.g. ACPI_ERROR or ACPI_anything.
> it does acpi_os_vprintf() -> printk(KERN_CONT).
> 
> the whole thing is already complicated, we already have all those len,
> cons etc. checks in various places, it also has that owner task, etc.
> 
> static struct cont {
> 	char buf[LOG_LINE_MAX];
> 	size_t len;			/* length == 0 means unused buffer */
> 	size_t cons;			/* bytes written to console */
> 	struct task_struct *owner;	/* task of first print*/
> 	u64 ts_nsec;			/* time of first print */
> 	u8 level;			/* log level of first message */
> 	u8 facility;			/* log facility of first message */
> 	enum log_flags flags;		/* prefix, newline flags */
> 	bool flushed:1;			/* buffer sealed and committed */
> } cont;
> 
> so I think, personally... it's sort of kind of not exactly what people
> need (aka 'broken'). so my idea is to forbid concurrent cont buffer usage.
> each CPU will have it's own buffer, and touch it with preemption_disabled.
> there will be no more partial flushes, cont buffer will be flushed when it's
> done -- via pr_cont_end()->log_store()->wake_up(printk_kthread). IOW it will
> just add the cont buffer content to log_buf and print it via console_unlock(),
> like the rest of the messages. no more console_cont_flush().

I think that people primary wants to have the messages stored in the
log buffer and visible on the console. Also they expect that printk
will be an easy interface.

Any more complicated code and more temporary buffers will increase
the risk that a message will get lost somewhere and it will be hard
to dig it from a crashdump, ...

I would be great to get a perfect log. But I am afraid that we would
need to redesign the printk code first to keep it maintainable.

I am still thinking if we could do better with the single cont
buffer.

BTW: I think that the LOG_CONT/LOG_PREFIX/LOG_NEWLINE informations
are stored correctly in the ring buffer. I think that dmesg
could do a better job when showing parts of the continuous messages
that are were stored separately.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web