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 v2 rcu 05/16] rcu: Refactor code a bit in rcu_nocb_do_flush_bypass()
Date:   Wed, 23 Nov 2022 20:00:18 -0500
Message-Id: <F3FB7417-0E5F-487A-A880-15964B676020@joelfernandes.org>
References: <20221123175437.GA4001@paulmck-ThinkPad-P17-Gen-1>
Cc:     Frederic Weisbecker <frederic@kernel.org>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@meta.com,
        rostedt@goodmis.org
In-Reply-To: <20221123175437.GA4001@paulmck-ThinkPad-P17-Gen-1>
To:     paulmck@kernel.org
Precedence: bulk


> On Nov 23, 2022, at 12:54 PM, Paul E. McKenney <paulmck@kernel.org> wrote:=

>=20
> =EF=BB=BFOn Wed, Nov 23, 2022 at 04:59:29PM +0100, Frederic Weisbecker wro=
te:
>>> On Mon, Nov 21, 2022 at 05:04:10PM -0800, Paul E. McKenney wrote:
>>> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>>>=20
>>> This consolidates the code a bit and makes it cleaner. Functionally it
>>> is the same.
>>>=20
>>> Reported-by: Paul E. McKenney <paulmck@kernel.org>
>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> ---
>>> kernel/rcu/tree_nocb.h | 17 +++++++++--------
>>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>>=20
>>> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
>>> index d6e4c076b0515..213daf81c057f 100644
>>> --- a/kernel/rcu/tree_nocb.h
>>> +++ b/kernel/rcu/tree_nocb.h
>>> @@ -327,10 +327,11 @@ static void wake_nocb_gp_defer(struct rcu_data *rd=
p, int waketype,
>>>  *
>>>  * Note that this function always returns true if rhp is NULL.
>>>  */
>>> -static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_h=
ead *rhp,
>>> +static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_h=
ead *rhp_in,
>>>                     unsigned long j, bool lazy)
>>> {
>>>    struct rcu_cblist rcl;
>>> +    struct rcu_head *rhp =3D rhp_in;
>>=20
>> Why that intermediate rhp_in?
>=20
> To avoid modifying the formal parameter, should the original value prove
> useful, for example, for tracing or debugging.

So as to not re assign function parameter and introduce bugs down the line (=
someone reading code thinks they passed a certain rhp but code is using some=
thing else later in the function).

Thanks.


>=20
>>>    WARN_ON_ONCE(!rcu_rdp_is_offloaded(rdp));
>>>    rcu_lockdep_assert_cblist_protected(rdp);
>>> @@ -345,16 +346,16 @@ static bool rcu_nocb_do_flush_bypass(struct rcu_da=
ta *rdp, struct rcu_head *rhp,
>>>=20
>>>    /*
>>>     * If the new CB requested was a lazy one, queue it onto the main
>>> -     * ->cblist so we can take advantage of a sooner grade period.
>>> +     * ->cblist so that we can take advantage of the grace-period that w=
ill
>>> +     * happen regardless. But queue it onto the bypass list first so th=
at
>>> +     * the lazy CB is ordered with the existing CBs in the bypass list.=

>>>     */
>>>    if (lazy && rhp) {
>>> -        rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, NULL);
>>> -        rcu_cblist_enqueue(&rcl, rhp);
>>> -        WRITE_ONCE(rdp->lazy_len, 0);
>>> -    } else {
>>> -        rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
>>> -        WRITE_ONCE(rdp->lazy_len, 0);
>>> +        rcu_cblist_enqueue(&rdp->nocb_bypass, rhp);
>>> +        rhp =3D NULL;
>>>    }
>>> +    rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
>>> +    WRITE_ONCE(rdp->lazy_len, 0);
>>=20
>> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
>=20
> Thank you!  I will apply this on my next rebase.
>=20
>                            Thanx, Paul
>=20
>> Thanks.
>>=20
>>>=20
>>>    rcu_segcblist_insert_pend_cbs(&rdp->cblist, &rcl);
>>>    WRITE_ONCE(rdp->nocb_bypass_first, j);
>>> --=20
>>> 2.31.1.189.g2e36527f23
>>>=20