Date:   Thu, 28 Jan 2021 16:19:21 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Frederic Weisbecker <frederic@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        Josh Triplett <josh@...htriplett.org>,
        Stable <stable@...r.kernel.org>,
        Joel Fernandes <joel@...lfernandes.org>
Subject: Re: [PATCH 05/16] rcu/nocb: Disable bypass when CPU isn't completely
 offloaded

On Thu, Jan 28, 2021 at 11:25:31PM +0100, Frederic Weisbecker wrote:
> On Thu, Jan 28, 2021 at 01:31:33PM -0800, Paul E. McKenney wrote:
> > On Thu, Jan 28, 2021 at 06:12:11PM +0100, Frederic Weisbecker wrote:
> > > ---
> > >  include/linux/rcu_segcblist.h |  7 ++++---
> > >  kernel/rcu/tree_plugin.h      | 31 +++++++++++++++++++++++--------
> > >  2 files changed, 27 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
> > > index 8afe886e85f1..5a2d6dadd720 100644
> > > --- a/include/linux/rcu_segcblist.h
> > > +++ b/include/linux/rcu_segcblist.h
> > > @@ -109,7 +109,7 @@ struct rcu_cblist {
> > >   *  |                           SEGCBLIST_KTHREAD_GP                           |
> > >   *  |                                                                          |
> > >   *  |   Kthreads handle callbacks holding nocb_lock, local rcu_core() stops    |
> > > - *  |   handling callbacks.                                                    |
> > > + *  |   handling callbacks. Allow bypass enqueue.                              |
> > 
> > "Allow bypass enqueue" as in bypass was disabled and entering this
> > state causes it to be enabled, correct?
> 
> Right.
> 
> > If so, "Enable bypass
> > queueing" would be less ambiguous and would match the change below.
> 
> Ok I'll fix that.
> 
> > > @@ -2412,6 +2423,7 @@ static long rcu_nocb_rdp_deoffload(void *arg)
> > >  
> > >  	rcu_nocb_lock_irqsave(rdp, flags);
> > >  
> > > +	WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies));
> > 
> > This flush suffices because we are running on the target CPU
> > holding ->nocb_lock (thus having interrupts disabled), and
> > because rdp_offload_toggle() invokes rcu_segcblist_offload(),
> > which clears SEGCBLIST_OFFLOADED.  Thus future calls to
> > rcu_segcblist_completely_offloaded() will return false,
> > which means that future calls to rcu_nocb_try_bypass() will
> > refuse to put anything into the bypass.
> 
> Exactly!

Whew!  ;-)

> > I believe that this deserves a comment, particularly if I am
> > confused about what is really happening here.  ;-)
> 
> Yes indeed I've been greedy there, will comment this :o)

Very good!

> > On another topic, since I saw it along the way...
> > 
> > The header comment for rcu_segcblist_offload() says that the
> > structure must be empty, but that isn't really the case, is it?
> 
> Ah strange indeed, must be a leftover. I'll remove it.

I should have spotted it earlier, shouldn't I have?  ;-)

> > >  	ret = rdp_offload_toggle(rdp, false, flags);
> > >  	swait_event_exclusive(rdp->nocb_state_wq,
> > >  			      !rcu_segcblist_test_flags(cblist, SEGCBLIST_KTHREAD_CB |
> > > @@ -2422,19 +2434,22 @@ static long rcu_nocb_rdp_deoffload(void *arg)
> > >  	rcu_nocb_unlock_irqrestore(rdp, flags);
> > >  	del_timer_sync(&rdp->nocb_timer);
> > >  
> > > +	/* Sanity check */
> > > +	WARN_ON_ONCE(rcu_cblist_n_cbs(&rdp->nocb_bypass));
> > > +
> > >  	/*
> > > -	 * Flush bypass. While IRQs are disabled and once we set
> > > -	 * SEGCBLIST_SOFTIRQ_ONLY, no callback is supposed to be
> > > -	 * enqueued on bypass.
> > > +	 * Lock one last time so we see latest updates from kthreads and timer
> > 
> > You lost me here.  What updates are we seeing from kthreads and timers?
> 
> We want to make sure that, whatever change has been made on the segcblist by
> kthreads such as length or callbacks dequeue, this is visible on the current
> CPU. The swait_event_exclusive() doesn't guarantee that everything from the
> kthreads is visible here as the flags are checked lockless and I haven't added
> specific barriers.
> 
> That said right after the swait_event there is a nocb_lock LOCK/UNLOCK cycle to
> disable the timer, so that's enough for the local CPU to see those updates. What
> remains is the updates made by pending timers flushed in del_timer_sync(). There
> is nothing special there to be visible here but out of paranoia...
> 
> In fact this matters later in the series as the above timer disablement and
> flush will disappear and the LOCK/UNLOCK cycle that comes along.

OK, so the point is that any future manipulations of this callback
list will see the desired stable state, correct?

> > > +	 * so that we can later run callbacks locally without the lock.
> > >  	 */
> > >  	rcu_nocb_lock_irqsave(rdp, flags);
> > > -	rcu_nocb_flush_bypass(rdp, NULL, jiffies);
> > > +	/*
> > > +	 * Theoretically we could set SEGCBLIST_SOFTIRQ_ONLY after the nocb
> > > +	 * LOCK/UNLOCK but let's be paranoid.
> > > +	 */
> > >  	rcu_segcblist_set_flags(cblist, SEGCBLIST_SOFTIRQ_ONLY);
> > 
> > As long as we are being paranoid, should we also check that the
> > bypass remains empty?
> 
> You missed it, check above for sanity check :-)

I did see that, but...  Why not place it as late as possible, like
just before releasing the ->nocb_lock?  Or is there some way that
a callback can sneak into the bypass list after the sanity check but
before ->nocb_lock is acquired?

							Thanx, Paul

> Thanks.
> 
> > 
> > >  	/*
> > >  	 * With SEGCBLIST_SOFTIRQ_ONLY, we can't use
> > > -	 * rcu_nocb_unlock_irqrestore() anymore. Theoretically we
> > > -	 * could set SEGCBLIST_SOFTIRQ_ONLY with cb unlocked and IRQs
> > > -	 * disabled now, but let's be paranoid.
> > > +	 * rcu_nocb_unlock_irqrestore() anymore.
> > >  	 */
> > >  	raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
> > >  
> > > -- 
> > > 2.25.1
> > > 

Powered by blists - more mailing lists