Re: Query regarding srcu_funnel_exp_start()

[Neeraj Upadhyay]

On 10/28/2017 03:50 AM, Paul E. McKenney wrote:
> On Fri, Oct 27, 2017 at 10:15:04PM +0530, Neeraj Upadhyay wrote:
> > On 10/27/2017 05:56 PM, Paul E. McKenney wrote:
> > > On Fri, Oct 27, 2017 at 02:23:07PM +0530, Neeraj Upadhyay wrote:
> > > > Hi,
> > > >
> > > > One query regarding srcu_funnel_exp_start() function in
> > > > kernel/rcu/srcutree.c.
> > > >
> > > > static void srcu_funnel_exp_start(struct srcu_struct *sp, struct
> > > > srcu_node *snp,
> > > > 				  unsigned long s)
> > > > {
> > > > 	<snip>
> > > > 	if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s))
> > > > 		sp->srcu_gp_seq_needed_exp = s;
> > > > 	<snip>
> > > > }
> > > >
> > > > Why is sp->srcu_gp_seq_needed_exp set to 's' if srcu_gp_seq_needed_exp is >=
> > > > 's'. Shouldn't srcu_gp_seq_needed_exp be equal to the greater of both?
> > >
> > > Let's suppose that it is incorrect as currently written.  Can you
> > > construct a test case demonstrating a failure of some sort, then provide
> > > a fix?
> >
> > Will check this. Might take some time to build a test case.
>
> Fair enough!
>
> I suggest checking to see if kernel/rcu/rcuperf.c can do what you need for
> this test.  (Might not with a single test, but perhaps a before-and-after
> comparison.  Or maybe you really do need to add some test code somewhere.)

Thanks for the suggestion, will try that out.

> > > To start with, if it is currently incorrect, what would be the nature
> > > of the failure?
> > >
> > > 							Thanx, Paul
> >
> > Hi Paul,
> >
> > I see below scenario, where new gp won't be expedited. Please correct
> > me if I am missing something here.
> >
> > 1. CPU0 calls synchronize_srcu_expedited()
> >
> > synchronize_srcu_expedited()
> >    __synchronize_srcu()
> >      __call_srcu()
> >              s = rcu_seq_snap(&sp->srcu_gp_seq); // lets say
> > srcu_gp_seq  = 0;
> >                                                  // s = 0x100
>
> Looks like you have one hex digit and then two binary digits, but why not?
> (RCU_SEQ_STATE_MASK is 3 rather than 0xff >

Yeah, sorry I confused myself while representing the values. 0x100 need 
to be replaced with b'100' and 0x200 with b'1000'.

> >              sdp->srcu_gp_seq_needed = s // 0x100
> >              needgp = true
> >              sdp->srcu_gp_seq_needed_exp = s // 0x100
> >          srcu_funnel_gp_start()
> >                  sp->srcu_gp_seq_needed_exp = s;
> >              srcu_gp_start(sp);
> >                  rcu_seq_start(&sp->srcu_gp_seq);
> >
> > 2. CPU1 calls normal synchronize_srcu()
> >
> > synchronize_srcu()
> >      __synchronize_srcu(sp, true)
> >          __call_srcu()
> >                  s = rcu_seq_snap(&sp->srcu_gp_seq); // srcu_gp_seq = 1
> >                                                      // s= 0x200
> >                  sdp->srcu_gp_seq_needed = s; // 0x200
> >              srcu_funnel_gp_start()
> >                  smp_store_release(&sp->srcu_gp_seq_needed, s); // 0x200
> >
> > 3. CPU3 calls synchronize_srcu_expedited()
> >
> > synchronize_srcu_expedited()
> >    __synchronize_srcu()
> >      __call_srcu()
> >              s = rcu_seq_snap(&sp->srcu_gp_seq); // srcu_gp_seq = 1
> >                                                  // s = 0x200
> >              sdp->srcu_gp_seq_needed_exp = s // 0x200
> >          srcu_funnel_exp_start(sp, sdp->mynode, s);
> >              // sp->srcu_gp_seq_needed_exp = 0x100
> >              // s = 0x200 ; sp->srcu_gp_seq_needed_exp is not updated
> >              if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s))
> >                  sp->srcu_gp_seq_needed_exp = s;
>
> Seems plausible, but you should be able to show the difference in
> grace-period duration with a test.

Ok sure, will attempt that.

> While you are in srcu_funnel_exp_start(), should it be rechecking
> rcu_seq_done(&sp->srcu_gp_seq, s) as well as the current
> ULONG_CMP_GE(snp->srcu_gp_seq_needed_exp, s) under the lock?
> Why or why not?
>
> 							Thanx, Paul

Hi Paul,

I don't see how it will impact. I have put markers in code snippet
below to explain my points. My understanding is

* rcu_seq_done check @a is a fastpath return, and avoid contention
for snp lock, if the gp has already elapsed.

* Checking it @b, inside srcu_node  lock might not make any
difference, as sp->srcu_gp_seq counter portion is updated
under srcu_struct lock. Also, we cannot lock srcu_struct at this
point, as it will cause lock contention among multiple CPUs.

* Checking rcu_seq_done @c also does not impact, as we have already
done all the work of traversing the entire parent chain and if
rcu_seq_done() is true srcu_gp_seq_needed_exp will be greater
than or equal to 's'.

  srcu_gp_end()
    raw_spin_lock_irq_rcu_node(sp);
    rcu_seq_end(&sp->srcu_gp_seq);
    gpseq = rcu_seq_current(&sp->srcu_gp_seq);
    if (ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, gpseq))
        sp->srcu_gp_seq_needed_exp = gpseq;
    raw_spin_unlock_irq_rcu_node(sp);

static void srcu_funnel_exp_start(...)
{
    <snip>

    for (; snp != NULL; snp = snp->srcu_parent) {
        if (rcu_seq_done(&sp->srcu_gp_seq, s) ||  /* a */
            ULONG_CMP_GE(READ_ONCE(snp->srcu_gp_seq_needed_exp), s))
            return;
        raw_spin_lock_irqsave_rcu_node(snp, flags);
        /* b */
        if (ULONG_CMP_GE(snp->srcu_gp_seq_needed_exp, s)) {
            raw_spin_unlock_irqrestore_rcu_node(snp, flags);
            return;
        }
        <snip>
        raw_spin_unlock_irqrestore_rcu_node(snp, flags);
    }
    raw_spin_lock_irqsave_rcu_node(sp, flags);
    /* c */
    if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s))
        sp->srcu_gp_seq_needed_exp = s;
    raw_spin_unlock_irqrestore_rcu_node(sp, flags);
}

Thanks
Neeraj

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation