Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Oct 2022 08:46:06 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     Frederic Weisbecker <frederic@kernel.org>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org, rushikesh.s.kadam@intel.com,
        urezki@gmail.com, neeraj.iitr10@gmail.com, rostedt@goodmis.org,
        youssefesmat@google.com, surenb@google.com
Subject: Re: [PATCH v8 01/13] rcu: Fix missing nocb gp wake on rcu_barrier()
Message-ID: <20221014154606.GJ4221@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20221011180142.2742289-1-joel@joelfernandes.org>
 <20221011180142.2742289-2-joel@joelfernandes.org>
 <20221014142127.GE4221@paulmck-ThinkPad-P17-Gen-1>
 <20221014144019.GB1108603@lothringen>
 <20221014150344.GG4221@paulmck-ThinkPad-P17-Gen-1>
 <CAEXW_YQoRUJ=0_GJG6JunR58yASmehPanp14zbR3Y+KAGL1TDA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEXW_YQoRUJ=0_GJG6JunR58yASmehPanp14zbR3Y+KAGL1TDA@mail.gmail.com>
Precedence: bulk

On Fri, Oct 14, 2022 at 11:19:28AM -0400, Joel Fernandes wrote:
> On Fri, Oct 14, 2022 at 11:03 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Fri, Oct 14, 2022 at 04:40:19PM +0200, Frederic Weisbecker wrote:
> > > On Fri, Oct 14, 2022 at 07:21:27AM -0700, Paul E. McKenney wrote:
> > > > On Tue, Oct 11, 2022 at 06:01:30PM +0000, Joel Fernandes (Google) wrote:
> > > > > From: Frederic Weisbecker <frederic@kernel.org>
> > > > >
> > > > > Upon entraining a callback to a NOCB CPU, no further wake up is
> > > > > issued on the corresponding nocb_gp kthread. As a result, the callback
> > > > > and all the subsequent ones on that CPU may be ignored, at least until
> > > > > an RCU_NOCB_WAKE_FORCE timer is ever armed or another NOCB CPU belonging
> > > > > to the same group enqueues a callback on an empty queue.
> > > > >
> > > > > Here is a possible bad scenario:
> > > > >
> > > > > 1) CPU 0 is NOCB unlike all other CPUs.
> > > > > 2) CPU 0 queues a callback
> > > >
> > > > Call it CB1.
> > > >
> > > > > 2) The grace period related to that callback elapses
> > > > > 3) The callback is moved to the done list (but is not invoked yet),
> > > > >    there are no more pending callbacks for CPU 0
> > > >
> > > > So CB1 is on ->cblist waiting to be invoked, correct?
> > > >
> > > > > 4) CPU 1 calls rcu_barrier() and sends an IPI to CPU 0
> > > > > 5) CPU 0 entrains the callback but doesn't wake up nocb_gp
> > > >
> > > > And CB1 must still be there because otherwise the IPI handler would not
> > > > have entrained the callback, correct?  If so, we have both CB1 and the
> > > > rcu_barrier() callback (call it CB2) in ->cblist, but on the done list.
> > > >
> > > > > 6) CPU 1 blocks forever, unless CPU 0 ever queues enough further
> > > > >    callbacks to arm an RCU_NOCB_WAKE_FORCE timer.
> > > >
> > > > Except that -something- must have already been prepared to wake up in
> > > > order to invoke CB1.  And that something would invoke CB2 along with CB1,
> > > > given that they are both on the done list.  If there is no such wakeup
> > > > already, then the hang could occur with just CB1, without the help of CB2.
> > >
> > > Heh good point. I was confused with CB1 on RCU_DONE_TAIL and the possibility
> > > for CB2 to be entrained on RCU_WAIT_TAIL. But that's indeed not supposed to
> > > happen. Ok so this patch indeed doesn't make sense outside lazy.
> >
> > Whew!!!  ;-)
> >
> > > > > This is also required to make sure lazy callbacks in future patches
> > > > > don't end up making rcu_barrier() wait for multiple seconds.
> > > >
> > > > But I do see that the wakeup is needed in the lazy case, and if I remember
> > > > correctly, the ten-second rcu_barrier() delay really did happen.  If I
> 
> Yes it did happen. Real world device testing confirmed it.

Very good, thank you!

> > > > understand correctly, for this to happen, all of the callbacks must be
> > > > in the bypass list, that is, ->cblist must be empty.
> > > >
> > > > So has the scenario steps 1-6 called out above actually happened in the
> > > > absence of lazy callbacks?
> > >
> > > Nope, so I guess we can have the pending check around rcu_nocb_flush_bypass()
> > > only...
> >
> > OK, sounds good.
> >
> > I have put this series on branch lazy.2022.10.14a and am testing it.
> 
> I agree with the discussion, though if all CBs are in the bypass list,
> the patch will also save 2 jiffies.
> 
> So just commit messages that need rework then? This one can be taken instead:
> https://lore.kernel.org/rcu/21ECDA9F-81B1-4D22-8B03-020FB5DADA4F@joelfernandes.org/T/#m14d21fbce23539a521693a4184b28ddc55d7d2c5

This one looks plausible to me.

							Thanx, Paul