Received-SPF: pass (google.com: domain of linux-kernel+bounces-30229-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 18 Jan 2024 06:51:57 -0800
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Frederic Weisbecker <frederic@kernel.org>
Cc: Zqiang <qiang.zhang1211@gmail.com>, quic_neeraju@quicinc.com,
	joel@joelfernandes.org, rcu@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rcu/nocb: Check rdp_gp->nocb_timer in
 __call_rcu_nocb_wake()
Message-ID: <3b63cf39-3805-4c1d-b79b-fdd5aeb17db3@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20240117102616.18302-1-qiang.zhang1211@gmail.com>
 <ZafC_YkTJKsOropE@localhost.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZafC_YkTJKsOropE@localhost.localdomain>

On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote:
> Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a ?crit :
> > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of
> > no-rdp_gp structure, the timer_pending() is always return false,
> > this commit therefore need to check rdp_gp->nocb_timer in
> > __call_rcu_nocb_wake().
> > 
> > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > ---
> >  kernel/rcu/tree_nocb.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> > index 54971afc3a9b..3f85577bddd4 100644
> > --- a/kernel/rcu/tree_nocb.h
> > +++ b/kernel/rcu/tree_nocb.h
> > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
> >  	long lazy_len;
> >  	long len;
> >  	struct task_struct *t;
> > +	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
> >  
> >  	// If we are being polled or there is no kthread, just leave.
> >  	t = READ_ONCE(rdp->nocb_gp_kthread);
> > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
> >  		smp_mb(); /* Enqueue before timer_pending(). */
> >  		if ((rdp->nocb_cb_sleep ||
> >  		     !rcu_segcblist_ready_cbs(&rdp->cblist)) &&
> > -		    !timer_pending(&rdp->nocb_timer)) {
> > +		    !timer_pending(&rdp_gp->nocb_timer)) {
> 
> Hehe, good eyes ;-)
> 
> I had that change in mind but while checking that area further I actually
> wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If
> we reach that place, it means that the nocb_gp kthread should be awaken
> already (or the timer pending), so what does a force wake up solve in that
> case?
> 
> Paul, any recollection of that?

Huh.  We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed
all the code paths correctly.

Historically, I have been worried about lost wakeups.  Also, there
used to be code paths in which a wakeup was not needed, for example,
because we knew that the ending of the current grace period would take
care of things.  Unless there was some huge pile of callbacks, in which
case an immediate wakeup could avoid falling behind a callback flood.

Given that rcutorture does test callback flooding, we appear to be OK,
but maybe it is time to crank up the flooding more.

On the other hand, I have started seeing the (very) occasional OOM
on TREE03.  (In addition to those that show up from time to time on the
single-CPU TREE09 scenario.)

							Thanx, Paul

> In the meantime:
> 
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>