Re: [PATCH v3] rcu: Remove impossible wakeup rcu GP kthread action from rcu_report_qs_rdp()

[Joel Fernandes]

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