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

Groups > linux.kernel > #1331237 > unrolled thread

Re: [PATCH] printk: avoid livelock if another CPU printks continuously

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] printk: avoid livelock if another CPU printks  continuously Petr Mladek <pmladek@suse.com> - 2016-02-10 15:50 +0100
    Re: [PATCH] printk: avoid livelock if another CPU printks  continuously Petr Mladek <pmladek@suse.com> - 2016-02-10 17:20 +0100
      Re: [PATCH] printk: avoid livelock if another CPU printks  continuously Steven Rostedt <rostedt@goodmis.org> - 2016-02-10 17:30 +0100
        Re: [PATCH] printk: avoid livelock if another CPU printks  continuously Peter Hurley <peter@hurleysoftware.com> - 2016-02-10 18:00 +0100
        Re: [PATCH] printk: avoid livelock if another CPU printks  continuously Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-02-11 09:30 +0100
          Re: [PATCH] printk: avoid livelock if another CPU printks  continuously Petr Mladek <pmladek@suse.com> - 2016-02-11 12:50 +0100

#1331237 — Re: [PATCH] printk: avoid livelock if another CPU printks continuously

Sent again with a correct mail header, hopefully.

On Mon 2016-02-08 21:35:03, Denys Vlasenko wrote:
> At the end of each printk(), kernel attempts to take console_sem.
> If this succeeds, it feeds buffered message data to console devices
> until there is nothing left, and releases console_sem:
> 
>         if (console_trylock_for_printk(this_cpu))
>                 console_unlock();
> 
> The livelock exists because code in console_unlock() has no
> limit on the amount of buffered data it would process under
> console_sem. This is bad if printk() was called with IRQs disabled.
> 
> This patch makes console_unlock() release console_sem after 5
> iterations, which usually amounts to 5 lines of printk messages,
> and give other printk'ing CPUs a chance to acquire console_sem.
> 
> If some CPU grabs it, console_unlock() finishes.
> If no one takes the semaphore, console_unlock() re-acquires it
> and loops back for another cycle of console output.
> 
> This seems to be a hard-to-trigger, but long-existing problem:

Yup, and there are more people trying to handle this. I add some
of them into CC.

Sadly, the problem is much more complicated that it looks. Jan Kara
(jack) has already provided many possible solutions that were not
accepted. The last one can be seen at
http://thread.gmane.org/gmane.linux.kernel/2105183/focus=2113787

See below some comments to your approach.

> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: linux-kernel@vger.kernel.org
> CC: srostedt@redhat.com
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Tejun Heo <tj@kernel.org>
> CC: Peter Hurley <peter@hurleysoftware.com>
> ---
>  kernel/printk/printk.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index c963ba5..ca4f9d55 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2235,6 +2235,7 @@ void console_unlock(void)
>  	unsigned long flags;
>  	bool wake_klogd = false;
>  	bool do_cond_resched, retry;
> +	unsigned cnt;
>  
>  	if (console_suspended) {
>  		up_console_sem();
> @@ -2257,6 +2258,7 @@ void console_unlock(void)
>  	/* flush buffered message fragment immediately to console */
>  	console_cont_flush(text, sizeof(text));
>  again:
> +	cnt = 5;
>  	for (;;) {
>  		struct printk_log *msg;
>  		size_t ext_len = 0;
> @@ -2284,6 +2286,9 @@ skip:
>  		if (console_seq == log_next_seq)
>  			break;
>  
> +		if (--cnt == 0)
> +			break;	/* Someone else printk's like crazy */
> +
>  		msg = log_from_idx(console_idx);
>  		if (msg->flags & LOG_NOCONS) {
>  			/*
> @@ -2350,6 +2355,26 @@ skip:
>  	if (retry && console_trylock())
>  		goto again;
>  
> +	if (cnt == 0) {
> +		/*
> +		 * Other CPU(s) printk like crazy, filling log_buf[].
> +		 * Try to get rid of the "honor" of servicing their data:
> +		 * give _them_ time to grab console_sem and start working.
> +		 */
> +		cnt = 9999;
> +		while (--cnt != 0) {
> +			cpu_relax();
> +			if (console_seq == log_next_seq) {

This condition is true when all available messages are printed to
the console. It means that there is nothing to do at all. It is
quite late. A much better solution would be to store console_seq
to a local variable and check it is being modified by an other CPU.


> +				/* Good, other CPU entered "for(;;)" loop */
> +				goto out;
> +			}
> +		}
> +		/* No one seems to be willing to take it... */
> +		if (console_trylock())
> +			goto again; /* we took it */
> +		/* Nope, someone else holds console_sem! Good */

The cycle gives a big chance other CPUs to enter console_unlock().
It means that more CPUs might end up in the above busy cycle.

It gives a chance to move the printing to another CPU. It likely
slows down the flood of messages because the producer end up
here as well.

So, it probably works but the performance is far from optimal.
Many CPUs might end up doing nothing. I am afraid that this is
not the right way to go.

Best Regards,
Petr

[toc] | [next] | [standalone]

#1331299

On Wed 2016-02-10 09:44:07, Steven Rostedt wrote:
> On Wed, 10 Feb 2016 15:36:49 +0100
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > Bcc: 
> > Subject: Re: [PATCH] printk: avoid livelock if another CPU printks
> >  continuously
> > Reply-To: 
> > In-Reply-To: <1454963703-20433-1-git-send-email-dvlasenk@redhat.com>
> > 
> 
> Hmm, playing with mail headers?

Yeah. I am trying to do this reply back in the original thread.

> > > +				/* Good, other CPU entered "for(;;)" loop */
> > > +				goto out;
> > > +			}
> > > +		}
> > > +		/* No one seems to be willing to take it... */
> > > +		if (console_trylock())
> > > +			goto again; /* we took it */
> > > +		/* Nope, someone else holds console_sem! Good */  
> > 
> > The cycle gives a big chance other CPUs to enter console_unlock().
> > It means that more CPUs might end up in the above busy cycle.
> > 
> > It gives a chance to move the printing to another CPU. It likely
> > slows down the flood of messages because the producer end up
> > here as well.
> > 
> > So, it probably works but the performance is far from optimal.
> > Many CPUs might end up doing nothing. I am afraid that this is
> > not the right way to go.
> 
> Note, it's not that performance critical, and the loop only happens if
> someone else is adding to the console, which hopefully, should be rare.

I probably used too strong words. It is possible that the performance
impact will not be critical. But the behavior is non-deterministic.
I think that the approach taken by Jack is more promising.
I mean the offloading of the console stuff to a workqueue.

Best Regards,
Petr

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

#1331314

On Wed, 10 Feb 2016 17:10:16 +0100
Petr Mladek <pmladek@suse.com> wrote:

> > Note, it's not that performance critical, and the loop only happens if
> > someone else is adding to the console, which hopefully, should be rare.  
> 
> I probably used too strong words. It is possible that the performance
> impact will not be critical. But the behavior is non-deterministic.
> I think that the approach taken by Jack is more promising.
> I mean the offloading of the console stuff to a workqueue.

My worry about that is that it never comes out. The point about printk,
is that it should pretty much be guaranteed to print. If the system is
dying, and we push it off to a work queue, and that workqueue never
runs, then we lose critical data.

-- Steve

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

#1331329

On 02/10/2016 08:25 AM, Steven Rostedt wrote:
> On Wed, 10 Feb 2016 17:10:16 +0100
> Petr Mladek <pmladek@suse.com> wrote:
> 
>>> Note, it's not that performance critical, and the loop only happens if
>>> someone else is adding to the console, which hopefully, should be rare.  
>>
>> I probably used too strong words. It is possible that the performance
>> impact will not be critical. But the behavior is non-deterministic.
>> I think that the approach taken by Jack is more promising.
>> I mean the offloading of the console stuff to a workqueue.
> 
> My worry about that is that it never comes out. The point about printk,
> is that it should pretty much be guaranteed to print. If the system is
> dying, and we push it off to a work queue, and that workqueue never
> runs, then we lose critical data.

I agree.

I thought a more promising approach was Pan Xinhui's patch from August [1]
which hands off console output to the incoming cpu. The reqd state machine
is described in more detail in the revised patch [2].

Unfortunately, the patch was abandoned. I think he may have misunderstood
when I also referred to Jack's patches; I meant to simply draw attention
to concurrent work for review and comparison.


[1] original https://lkml.org/lkml/2015/8/11/333
[2] revised https://lkml.org/lkml/2015/8/12/321

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

#1331729

Hello,
Thanks for Cc-ing, and sorry for long reply, I'm traveling now.

On (02/10/16 11:25), Steven Rostedt wrote:
> On Wed, 10 Feb 2016 17:10:16 +0100
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > > Note, it's not that performance critical, and the loop only happens if
> > > someone else is adding to the console, which hopefully, should be rare.  
> > 
> > I probably used too strong words. It is possible that the performance
> > impact will not be critical. But the behavior is non-deterministic.
> > I think that the approach taken by Jack is more promising.
> > I mean the offloading of the console stuff to a workqueue.
> 
> My worry about that is that it never comes out. The point about printk,
> is that it should pretty much be guaranteed to print. If the system is
> dying, and we push it off to a work queue, and that workqueue never
> runs, then we lose critical data.

correct, IIRC Jan agreed to switch to 'direct' (current behaviour) printk when
one of the CPUs calls panic() (we still can use that approach even with
workqueue based printk)
	http://marc.info/?l=linux-kernel&m=145200464309562

the other thing with workqueues based approach is that all of them can be 'blocked'
in some OOM cases, so sort of fallback mechanism is also needed here
	http://marc.info/?l=linux-kernel&m=145251885502488

	-ss

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

#1331842

On Thu 2016-02-11 17:21:12, Sergey Senozhatsky wrote:
> Hello,
> Thanks for Cc-ing, and sorry for long reply, I'm traveling now.
> 
> On (02/10/16 11:25), Steven Rostedt wrote:
> > On Wed, 10 Feb 2016 17:10:16 +0100
> > Petr Mladek <pmladek@suse.com> wrote:
> > 
> > > > Note, it's not that performance critical, and the loop only happens if
> > > > someone else is adding to the console, which hopefully, should be rare.  
> > > 
> > > I probably used too strong words. It is possible that the performance
> > > impact will not be critical. But the behavior is non-deterministic.
> > > I think that the approach taken by Jack is more promising.
> > > I mean the offloading of the console stuff to a workqueue.
> > 
> > My worry about that is that it never comes out. The point about printk,
> > is that it should pretty much be guaranteed to print. If the system is
> > dying, and we push it off to a work queue, and that workqueue never
> > runs, then we lose critical data.
> 
> correct, IIRC Jan agreed to switch to 'direct' (current behaviour) printk when
> one of the CPUs calls panic() (we still can use that approach even with
> workqueue based printk)
> 	http://marc.info/?l=linux-kernel&m=145200464309562

Yup.

> the other thing with workqueues based approach is that all of them can be 'blocked'
> in some OOM cases, so sort of fallback mechanism is also needed here
> 	http://marc.info/?l=linux-kernel&m=145251885502488

If this proves to be a problem. We could always use a workqueue with a
rescue worker.

Regarding the patch from Pan Xinhui. My main problem with it is that
it adds many handshakes and twists to the already complicated printk
code. Also it does not solve the problem if the flood of messages
comes entirely from an IRQ context.

Workqueues code is not trivial but mature. And the usage of the workqueues
in printk is quite straightforward.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web