On Wed, May 16, 2018 at 08:45:43AM -0700, Paul E. McKenney wrote:
[...]
> > > > The code would be correct then, but one issue is it would shout out the
> > > > 'Prestarted' tracepoint for 'c' when that's not really true..
> > > >
> > > > rcu_seq_done(&rnp_root->gp_seq, c)
> > > >
> > > > translates to ULONG_CMP_GE(&rnp_root->gp_seq, c)
> > > >
> > > > which translates to the fact that c-1 completed.
> > > >
> > > > So in this case if rcu_seq_done returns true, then saying that c has
> > > > been
> > > > 'Prestarted' seems a bit off to me. It should be 'Startedleaf' or
> > > > something
> > > > since what we really are doing is just marking the leaf as you
> > > > mentioned in
> > > > the unlock_out part for a future start.
> > >
> > > Indeed, some of the tracing is not all that accurate. But the trace
> > > message itself contains the information needed to work out why the
> > > loop was exited, so perhaps something like 'EarlyExit'?
> >
> > I think since you're now using rcu_seq_start to determine if c has started
> > or
> > completed since, the current 'Prestarted' trace will cover it.
>
> "My work is done!" ;-)
:-D Its cool how a conversation can turn into a code improvement.
> > > ------------------------------------------------------------------------
> > >
> > > commit 59a4f38edcffbef1521852fe3b26ed4ed85af16e
> > > Author: Paul E. McKenney <paul...@linux.vnet.ibm.com>
> > > Date: Tue May 15 11:53:41 2018 -0700
> > >
> > > rcu: Make rcu_start_this_gp() check for grace period already started
> > >
> > > In the old days of ->gpnum and ->completed, the code requesting a new
> > > grace period checked to see if that grace period had already started,
> > > bailing early if so. The new-age ->gp_seq approach instead checks
> > > whether the grace period has already finished. A compensating change
> > > pushed the requested grace period down to the bottom of the tree, thus
> > > reducing lock contention and even eliminating it in some cases. But
> > > why
> > > not further reduce contention, especially on large systems, by doing
> > > both,
> > > especially given that the cost of doing both is extremely small?
> > >
> > > This commit therefore adds a new rcu_seq_started() function that
> > > checks
> > > whether a specified grace period has already started. It then uses
> > > this new function in place of rcu_seq_done() in the
> > > rcu_start_this_gp()
> > > function's funnel locking code.
> > >
> > > Reported-by: Joel Fernandes <j...@joelfernandes.org>
> > > Signed-off-by: Paul E. McKenney <paul...@linux.vnet.ibm.com>
> > >
> > > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > > index 003671825d62..1c5cbd9d7c97 100644
> > > --- a/kernel/rcu/rcu.h
> > > +++ b/kernel/rcu/rcu.h
> > > @@ -108,6 +108,15 @@ static inline unsigned long rcu_seq_current(unsigned
> > > long *sp)
> > > }
> > >
> > > /*
> > > + * Given a snapshot from rcu_seq_snap(), determine whether or not the
> > > + * corresponding update-side operation has started.
> > > + */
> > > +static inline bool rcu_seq_started(unsigned long *sp, unsigned long s)
> > > +{
> > > + return ULONG_CMP_LT((s - 1) & ~RCU_SEQ_STATE_MASK, READ_ONCE(*sp));
> > > +}
> > > +
> > > +/*
> > > * Given a snapshot from rcu_seq_snap(), determine whether or not a
> > > * full update-side operation has occurred.
> > > */
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 9e900c5926cc..ed69f49b7054 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -1580,7 +1580,7 @@ static bool rcu_start_this_gp(struct rcu_node *rnp,
> > > struct rcu_data *rdp,
> > > if (rnp_root != rnp)
> > > raw_spin_lock_rcu_node(rnp_root);
> > > if (ULONG_CMP_GE(rnp_root->gp_seq_needed, c) ||
> > > - rcu_seq_done(&rnp_root->gp_seq, c) ||
> > > + rcu_seq_started(&rnp_root->gp_seq, c) ||
> >
> > Yes, this does exactly what I was wanting, thanks! I think this puts our
> > discussion about this to rest :-)
> >
> > By the way I was starting to beautify this loop like below last week, with
> > code comments. I felt it would be easier to parse this loop in the future
> > for whoever was reading it. Are you interested in such a patch? If not, let
> > me know and I'll drop this and focus on the other changes you requested.
> >
> > Something like... (just an example , actual code would be different)
> >
> > for (rnp_node = rnp; 1; rnp_node = rnp_node->parent) {
> > int prestarted = 0;
> >
> > /* Acquire lock if non-leaf node */
> > if (rnp_node != rnp)
> > raw_spin_lock_rcu_node(rnp_node);
> >
> > /* Has the GP asked been recorded as a future need */
> > if (ULONG_CMP_GE(rnp_node->gp_seq_needed, gp_seq_start))
> > prestarted = 1;
> >
> > /* Has the GP requested for already been completed */
> > if (!prestarted && rcu_seq_completed(&rnp_node->gp_seq,
> > gp_seq_start))
> > prestarted = 1;
> >
> > ... etc...
> > if (prestarted) {
> > trace_rcu_this_gp(rnp_node, rdp, gp_seq_start,
> > TPS("Prestarted"));
> > goto unlock_out;
> > }
>
> At the moment, I don't believe that the extra lines of code pay for
> themselves, but I do agree that this loop is a bit more obtuse than I
> would like.
Yeah I was also thinking the same. I'm glad I checked, thanks for the
feedback. I'll focus on the other comments then.
thanks,
- Joel
