lkml.org 
[lkml]   [2018]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] rcu: exp: Fix "must hold exp_mutex" comments for QS reporting functions
On Mon, Mar 12, 2018 at 01:28:38PM +0800, Boqun Feng wrote:
> On Fri, Mar 09, 2018 at 12:17:07PM -0800, Paul E. McKenney wrote:
> > On Fri, Mar 09, 2018 at 02:57:00PM +0800, Boqun Feng wrote:
> > > On Thu, Mar 08, 2018 at 07:42:55AM -0800, Paul E. McKenney wrote:
> > > > On Thu, Mar 08, 2018 at 04:30:06PM +0800, Boqun Feng wrote:
> > > > > On Thu, Mar 08, 2018 at 12:54:29PM +0800, Boqun Feng wrote:
> > > > > > On Wed, Mar 07, 2018 at 08:30:17PM -0800, Paul E. McKenney wrote:
> > > > > > [...]
> > > > > > > >
> > > > > > > > +/*
> > > > > > > > + * Like sync_rcu_preempt_exp_done(), but this function assumes the caller
> > > > > > > > + * doesn't hold the rcu_node's ->lock, and will acquire and release the lock
> > > > > > > > + * itself
> > > > > > > > + */
> > > > > > > > +static bool sync_rcu_preempt_exp_done_unlocked(struct rcu_node *rnp)
> > > > > > > > +{
> > > > > > > > + unsigned long flags;
> > > > > > > > + bool ret;
> > > > > > > > +
> > > > > > > > + raw_spin_lock_irqsave_rcu_node(rnp, flags);
> > > > > > > > + ret = sync_rcu_preempt_exp_done(rnp);
> > > > > > >
> > > > > > > Let's see... The sync_rcu_preempt_exp_done() function checks the
> > > > > > > ->exp_tasks pointer and the ->expmask bitmask. The number of bits in the
> > > > > > > mask can only decrease, and the ->exp_tasks pointer can only transition
> > > > > > > from NULL to non-NULL when there is at least one bit set. However,
> > > > > > > there is no ordering in sync_rcu_preempt_exp_done(), so it is possible
> > > > > > > that it could be fooled without the lock:
> > > > > > >
> > > > > > > o CPU 0 in sync_rcu_preempt_exp_done() reads ->exp_tasks and
> > > > > > > sees that it is NULL.
> > > > > > >
> > > > > > > o CPU 1 blocks within an RCU read-side critical section, so
> > > > > > > it enqueues the task and points ->exp_tasks at it and
> > > > > > > clears CPU 1's bit in ->expmask.
> > > > > > >
> > > > > > > o All other CPUs clear their bits in ->expmask.
> > > > > > >
> > > > > > > o CPU 0 reads ->expmask, sees that it is zero, so incorrectly
> > > > > > > concludes that all quiescent states have completed, despite
> > > > > > > the fact that ->exp_tasks is non-NULL.
> > > > > > >
> > > > > > > So it seems to me that the lock is needed. Good catch!!! The problem
> > > > > > > would occur only if the task running on CPU 0 received a spurious
> > > > > > > wakeup, but that could potentially happen.
> > > > > >
> > > > > > Thanks for the analysis ;-)
> > > >
> > > > The other limitation is that it occurs only on systems small enough
> > > > to have a single-node rcu_node tree. But still...
> > > >
> > > > > > > If lock contention becomes a problem, memory-ordering tricks could be
> > > > > > > applied, but the lock is of course simpler.
> > > > > > >
> > > > > >
> > > > > > Agreed.
> > > > > >
> > > > > > > I am guessing that this is a prototype patch, and that you are planning
> > > > > >
> > > > > > Yes, this is a prototype. And I'm preparing a proper patch to send
> > > > > > later.
> > > >
> > > > Very good, thank you!
> > > >
> > > > > > > to add lockdep annotations in more places, but either way please let
> > > > > > > me know.
> > > > > >
> > > > > > Give it's a bug as per your analysis, I'd like to defer other lockdep
> > > > > > annotations and send this first. However, I'm currently getting other
> > > > > > lockdep splats after applying this, so I need to get that sorted first.
> > > > >
> > > > > Hmm.. the other lockdep splat seems irrelevant with my patch, I could
> > > > > observe it on mainline using rcutorture with CONFIG_PROVE_LOCKING=y. I'd
> > > > > spend some more time on it, in the meanwhile, send a proper patch for
> > > > > this sync_rcu_preempt_exp_done().
> > > >
> > > > I am not seeing that one, but am very interested in getting it fixed! ;-)
> > >
> > > Found the root cause, and send out the patch ;-)
> >
> > Very good! Still not sure why I don't see it, but as long as it is fixed!
> >
>
> One thing I could hit this is because I ran rcutorture with
> CONFIG_PROVE_LOCKING=y, maybe you could consider adding that in your
> rcutorture testsuite?

I also need to test without lockdep, but you are right that I should
certainly do a full-up lockdep once in a while!

Thanx, Paul

\
 
 \ /
  Last update: 2018-03-13 01:15    [W:0.063 / U:1.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site