Messages in this thread | | | Date | Sun, 29 Oct 2017 12:24:15 -0700 | From | "Paul E. McKenney" <> | Subject | Re: Query regarding srcu_funnel_exp_start() |
| |
On Sat, Oct 28, 2017 at 09:19:52AM +0530, Neeraj Upadhyay wrote: > 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'.
Sounds like something I would do! ;-)
> >> 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); > }
That does match my understanding, thank you for taking the time to go through it! Especially given that my understanding has proven to be wrong from time to time. ;-)
Thanx, Paul
| |