Hi Frederic,
On Fri, Jan 20, 2023 at 6:04 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
>
> On Fri, Jan 20, 2023 at 08:27:03AM -0500, Joel Fernandes wrote:
> >
> > Sure, I know what the code currently does, I am asking why and it feels wrong.
> >
> > I suggest you slightly change your approach to not assuming the code should be bonafide correct and then fixing it (which is ok once in a while), and asking higher level questions to why things are the way they are in the first place (that is just my suggestion and I am not in a place to provide advice, far from it, but I am just telling you my approach — I care more about the code than increasing my patch count :P).
> >
> > If you are in an intermediate state, part way to a !nocb state — you may have
> > missed a nocb-related accel and wake, correct? Why does that matter? Once we
> > transition to a !nocb state, we do not do a post-qs-report accel+wake anyway
> > as we clearly know from the discussion.
>
> I'm confused. We are doing that acceleration on qs report for !nocb CPU, right?
>
> > So why do we need to do it if we
> > missed it for the intermediate stage? So, I am not fully sure yet what that
> > needac is doing and why it is needed.
>
> To summarize:
>
> * If the CPU is NOCB, all the callbacks advance and acceleration is performed
> by the rcuo/rcuog kthreads.
>
> * If the CPU is not NOCB, all the callbacks acceleration is performed by the
> CPU, such as in the case of rcu_report_qs_rdp().
>
> * If the CPU is transitionning from NOCB to !NOCB or from !NOCB to NOCB, the
> kthreads may not be available to do the advance/acceleration, so we must do
> it locally. That's the needacc path.
Sure, I agree it "must be done locally" for the benefit of the
half-way transition.
> What am I missing?
That the acceleration is also done by __note_gp_changes() once the
grace period ends anyway, so if any acceleration was missed as you
say, it will be done anyway.
Also it is done by scheduler tick raising softirq:
rcu_pending() does this:
/* Has RCU gone idle with this CPU needing another grace period? */
if (!gp_in_progress && rcu_segcblist_is_enabled(&rdp->cblist) &&
!rcu_rdp_is_offloaded(rdp) &&
!rcu_segcblist_restempty(&rdp->cblist, RCU_NEXT_READY_TAIL))
return 1;
and rcu_core():
/* No grace period and unregistered callbacks? */
if (!rcu_gp_in_progress() &&
rcu_segcblist_is_enabled(&rdp->cblist) && do_batch) {
rcu_nocb_lock_irqsave(rdp, flags);
if (!rcu_segcblist_restempty(&rdp->cblist, RCU_NEXT_READY_TAIL))
rcu_accelerate_cbs_unlocked(rnp, rdp);
rcu_nocb_unlock_irqrestore(rdp, flags);
}
So, I am not sure if you need needacc at all. Those CBs that have not
been assigned grace period numbers will be taken care off :)
Thanks!
-Joel
