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;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Joel Fernandes <joel@joelfernandes.org>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH 1/2] rcu: Fix missing nocb gp wake on rcu_barrier()
Date:   Tue, 11 Oct 2022 03:33:37 -0400
Message-Id: <73CCC477-83F0-45BD-B484-973F94C71C7F@joelfernandes.org>
References: <20221011072517.GB4221@paulmck-ThinkPad-P17-Gen-1>
Cc:     Frederic Weisbecker <frederic@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
In-Reply-To: <20221011072517.GB4221@paulmck-ThinkPad-P17-Gen-1>
To:     paulmck@kernel.org
Precedence: bulk


> On Oct 11, 2022, at 3:25 AM, Paul E. McKenney <paulmck@kernel.org> wrote:
>=20
> =EF=BB=BFOn Tue, Oct 11, 2022 at 02:01:23AM +0000, Joel Fernandes wrote:
>>> On Tue, Oct 11, 2022 at 12:39:55AM +0200, Frederic Weisbecker wrote:
>>> 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.
>>>=20
>>> Here is a possible bad scenario:
>>>=20
>>> 1) CPU 0 is NOCB unlike all other CPUs.
>>> 2) CPU 0 queues a callback
>>> 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
>>> 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
>>> 6) CPU 1 blocks forever, unless CPU 0 ever queues enough further
>>>   callbacks to arm an RCU_NOCB_WAKE_FORCE timer.
>>>=20
>>> Make sure the necessary wake up is produced whenever necessary.
>>>=20
>>> Reported-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>> Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
>>> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
>>=20
>> Acked-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>=20
>> And if Paul is taking this, I'll rebase and drop this patch from the lazy=

>> series.
>=20
> Joel, could you please incorporate this into your series?  My internet
> access is likely to be a bit iffy over the next few days.  Likely no
> problem for email and the occasional test-systme access, but best not
> to take it for granted.  ;-)

Sure, I=E2=80=99ll do that. Thanks.

Fingers crossed on the internet ;-)

Thanks,

 - Joel


>=20
>                            Thanx, Paul
>=20
>> thanks,
>>=20
>> - Joel
>>=20
>>=20
>>> ---
>>> kernel/rcu/tree.c      | 6 ++++++
>>> kernel/rcu/tree.h      | 1 +
>>> kernel/rcu/tree_nocb.h | 5 +++++
>>> 3 files changed, 12 insertions(+)
>>>=20
>>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>>> index 96d678c9cfb6..025f59f6f97f 100644
>>> --- a/kernel/rcu/tree.c
>>> +++ b/kernel/rcu/tree.c
>>> @@ -3914,6 +3914,8 @@ static void rcu_barrier_entrain(struct rcu_data *r=
dp)
>>> {
>>>    unsigned long gseq =3D READ_ONCE(rcu_state.barrier_sequence);
>>>    unsigned long lseq =3D READ_ONCE(rdp->barrier_seq_snap);
>>> +    bool wake_nocb =3D false;
>>> +    bool was_alldone =3D false;
>>>=20
>>>    lockdep_assert_held(&rcu_state.barrier_lock);
>>>    if (rcu_seq_state(lseq) || !rcu_seq_state(gseq) || rcu_seq_ctr(lseq) !=
=3D rcu_seq_ctr(gseq))
>>> @@ -3922,6 +3924,7 @@ static void rcu_barrier_entrain(struct rcu_data *r=
dp)
>>>    rdp->barrier_head.func =3D rcu_barrier_callback;
>>>    debug_rcu_head_queue(&rdp->barrier_head);
>>>    rcu_nocb_lock(rdp);
>>> +    was_alldone =3D rcu_rdp_is_offloaded(rdp) && !rcu_segcblist_pend_cb=
s(&rdp->cblist);
>>>    WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies));
>>>    if (rcu_segcblist_entrain(&rdp->cblist, &rdp->barrier_head)) {
>>>        atomic_inc(&rcu_state.barrier_cpu_count);
>>> @@ -3929,7 +3932,10 @@ static void rcu_barrier_entrain(struct rcu_data *=
rdp)
>>>        debug_rcu_head_unqueue(&rdp->barrier_head);
>>>        rcu_barrier_trace(TPS("IRQNQ"), -1, rcu_state.barrier_sequence);
>>>    }
>>> +    wake_nocb =3D was_alldone && rcu_segcblist_pend_cbs(&rdp->cblist);
>>>    rcu_nocb_unlock(rdp);
>>> +    if (wake_nocb)
>>> +        wake_nocb_gp(rdp, false);
>>>    smp_store_release(&rdp->barrier_seq_snap, gseq);
>>> }
>>>=20
>>> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
>>> index d4a97e40ea9c..925dd98f8b23 100644
>>> --- a/kernel/rcu/tree.h
>>> +++ b/kernel/rcu/tree.h
>>> @@ -439,6 +439,7 @@ static void zero_cpu_stall_ticks(struct rcu_data *rd=
p);
>>> static struct swait_queue_head *rcu_nocb_gp_get(struct rcu_node *rnp);
>>> static void rcu_nocb_gp_cleanup(struct swait_queue_head *sq);
>>> static void rcu_init_one_nocb(struct rcu_node *rnp);
>>> +static bool wake_nocb_gp(struct rcu_data *rdp, bool force);
>>> static bool rcu_nocb_flush_bypass(struct rcu_data *rdp, struct rcu_head *=
rhp,
>>>                  unsigned long j);
>>> static bool rcu_nocb_try_bypass(struct rcu_data *rdp, struct rcu_head *r=
hp,
>>> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
>>> index f77a6d7e1356..094fd454b6c3 100644
>>> --- a/kernel/rcu/tree_nocb.h
>>> +++ b/kernel/rcu/tree_nocb.h
>>> @@ -1558,6 +1558,11 @@ static void rcu_init_one_nocb(struct rcu_node *rn=
p)
>>> {
>>> }
>>>=20
>>> +static bool wake_nocb_gp(struct rcu_data *rdp, bool force)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> static bool rcu_nocb_flush_bypass(struct rcu_data *rdp, struct rcu_head *=
rhp,
>>>                  unsigned long j)
>>> {
>>> --=20
>>> 2.25.1
>>>=20