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

Groups > linux.kernel > #1557435 > unrolled thread

Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying.

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] mm/page_alloc: Wait for oom_lock before retrying. Petr Mladek <pmladek@suse.com> - 2017-01-12 14:20 +0100
    Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying. Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-01-13 04:00 +0100
      Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying. Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-01-13 05:00 +0100
        Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying. Petr Mladek <pmladek@suse.com> - 2017-01-13 12:20 +0100
      Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying. Petr Mladek <pmladek@suse.com> - 2017-01-13 12:20 +0100

#1557435 — Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying.

On Mon 2016-12-26 20:34:07, Sergey Senozhatsky wrote:
> Cc Greg, Jiri,
> 
> On (12/26/16 19:54), Tetsuo Handa wrote:
> [..]
> > 
> > (3) I got below warning. (Though not reproducible.)
> >     If fb_flashcursor() called console_trylock(), console_may_schedule is set to 1?
> 
> hmmm... it takes an atomic/spin `printing_lock' lock in vt_console_print(),
> then call console_conditional_schedule() from lf(), being under spin_lock.
> `console_may_schedule' in console_conditional_schedule() still keeps the
> value from console_trylock(), which was ok (console_may_schedule permits
> rescheduling). but preemption got changed under console_trylock(), by
> that spin_lock.
> 
> console_trylock() used to always forbid rescheduling; but it got changed
> like a yaer ago.
> 
> the other thing is... do we really need to console_conditional_schedule()
> from fbcon_*()? console_unlock() does cond_resched() after every line it
> prints. wouldn't that be enough?
> 
> so may be we can drop some of console_conditional_schedule()
> call sites in fbcon. or update console_conditional_schedule()
> function to always return the current preemption value, not the
> one we saw in console_trylock().
> 
> (not tested)
> 
> ---
> 
>  kernel/printk/printk.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 8b2696420abb..ad4a02cf9f15 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2075,6 +2075,24 @@ static int console_cpu_notify(unsigned int cpu)
>  	return 0;
>  }
>  
> +static int get_console_may_schedule(void)
> +{
> +	/*
> +	 * When PREEMPT_COUNT disabled we can't reliably detect if it's
> +	 * safe to schedule (e.g. calling printk while holding a spin_lock),
> +	 * because preempt_disable()/preempt_enable() are just barriers there
> +	 * and preempt_count() is always 0.
> +	 *
> +	 * RCU read sections have a separate preemption counter when
> +	 * PREEMPT_RCU enabled thus we must take extra care and check
> +	 * rcu_preempt_depth(), otherwise RCU read sections modify
> +	 * preempt_count().
> +	 */
> +	return !oops_in_progress &&
> +		preemptible() &&
> +		!rcu_preempt_depth();
> +}
> +
>  /**
>   * console_lock - lock the console system for exclusive use.
>   *
> @@ -2316,7 +2321,7 @@ EXPORT_SYMBOL(console_unlock);
>   */
>  void __sched console_conditional_schedule(void)
>  {
> -	if (console_may_schedule)
> +	if (get_console_may_schedule())

Note that console_may_schedule should be zero when
the console drivers are called. See the following lines in
console_unlock():

	/*
	 * Console drivers are called under logbuf_lock, so
	 * @console_may_schedule should be cleared before; however, we may
	 * end up dumping a lot of lines, for example, if called from
	 * console registration path, and should invoke cond_resched()
	 * between lines if allowable.  Not doing so can cause a very long
	 * scheduling stall on a slow console leading to RCU stall and
	 * softlockup warnings which exacerbate the issue with more
	 * messages practically incapacitating the system.
	 */
	do_cond_resched = console_may_schedule;
	console_may_schedule = 0;

IMHO, there is the problem described by Tetsuo in the other mail.
We do not call the above lines when the console semaphore is
re-taken and we do the main cycle again:

	/*
	 * Someone could have filled up the buffer again, so re-check if there's
	 * something to flush. In case we cannot trylock the console_sem again,
	 * there's a new owner and the console_unlock() from them will do the
	 * flush, no worries.
	 */
	raw_spin_lock(&logbuf_lock);
	retry = console_seq != log_next_seq;
	raw_spin_unlock_irqrestore(&logbuf_lock, flags);

	if (retry && console_trylock())
		goto again;


Well, simply moving the again: label is not correct as well.
The global variable is explicitly set in some functions:

	console_lock[2094]             console_may_schedule = 1;
	console_unblank[2339]          console_may_schedule = 0;
	console_flush_on_panic[2361]   console_may_schedule = 0;

But console_try_lock() will set it according to the real context
in console_unlock().


Hmm, the enforced values were there for ages (even in the initial
git commit). It was always 0 also console_trylock() until
the commit 6b97a20d3a7909daa06625d ("printk: set may_schedule for some
of console_trylock() callers").

It might make sense to completely remove the global
@console_may_schedule variable and always decide
by the context. It is slightly suboptimal. But it
simplifies the code and should be sane in all situations.

Sergey, if you agree with the above paragraph. Do you want to prepare
the patch or should I do so?

Best Regards,
Petr

[toc] | [next] | [standalone]

#1557973

On (01/12/17 14:10), Petr Mladek wrote:
[..]
> >  /**
> >   * console_lock - lock the console system for exclusive use.
> >   *
> > @@ -2316,7 +2321,7 @@ EXPORT_SYMBOL(console_unlock);
> >   */
> >  void __sched console_conditional_schedule(void)
> >  {
> > -	if (console_may_schedule)
> > +	if (get_console_may_schedule())
> 
> Note that console_may_schedule should be zero when
> the console drivers are called. See the following lines in
> console_unlock():
> 
> 	/*
> 	 * Console drivers are called under logbuf_lock, so
> 	 * @console_may_schedule should be cleared before; however, we may
> 	 * end up dumping a lot of lines, for example, if called from
> 	 * console registration path, and should invoke cond_resched()
> 	 * between lines if allowable.  Not doing so can cause a very long
> 	 * scheduling stall on a slow console leading to RCU stall and
> 	 * softlockup warnings which exacerbate the issue with more
> 	 * messages practically incapacitating the system.
> 	 */
> 	do_cond_resched = console_may_schedule;
> 	console_may_schedule = 0;



console drivers are never-ever-ever getting called under logbuf lock.
never. with disabled local IRQs - yes. under logbuf lock - no. that
would soft lockup systems in really bad ways, otherwise.

the reason why we set console_may_schedule to zero in
console_unlock() is.... VT. and lf() function in particular.

commit 78944e549d36673eb6265a2411574e79c28e23dc
Author: Antonino A. Daplas XXXX
Date:   Sat Aug 5 12:14:16 2006 -0700

    [PATCH] vt: printk: Fix framebuffer console triggering might_sleep assertion
    
    Reported by: Dave Jones
    
    Whilst printk'ing to both console and serial console, I got this...
    (2.6.18rc1)
    
    BUG: sleeping function called from invalid context at kernel/sched.c:4438
    in_atomic():0, irqs_disabled():1
    
    Call Trace:
     [<ffffffff80271db8>] show_trace+0xaa/0x23d
     [<ffffffff80271f60>] dump_stack+0x15/0x17
     [<ffffffff8020b9f8>] __might_sleep+0xb2/0xb4
     [<ffffffff8029232e>] __cond_resched+0x15/0x55
     [<ffffffff80267eb8>] cond_resched+0x3b/0x42
     [<ffffffff80268c64>] console_conditional_schedule+0x12/0x14
     [<ffffffff80368159>] fbcon_redraw+0xf6/0x160
     [<ffffffff80369c58>] fbcon_scroll+0x5d9/0xb52
     [<ffffffff803a43c4>] scrup+0x6b/0xd6
     [<ffffffff803a4453>] lf+0x24/0x44
     [<ffffffff803a7ff8>] vt_console_print+0x166/0x23d
     [<ffffffff80295528>] __call_console_drivers+0x65/0x76
     [<ffffffff80295597>] _call_console_drivers+0x5e/0x62
     [<ffffffff80217e3f>] release_console_sem+0x14b/0x232
     [<ffffffff8036acd6>] fb_flashcursor+0x279/0x2a6
     [<ffffffff80251e3f>] run_workqueue+0xa8/0xfb
     [<ffffffff8024e5e0>] worker_thread+0xef/0x122
     [<ffffffff8023660f>] kthread+0x100/0x136
     [<ffffffff8026419e>] child_rip+0x8/0x12


and we really don't want to cond_resched() when we are in panic.
that's why console_flush_on_panic() sets it to zero explicitly.

console_trylock() checks oops_in_progress, so re-taking the semaphore
when we are in

	panic()
	 console_flush_on_panic()
          console_unlock()
           console_trylock()

should be OK. as well as doing get_console_conditional_schedule() somewhere
in console driver code.


I still don't understand why do you guys think we can't simply do
get_console_conditional_schedule() and get the actual value.


[..]

> Sergey, if you agree with the above paragraph. Do you want to prepare
> the patch or should I do so?

I'm on it.

	-ss

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

#1557998

On (01/13/17 11:52), Sergey Senozhatsky wrote:
[..]
> and we really don't want to cond_resched() when we are in panic.
> that's why console_flush_on_panic() sets it to zero explicitly.
> 
> console_trylock() checks oops_in_progress, so re-taking the semaphore
> when we are in
> 
> 	panic()
> 	 console_flush_on_panic()
>           console_unlock()
>            console_trylock()
> 
> should be OK. as well as doing get_console_conditional_schedule() somewhere
> in console driver code.

d'oh... no, this is false. console_flush_on_panic() is called after we
bust_spinlocks(0), BUT with local IRQs disabled. so console_trylock()
would still set console_may_schedule to 0.

	-ss

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

#1558241

On Fri 2017-01-13 12:53:07, Sergey Senozhatsky wrote:
> On (01/13/17 11:52), Sergey Senozhatsky wrote:
> [..]
> > and we really don't want to cond_resched() when we are in panic.
> > that's why console_flush_on_panic() sets it to zero explicitly.
> > 
> > console_trylock() checks oops_in_progress, so re-taking the semaphore
> > when we are in
> > 
> > 	panic()
> > 	 console_flush_on_panic()
> >           console_unlock()
> >            console_trylock()
> > 
> > should be OK. as well as doing get_console_conditional_schedule() somewhere
> > in console driver code.
> 
> d'oh... no, this is false. console_flush_on_panic() is called after we
> bust_spinlocks(0), BUT with local IRQs disabled. so console_trylock()
> would still set console_may_schedule to 0.

Ah, you found it yourself.

Best Regards,
Petr

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

#1558239

On Fri 2017-01-13 11:52:55, Sergey Senozhatsky wrote:
> On (01/12/17 14:10), Petr Mladek wrote:
> [..]
> > >  /**
> > >   * console_lock - lock the console system for exclusive use.
> > >   *
> > > @@ -2316,7 +2321,7 @@ EXPORT_SYMBOL(console_unlock);
> > >   */
> > >  void __sched console_conditional_schedule(void)
> > >  {
> > > -	if (console_may_schedule)
> > > +	if (get_console_may_schedule())
> > 
> > Note that console_may_schedule should be zero when
> > the console drivers are called. See the following lines in
> > console_unlock():
> > 
> > 	/*
> > 	 * Console drivers are called under logbuf_lock, so
> > 	 * @console_may_schedule should be cleared before; however, we may
> > 	 * end up dumping a lot of lines, for example, if called from
> > 	 * console registration path, and should invoke cond_resched()
> > 	 * between lines if allowable.  Not doing so can cause a very long
> > 	 * scheduling stall on a slow console leading to RCU stall and
> > 	 * softlockup warnings which exacerbate the issue with more
> > 	 * messages practically incapacitating the system.
> > 	 */
> > 	do_cond_resched = console_may_schedule;
> > 	console_may_schedule = 0;
> 
> 
> 
> console drivers are never-ever-ever getting called under logbuf lock.
> never. with disabled local IRQs - yes. under logbuf lock - no. that
> would soft lockup systems in really bad ways, otherwise.

Sure. It is just a misleading comment that someone wrote. I have
already fixed this in my patch.


> the reason why we set console_may_schedule to zero in
> console_unlock() is.... VT. and lf() function in particular.
> 
> commit 78944e549d36673eb6265a2411574e79c28e23dc
> Author: Antonino A. Daplas XXXX
> Date:   Sat Aug 5 12:14:16 2006 -0700
> 
>     [PATCH] vt: printk: Fix framebuffer console triggering might_sleep assertion
>     
>     Reported by: Dave Jones
>     
>     Whilst printk'ing to both console and serial console, I got this...
>     (2.6.18rc1)
>     
>     BUG: sleeping function called from invalid context at kernel/sched.c:4438
>     in_atomic():0, irqs_disabled():1

This is basically the same problem that Testuo has. This commit added
the line

	console_may_schedule = 0;

Tetsuo found that we did not clear it when going back
via the "again:" goto target.


> and we really don't want to cond_resched() when we are in panic.
> that's why console_flush_on_panic() sets it to zero explicitly.

This actually works even with the bug. console_flush_on_panic()
is called with interrupts disabled in panic(). Therefore
console_trylock would disable cond_resched.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web