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

Groups > linux.kernel > #1732829 > unrolled thread

Query regarding synchronize_sched_expedited and resched_cpu

Started byNeeraj Upadhyay <neeraju@codeaurora.org>
First post2017-09-15 13:20 +0200
Last post2017-09-21 17:50 +0200
Articles 15 on this page of 35 — 7 participants

Back to article view | Back to linux.kernel

Contents

  Query regarding synchronize_sched_expedited and resched_cpu Neeraj Upadhyay <neeraju@codeaurora.org> - 2017-09-15 13:20 +0200
    Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-17 03:10 +0200
      Re: Query regarding synchronize_sched_expedited and resched_cpu Neeraj Upadhyay <neeraju@codeaurora.org> - 2017-09-17 08:10 +0200
        Re: Query regarding synchronize_sched_expedited and resched_cpu Steven Rostedt <rostedt@goodmis.org> - 2017-09-18 17:20 +0200
          Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-18 18:10 +0200
            Re: Query regarding synchronize_sched_expedited and resched_cpu Steven Rostedt <rostedt@goodmis.org> - 2017-09-18 18:20 +0200
              Re: Query regarding synchronize_sched_expedited and resched_cpu Steven Rostedt <rostedt@goodmis.org> - 2017-09-18 18:30 +0200
                Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-18 19:00 +0200
                  Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-19 02:00 +0200
                    Re: Query regarding synchronize_sched_expedited and resched_cpu Steven Rostedt <rostedt@goodmis.org> - 2017-09-19 03:30 +0200
                      Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-19 04:30 +0200
                    Re: Query regarding synchronize_sched_expedited and resched_cpu Byungchul Park <byungchul.park@lge.com> - 2017-09-19 04:00 +0200
                      Re: Query regarding synchronize_sched_expedited and resched_cpu Byungchul Park <byungchul.park@lge.com> - 2017-09-19 04:10 +0200
                        Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-19 04:40 +0200
                          Re: Query regarding synchronize_sched_expedited and resched_cpu Byungchul Park <byungchul.park@lge.com> - 2017-09-19 04:50 +0200
                            Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-19 06:10 +0200
                              Re: Query regarding synchronize_sched_expedited and resched_cpu Boqun Feng <boqun.feng@gmail.com> - 2017-09-19 07:40 +0200
                                Re: Query regarding synchronize_sched_expedited and resched_cpu Mike Galbraith <efault@gmx.de> - 2017-09-19 08:20 +0200
                                  Re: Query regarding synchronize_sched_expedited and resched_cpu Byungchul Park <byungchul.park@lge.com> - 2017-09-19 09:00 +0200
                                  Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-19 15:50 +0200
                  Re: Query regarding synchronize_sched_expedited and resched_cpu Peter Zijlstra <peterz@infradead.org> - 2017-09-21 16:00 +0200
                    Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-21 17:40 +0200
                Re: Query regarding synchronize_sched_expedited and resched_cpu Byungchul Park <byungchul.park@lge.com> - 2017-09-19 04:00 +0200
              Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-18 18:30 +0200
                Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-19 17:40 +0200
                  Re: Query regarding synchronize_sched_expedited and resched_cpu Steven Rostedt <rostedt@goodmis.org> - 2017-09-19 18:00 +0200
                    Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-19 18:20 +0200
                  Re: Query regarding synchronize_sched_expedited and resched_cpu Peter Zijlstra <peterz@infradead.org> - 2017-09-21 16:10 +0200
                    Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-21 18:10 +0200
                      Re: Query regarding synchronize_sched_expedited and resched_cpu Peter Zijlstra <peterz@infradead.org> - 2017-09-21 18:40 +0200
                        Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-21 18:50 +0200
          Re: Query regarding synchronize_sched_expedited and resched_cpu Peter Zijlstra <peterz@infradead.org> - 2017-09-21 16:00 +0200
            Re: Query regarding synchronize_sched_expedited and resched_cpu "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-21 17:40 +0200
              Re: Query regarding synchronize_sched_expedited and resched_cpu Peter Zijlstra <peterz@infradead.org> - 2017-09-21 18:20 +0200
            Re: Query regarding synchronize_sched_expedited and resched_cpu Steven Rostedt <rostedt@goodmis.org> - 2017-09-21 17:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1736682

FromPeter Zijlstra <peterz@infradead.org>
Date2017-09-21 16:00 +0200
Message-ID<usabh-1IE-43@gated-at.bofh.it>
In reply to#1734303
On Mon, Sep 18, 2017 at 09:55:27AM -0700, Paul E. McKenney wrote:
> On Mon, Sep 18, 2017 at 12:29:31PM -0400, Steven Rostedt wrote:
> > On Mon, 18 Sep 2017 09:24:12 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > 
> > 
> > > As soon as I work through the backlog of lockdep complaints that
> > > appeared in the last merge window...  :-(
> > > 
> > > sparse_irq_lock, I am looking at you!!!  ;-)

That one is a false positive and I have send patches to address.

> > I just hit one too, and decided to write a patch to show a chain of 3
> > when applicable.
> > 
> > For example:
> > 
> >  Chain exists of:
> >    cpu_hotplug_lock.rw_sem --> smpboot_threads_lock --> (complete)&self->parked
> > 
> >   Possible unsafe locking scenario by crosslock:
> > 
> >         CPU0                    CPU1                    CPU2
> >         ----                    ----                    ----
> >    lock(smpboot_threads_lock);
> >    lock((complete)&self->parked);
> >                                 lock(cpu_hotplug_lock.rw_sem);
> >                                 lock(smpboot_threads_lock);
> >                                                        lock(cpu_hotplug_lock.rw_sem);
> >                                                        unlock((complete)&self->parked);
> > 
> >   *** DEADLOCK ***
> > 
> > :-)
> 
> Nice!!!

That one looks like the watchdog thing, and Thomas was poking at that.

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

#1736792

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-09-21 17:40 +0200
Message-ID<usbK3-2QA-35@gated-at.bofh.it>
In reply to#1736682
On Thu, Sep 21, 2017 at 03:57:49PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 18, 2017 at 09:55:27AM -0700, Paul E. McKenney wrote:
> > On Mon, Sep 18, 2017 at 12:29:31PM -0400, Steven Rostedt wrote:
> > > On Mon, 18 Sep 2017 09:24:12 -0700
> > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > 
> > > 
> > > > As soon as I work through the backlog of lockdep complaints that
> > > > appeared in the last merge window...  :-(
> > > > 
> > > > sparse_irq_lock, I am looking at you!!!  ;-)
> 
> That one is a false positive and I have send patches to address.

I did try them out, and they work fine for me when lockdep is enabled.
I get build failures if lockdep is not enabled.  Things are a bit crazy
here, so I have not had a chance to try to fix them, though it should
not be a big deal (easy for me to say!).

> > > I just hit one too, and decided to write a patch to show a chain of 3
> > > when applicable.
> > > 
> > > For example:
> > > 
> > >  Chain exists of:
> > >    cpu_hotplug_lock.rw_sem --> smpboot_threads_lock --> (complete)&self->parked
> > > 
> > >   Possible unsafe locking scenario by crosslock:
> > > 
> > >         CPU0                    CPU1                    CPU2
> > >         ----                    ----                    ----
> > >    lock(smpboot_threads_lock);
> > >    lock((complete)&self->parked);
> > >                                 lock(cpu_hotplug_lock.rw_sem);
> > >                                 lock(smpboot_threads_lock);
> > >                                                        lock(cpu_hotplug_lock.rw_sem);
> > >                                                        unlock((complete)&self->parked);
> > > 
> > >   *** DEADLOCK ***
> > > 
> > > :-)
> > 
> > Nice!!!
> 
> That one looks like the watchdog thing, and Thomas was poking at that.

For whatever it is worth, I am still chasing lost-timer bugs.  I now know
of a large number of things that are not the cause of the problem.  :-/

							Thanx, Paul

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

#1734593

FromByungchul Park <byungchul.park@lge.com>
Date2017-09-19 04:00 +0200
Message-ID<urfZn-66X-3@gated-at.bofh.it>
In reply to#1734283
On Mon, Sep 18, 2017 at 12:29:31PM -0400, Steven Rostedt wrote:
> On Mon, 18 Sep 2017 09:24:12 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> 
> > As soon as I work through the backlog of lockdep complaints that
> > appeared in the last merge window...  :-(
> > 
> > sparse_irq_lock, I am looking at you!!!  ;-)
> 
> I just hit one too, and decided to write a patch to show a chain of 3
> when applicable.

Hello Steven,

I really agree with this. Currently, in case that more than two locks
participates in a deadlock, the report informs insuffucuently.

Thanks,
Byungchul

> 
> For example:
> 
>  Chain exists of:
>    cpu_hotplug_lock.rw_sem --> smpboot_threads_lock --> (complete)&self->parked
>  
>   Possible unsafe locking scenario by crosslock:
>  
>         CPU0                    CPU1                    CPU2
>         ----                    ----                    ----
>    lock(smpboot_threads_lock);
>    lock((complete)&self->parked);
>                                 lock(cpu_hotplug_lock.rw_sem);
>                                 lock(smpboot_threads_lock);
>                                                        lock(cpu_hotplug_lock.rw_sem);
>                                                        unlock((complete)&self->parked);
>  
>   *** DEADLOCK ***
> 
> :-)
> 
> -- Steve

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

#1734286

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-09-18 18:30 +0200
Message-ID<ur75M-b2-41@gated-at.bofh.it>
In reply to#1734274
On Mon, Sep 18, 2017 at 12:12:13PM -0400, Steven Rostedt wrote:
> On Mon, 18 Sep 2017 09:01:25 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> 
> >     sched: Make resched_cpu() unconditional
> >     
> >     The current implementation of synchronize_sched_expedited() incorrectly
> >     assumes that resched_cpu() is unconditional, which it is not.  This means
> >     that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
> >     fails as follows (analysis by Neeraj Upadhyay):
> >     
> >     o    CPU1 is waiting for expedited wait to complete:
> >     sync_rcu_exp_select_cpus
> >          rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
> >          IPI sent to CPU5
> >     
> >     synchronize_sched_expedited_wait
> >              ret = swait_event_timeout(
> >                                          rsp->expedited_wq,
> >       sync_rcu_preempt_exp_done(rnp_root),
> >                                          jiffies_stall);
> >     
> >                 expmask = 0x20 , and CPU 5 is in idle path (in cpuidle_enter())
> >     
> >     o    CPU5 handles IPI and fails to acquire rq lock.
> >     
> >     Handles IPI
> >          sync_sched_exp_handler
> >              resched_cpu
> >                  returns while failing to try lock acquire rq->lock
> >              need_resched is not set
> >     
> >     o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
> >          idle (schedule() is not called).
> >     
> >     o    CPU 1 reports RCU stall.
> >     
> >     Given that resched_cpu() is used only by RCU, this commit fixes the
> >     assumption by making resched_cpu() unconditional.
> 
> Probably want to run this with several workloads with lockdep enabled
> first.

As soon as I work through the backlog of lockdep complaints that
appeared in the last merge window...  :-(

sparse_irq_lock, I am looking at you!!!  ;-)

							Thanx, Paul

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

#1735018

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-09-19 17:40 +0200
Message-ID<ursMW-7dF-25@gated-at.bofh.it>
In reply to#1734286
On Mon, Sep 18, 2017 at 09:24:12AM -0700, Paul E. McKenney wrote:
> On Mon, Sep 18, 2017 at 12:12:13PM -0400, Steven Rostedt wrote:
> > On Mon, 18 Sep 2017 09:01:25 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > 
> > 
> > >     sched: Make resched_cpu() unconditional
> > >     
> > >     The current implementation of synchronize_sched_expedited() incorrectly
> > >     assumes that resched_cpu() is unconditional, which it is not.  This means
> > >     that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
> > >     fails as follows (analysis by Neeraj Upadhyay):
> > >     
> > >     o    CPU1 is waiting for expedited wait to complete:
> > >     sync_rcu_exp_select_cpus
> > >          rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
> > >          IPI sent to CPU5
> > >     
> > >     synchronize_sched_expedited_wait
> > >              ret = swait_event_timeout(
> > >                                          rsp->expedited_wq,
> > >       sync_rcu_preempt_exp_done(rnp_root),
> > >                                          jiffies_stall);
> > >     
> > >                 expmask = 0x20 , and CPU 5 is in idle path (in cpuidle_enter())
> > >     
> > >     o    CPU5 handles IPI and fails to acquire rq lock.
> > >     
> > >     Handles IPI
> > >          sync_sched_exp_handler
> > >              resched_cpu
> > >                  returns while failing to try lock acquire rq->lock
> > >              need_resched is not set
> > >     
> > >     o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
> > >          idle (schedule() is not called).
> > >     
> > >     o    CPU 1 reports RCU stall.
> > >     
> > >     Given that resched_cpu() is used only by RCU, this commit fixes the
> > >     assumption by making resched_cpu() unconditional.
> > 
> > Probably want to run this with several workloads with lockdep enabled
> > first.
> 
> As soon as I work through the backlog of lockdep complaints that
> appeared in the last merge window...  :-(

And this patch survived all rcutorture scenarios, including those with
lockdep enabled.  There were failures, but these are pre-existing issues
I am chasing:  Lost timeouts on TREE01 and rt_mutex trying to awaken
an offline CPU in TREE03.

So I have this one queued.  Objections?

							Thanx, Paul

------------------------------------------------------------------------

commit bc43e2e7e08134e6f403ac845edcf4f85668d803
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Sep 18 08:54:40 2017 -0700

    sched: Make resched_cpu() unconditional
    
    The current implementation of synchronize_sched_expedited() incorrectly
    assumes that resched_cpu() is unconditional, which it is not.  This means
    that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
    fails as follows (analysis by Neeraj Upadhyay):
    
    o    CPU1 is waiting for expedited wait to complete:
    sync_rcu_exp_select_cpus
         rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
         IPI sent to CPU5
    
    synchronize_sched_expedited_wait
             ret = swait_event_timeout(
                                         rsp->expedited_wq,
      sync_rcu_preempt_exp_done(rnp_root),
                                         jiffies_stall);
    
                expmask = 0x20 , and CPU 5 is in idle path (in cpuidle_enter())
    
    o    CPU5 handles IPI and fails to acquire rq lock.
    
    Handles IPI
         sync_sched_exp_handler
             resched_cpu
                 returns while failing to try lock acquire rq->lock
             need_resched is not set
    
    o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
         idle (schedule() is not called).
    
    o    CPU 1 reports RCU stall.
    
    Given that resched_cpu() is used only by RCU, this commit fixes the
    assumption by making resched_cpu() unconditional.
    
    Reported-by: Neeraj Upadhyay <neeraju@codeaurora.org>
    Suggested-by: Neeraj Upadhyay <neeraju@codeaurora.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cab8c5ec128e..b2281971894c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -505,8 +505,7 @@ void resched_cpu(int cpu)
 	struct rq *rq = cpu_rq(cpu);
 	unsigned long flags;
 
-	if (!raw_spin_trylock_irqsave(&rq->lock, flags))
-		return;
+	raw_spin_lock_irqsave(&rq->lock, flags);
 	resched_curr(rq);
 	raw_spin_unlock_irqrestore(&rq->lock, flags);
 }

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

#1735043

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-09-19 18:00 +0200
Message-ID<urt6j-7kk-31@gated-at.bofh.it>
In reply to#1735018
On Tue, 19 Sep 2017 08:31:26 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> commit bc43e2e7e08134e6f403ac845edcf4f85668d803
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Mon Sep 18 08:54:40 2017 -0700
> 
>     sched: Make resched_cpu() unconditional
>     
>     The current implementation of synchronize_sched_expedited() incorrectly
>     assumes that resched_cpu() is unconditional, which it is not.  This means
>     that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
>     fails as follows (analysis by Neeraj Upadhyay):
>     
>     o    CPU1 is waiting for expedited wait to complete:
>     sync_rcu_exp_select_cpus
>          rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
>          IPI sent to CPU5
>     
>     synchronize_sched_expedited_wait
>              ret = swait_event_timeout(
>                                          rsp->expedited_wq,
>       sync_rcu_preempt_exp_done(rnp_root),
>                                          jiffies_stall);
>     
>                 expmask = 0x20 , and CPU 5 is in idle path (in cpuidle_enter())
>     
>     o    CPU5 handles IPI and fails to acquire rq lock.
>     
>     Handles IPI
>          sync_sched_exp_handler
>              resched_cpu
>                  returns while failing to try lock acquire rq->lock
>              need_resched is not set
>     
>     o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
>          idle (schedule() is not called).
>     
>     o    CPU 1 reports RCU stall.
>     
>     Given that resched_cpu() is used only by RCU, this commit fixes the

"is now only used by RCU", as it was created for another purpose.

>     assumption by making resched_cpu() unconditional.
>     
>     Reported-by: Neeraj Upadhyay <neeraju@codeaurora.org>
>     Suggested-by: Neeraj Upadhyay <neeraju@codeaurora.org>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Cc: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index cab8c5ec128e..b2281971894c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -505,8 +505,7 @@ void resched_cpu(int cpu)
>  	struct rq *rq = cpu_rq(cpu);
>  	unsigned long flags;
>  
> -	if (!raw_spin_trylock_irqsave(&rq->lock, flags))
> -		return;
> +	raw_spin_lock_irqsave(&rq->lock, flags);
>  	resched_curr(rq);
>  	raw_spin_unlock_irqrestore(&rq->lock, flags);
>  }

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

#1735079

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-09-19 18:20 +0200
Message-ID<urtpF-7FJ-49@gated-at.bofh.it>
In reply to#1735043
On Tue, Sep 19, 2017 at 11:58:59AM -0400, Steven Rostedt wrote:
> On Tue, 19 Sep 2017 08:31:26 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > commit bc43e2e7e08134e6f403ac845edcf4f85668d803
> > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Date:   Mon Sep 18 08:54:40 2017 -0700
> > 
> >     sched: Make resched_cpu() unconditional
> >     
> >     The current implementation of synchronize_sched_expedited() incorrectly
> >     assumes that resched_cpu() is unconditional, which it is not.  This means
> >     that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
> >     fails as follows (analysis by Neeraj Upadhyay):
> >     
> >     o    CPU1 is waiting for expedited wait to complete:
> >     sync_rcu_exp_select_cpus
> >          rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
> >          IPI sent to CPU5
> >     
> >     synchronize_sched_expedited_wait
> >              ret = swait_event_timeout(
> >                                          rsp->expedited_wq,
> >       sync_rcu_preempt_exp_done(rnp_root),
> >                                          jiffies_stall);
> >     
> >                 expmask = 0x20 , and CPU 5 is in idle path (in cpuidle_enter())
> >     
> >     o    CPU5 handles IPI and fails to acquire rq lock.
> >     
> >     Handles IPI
> >          sync_sched_exp_handler
> >              resched_cpu
> >                  returns while failing to try lock acquire rq->lock
> >              need_resched is not set
> >     
> >     o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
> >          idle (schedule() is not called).
> >     
> >     o    CPU 1 reports RCU stall.
> >     
> >     Given that resched_cpu() is used only by RCU, this commit fixes the
> 
> "is now only used by RCU", as it was created for another purpose.

Good catch, fixed.

> >     assumption by making resched_cpu() unconditional.
> >     
> >     Reported-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> >     Suggested-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >     Cc: Peter Zijlstra <peterz@infradead.org>
> >     Cc: Steven Rostedt <rostedt@goodmis.org>
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Applied, thank you!

							Thanx, Paul

> -- Steve
> 
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index cab8c5ec128e..b2281971894c 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -505,8 +505,7 @@ void resched_cpu(int cpu)
> >  	struct rq *rq = cpu_rq(cpu);
> >  	unsigned long flags;
> >  
> > -	if (!raw_spin_trylock_irqsave(&rq->lock, flags))
> > -		return;
> > +	raw_spin_lock_irqsave(&rq->lock, flags);
> >  	resched_curr(rq);
> >  	raw_spin_unlock_irqrestore(&rq->lock, flags);
> >  }
>

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

#1736693

FromPeter Zijlstra <peterz@infradead.org>
Date2017-09-21 16:10 +0200
Message-ID<usakW-21y-19@gated-at.bofh.it>
In reply to#1735018
On Tue, Sep 19, 2017 at 08:31:26AM -0700, Paul E. McKenney wrote:
> So I have this one queued.  Objections?

Changelog reads like its whitespace damaged.

> commit bc43e2e7e08134e6f403ac845edcf4f85668d803
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Mon Sep 18 08:54:40 2017 -0700
> 
>     sched: Make resched_cpu() unconditional
>     
>     The current implementation of synchronize_sched_expedited() incorrectly
>     assumes that resched_cpu() is unconditional, which it is not.  This means
>     that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
>     fails as follows (analysis by Neeraj Upadhyay):
>     
>     o    CPU1 is waiting for expedited wait to complete:
>     sync_rcu_exp_select_cpus
>          rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
>          IPI sent to CPU5
>     
>     synchronize_sched_expedited_wait
>              ret = swait_event_timeout(
>                                          rsp->expedited_wq,
>       sync_rcu_preempt_exp_done(rnp_root),
>                                          jiffies_stall);
>     
>                 expmask = 0x20 , and CPU 5 is in idle path (in cpuidle_enter())
>     
>     o    CPU5 handles IPI and fails to acquire rq lock.
>     
>     Handles IPI
>          sync_sched_exp_handler
>              resched_cpu
>                  returns while failing to try lock acquire rq->lock
>              need_resched is not set
>     
>     o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
>          idle (schedule() is not called).
>     
>     o    CPU 1 reports RCU stall.
>     
>     Given that resched_cpu() is used only by RCU, this commit fixes the
>     assumption by making resched_cpu() unconditional.
>     
>     Reported-by: Neeraj Upadhyay <neeraju@codeaurora.org>
>     Suggested-by: Neeraj Upadhyay <neeraju@codeaurora.org>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Cc: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index cab8c5ec128e..b2281971894c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -505,8 +505,7 @@ void resched_cpu(int cpu)
>  	struct rq *rq = cpu_rq(cpu);
>  	unsigned long flags;
>  
> -	if (!raw_spin_trylock_irqsave(&rq->lock, flags))
> -		return;
> +	raw_spin_lock_irqsave(&rq->lock, flags);
>  	resched_curr(rq);
>  	raw_spin_unlock_irqrestore(&rq->lock, flags);
>  }
>

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

#1736820

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-09-21 18:10 +0200
Message-ID<uscd3-3f6-11@gated-at.bofh.it>
In reply to#1736693
On Thu, Sep 21, 2017 at 03:59:46PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 19, 2017 at 08:31:26AM -0700, Paul E. McKenney wrote:
> > So I have this one queued.  Objections?
> 
> Changelog reads like its whitespace damaged.

It does, now that you mention it.  How about the updated version below?

							Thanx, Paul

------------------------------------------------------------------------

commit c21c9b78182e35eb0e72ef4e3bba3054f26eaaea
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Sep 18 08:54:40 2017 -0700

    sched: Make resched_cpu() unconditional
    
    The current implementation of synchronize_sched_expedited() incorrectly
    assumes that resched_cpu() is unconditional, which it is not.  This means
    that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
    fails as follows (analysis by Neeraj Upadhyay):
    
    o	CPU1 is waiting for expedited wait to complete:
    
    	sync_rcu_exp_select_cpus
    	     rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
    	     IPI sent to CPU5
    
    	synchronize_sched_expedited_wait
    		 ret = swait_event_timeout(rsp->expedited_wq,
    					   sync_rcu_preempt_exp_done(rnp_root),
    					   jiffies_stall);
    
    	expmask = 0x20, CPU 5 in idle path (in cpuidle_enter())
    
    o	CPU5 handles IPI and fails to acquire rq lock.
    
    	Handles IPI
    	     sync_sched_exp_handler
    		 resched_cpu
    		     returns while failing to try lock acquire rq->lock
    		 need_resched is not set
    
    o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
         idle (schedule() is not called).
    
    o    CPU 1 reports RCU stall.
    
    Given that resched_cpu() is now used only by RCU, this commit fixes the
    assumption by making resched_cpu() unconditional.
    
    Reported-by: Neeraj Upadhyay <neeraju@codeaurora.org>
    Suggested-by: Neeraj Upadhyay <neeraju@codeaurora.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cab8c5ec128e..b2281971894c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -505,8 +505,7 @@ void resched_cpu(int cpu)
 	struct rq *rq = cpu_rq(cpu);
 	unsigned long flags;
 
-	if (!raw_spin_trylock_irqsave(&rq->lock, flags))
-		return;
+	raw_spin_lock_irqsave(&rq->lock, flags);
 	resched_curr(rq);
 	raw_spin_unlock_irqrestore(&rq->lock, flags);
 }

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

#1736838

FromPeter Zijlstra <peterz@infradead.org>
Date2017-09-21 18:40 +0200
Message-ID<uscG5-3pb-5@gated-at.bofh.it>
In reply to#1736820
On Thu, Sep 21, 2017 at 09:00:48AM -0700, Paul E. McKenney wrote:
> commit c21c9b78182e35eb0e72ef4e3bba3054f26eaaea
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Mon Sep 18 08:54:40 2017 -0700
> 
>     sched: Make resched_cpu() unconditional
>     
>     The current implementation of synchronize_sched_expedited() incorrectly
>     assumes that resched_cpu() is unconditional, which it is not.  This means
>     that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
>     fails as follows (analysis by Neeraj Upadhyay):
>     
>     o	CPU1 is waiting for expedited wait to complete:
>     
>     	sync_rcu_exp_select_cpus
>     	     rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
>     	     IPI sent to CPU5
>     
>     	synchronize_sched_expedited_wait
>     		 ret = swait_event_timeout(rsp->expedited_wq,
>     					   sync_rcu_preempt_exp_done(rnp_root),
>     					   jiffies_stall);
>     
>     	expmask = 0x20, CPU 5 in idle path (in cpuidle_enter())
>     
>     o	CPU5 handles IPI and fails to acquire rq lock.
>     
>     	Handles IPI
>     	     sync_sched_exp_handler
>     		 resched_cpu
>     		     returns while failing to try lock acquire rq->lock
>     		 need_resched is not set
>     
>     o    CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
>          idle (schedule() is not called).
>     
>     o    CPU 1 reports RCU stall.


Inconsistent spacing after your bullet 'o', first two points have a
space the last two a tab or so.

>     Given that resched_cpu() is now used only by RCU, this commit fixes the
>     assumption by making resched_cpu() unconditional.

Other than that, yes looks _much_ better, thanks!

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Also, you might want to tag it for stable.

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

#1736841

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-09-21 18:50 +0200
Message-ID<uscPL-3sF-3@gated-at.bofh.it>
In reply to#1736838
On Thu, Sep 21, 2017 at 06:30:12PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 21, 2017 at 09:00:48AM -0700, Paul E. McKenney wrote:
> > commit c21c9b78182e35eb0e72ef4e3bba3054f26eaaea

[ . . . ]

> Inconsistent spacing after your bullet 'o', first two points have a
> space the last two a tab or so.
> 
> >     Given that resched_cpu() is now used only by RCU, this commit fixes the
> >     assumption by making resched_cpu() unconditional.
> 
> Other than that, yes looks _much_ better, thanks!
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Also, you might want to tag it for stable.

Like this, then?

							Thanx, Paul

------------------------------------------------------------------------

commit 62d94c97e96d5aa6a977a53dd007029df7a65586
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Sep 18 08:54:40 2017 -0700

    sched: Make resched_cpu() unconditional
    
    The current implementation of synchronize_sched_expedited() incorrectly
    assumes that resched_cpu() is unconditional, which it is not.  This means
    that synchronize_sched_expedited() can hang when resched_cpu()'s trylock
    fails as follows (analysis by Neeraj Upadhyay):
    
    o	CPU1 is waiting for expedited wait to complete:
    
    	sync_rcu_exp_select_cpus
    	     rdp->exp_dynticks_snap & 0x1   // returns 1 for CPU5
    	     IPI sent to CPU5
    
    	synchronize_sched_expedited_wait
    		 ret = swait_event_timeout(rsp->expedited_wq,
    					   sync_rcu_preempt_exp_done(rnp_root),
    					   jiffies_stall);
    
    	expmask = 0x20, CPU 5 in idle path (in cpuidle_enter())
    
    o	CPU5 handles IPI and fails to acquire rq lock.
    
    	Handles IPI
    	     sync_sched_exp_handler
    		 resched_cpu
    		     returns while failing to try lock acquire rq->lock
    		 need_resched is not set
    
    o	CPU5 calls  rcu_idle_enter() and as need_resched is not set, goes to
    	idle (schedule() is not called).
    
    o	CPU 1 reports RCU stall.
    
    Given that resched_cpu() is now used only by RCU, this commit fixes the
    assumption by making resched_cpu() unconditional.
    
    Reported-by: Neeraj Upadhyay <neeraju@codeaurora.org>
    Suggested-by: Neeraj Upadhyay <neeraju@codeaurora.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
    Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    Cc: stable@vger.kernel.org

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cab8c5ec128e..b2281971894c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -505,8 +505,7 @@ void resched_cpu(int cpu)
 	struct rq *rq = cpu_rq(cpu);
 	unsigned long flags;
 
-	if (!raw_spin_trylock_irqsave(&rq->lock, flags))
-		return;
+	raw_spin_lock_irqsave(&rq->lock, flags);
 	resched_curr(rq);
 	raw_spin_unlock_irqrestore(&rq->lock, flags);
 }

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

#1736681

FromPeter Zijlstra <peterz@infradead.org>
Date2017-09-21 16:00 +0200
Message-ID<usabh-1IE-41@gated-at.bofh.it>
In reply to#1734223
On Mon, Sep 18, 2017 at 11:11:05AM -0400, Steven Rostedt wrote:
> On Sun, 17 Sep 2017 11:37:06 +0530
> Neeraj Upadhyay <neeraju@codeaurora.org> wrote:
> 
> > Hi Paul, how about replacing raw_spin_trylock_irqsave with
> > raw_spin_lock_irqsave in resched_cpu()? Are there any paths
> > in RCU code, which depend on trylock check/spinlock recursion?
> 
> It looks to me that resched_cpu() was added for nohz full sched
> balancing, but is not longer used by that. The only user is currently
> RCU. Perhaps we should change that from a trylock to a lock.

No, regular NOHZ balancing. NOHZ FULL wasn't conceived back then.

  46cb4b7c88fa ("sched: dynticks idle load balancing")

And yeah, its no longer used for that.

And given RCU is the only user of that thing, I suppose we can indeed
change it to a full lock.

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

#1736791

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-09-21 17:40 +0200
Message-ID<usbK3-2QA-33@gated-at.bofh.it>
In reply to#1736681
On Thu, Sep 21, 2017 at 03:55:46PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 18, 2017 at 11:11:05AM -0400, Steven Rostedt wrote:
> > On Sun, 17 Sep 2017 11:37:06 +0530
> > Neeraj Upadhyay <neeraju@codeaurora.org> wrote:
> > 
> > > Hi Paul, how about replacing raw_spin_trylock_irqsave with
> > > raw_spin_lock_irqsave in resched_cpu()? Are there any paths
> > > in RCU code, which depend on trylock check/spinlock recursion?
> > 
> > It looks to me that resched_cpu() was added for nohz full sched
> > balancing, but is not longer used by that. The only user is currently
> > RCU. Perhaps we should change that from a trylock to a lock.
> 
> No, regular NOHZ balancing. NOHZ FULL wasn't conceived back then.
> 
>   46cb4b7c88fa ("sched: dynticks idle load balancing")
> 
> And yeah, its no longer used for that.
> 
> And given RCU is the only user of that thing, I suppose we can indeed
> change it to a full lock.

Thank you!  May I have your ack?

							Thanx, Paul

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

#1736829

FromPeter Zijlstra <peterz@infradead.org>
Date2017-09-21 18:20 +0200
Message-ID<uscmK-3iX-35@gated-at.bofh.it>
In reply to#1736791
On Thu, Sep 21, 2017 at 08:31:34AM -0700, Paul E. McKenney wrote:
> > And given RCU is the only user of that thing, I suppose we can indeed
> > change it to a full lock.
> 
> Thank you!  May I have your ack?

Last version I saw still had a Changelog that looked like whitespace
damage.

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

#1736794

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-09-21 17:50 +0200
Message-ID<usbTH-2TP-3@gated-at.bofh.it>
In reply to#1736681
On Thu, 21 Sep 2017 15:55:46 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Sep 18, 2017 at 11:11:05AM -0400, Steven Rostedt wrote:
> > On Sun, 17 Sep 2017 11:37:06 +0530
> > Neeraj Upadhyay <neeraju@codeaurora.org> wrote:
> >   
> > > Hi Paul, how about replacing raw_spin_trylock_irqsave with
> > > raw_spin_lock_irqsave in resched_cpu()? Are there any paths
> > > in RCU code, which depend on trylock check/spinlock recursion?  
> > 
> > It looks to me that resched_cpu() was added for nohz full sched
> > balancing, but is not longer used by that. The only user is currently
> > RCU. Perhaps we should change that from a trylock to a lock.  
> 
> No, regular NOHZ balancing. NOHZ FULL wasn't conceived back then.
> 
>   46cb4b7c88fa ("sched: dynticks idle load balancing")

Bah, every time I type NOHZ today I add FULL after it. I did mean NOHZ
without FULL. :-p

-- Steve

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web