Date:   Mon, 23 Jan 2023 17:27:10 +0100
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     "Zhang, Qiang1" <qiang1.zhang@intel.com>,
        "Paul E. McKenney" <paulmck@kernel.org>, quic_neeraju@quicinc.com,
        rcu@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] rcu: Remove impossible wakeup rcu GP kthread action
 from rcu_report_qs_rdp()
Message-ID: <Y861Xuoa+CrZt9PB@lothringen>
References: <PH0PR11MB5880A16045A842AB80A25C4BDAC59@PH0PR11MB5880.namprd11.prod.outlook.com>
 <B3E458A6-9279-4716-B242-873C77EC1E3A@joelfernandes.org>
 <Y8seE8Tr1w0UWf+m@lothringen>
 <CAEXW_YTTk_zETWy=xpL-8wF8vh6s_U5z9UyS17uBMda1BYpvug@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEXW_YTTk_zETWy=xpL-8wF8vh6s_U5z9UyS17uBMda1BYpvug@mail.gmail.com>
Precedence: bulk

On Mon, Jan 23, 2023 at 10:22:19AM -0500, Joel Fernandes wrote:
> > 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 :)

But that's only when there is no grace period pending, so it can't happen while
we report a QS.

OTOH without the needacc, those callbacks waiting to be accelerated would be
eventually processed but only on the next tick following the end of a grace
period...if none has started since then. So if someone else starts a new GP
before the current CPU, we must wait another GP, etc...

That's potentially dangerous.

And unfortunately we can't do the acceleration from __note_gp_changes() due
to lock ordering restrictions: nocb_lock -> rnp_lock

> 
> Thanks!
> 
>   -Joel