linux-kernel - Re: [PATCH v2 rcu 05/16] rcu: Refactor code a bit in rcu_nocb_do_flush

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <F3FB7417-0E5F-487A-A880-15964B676020@joelfernandes.org>
Date:   Wed, 23 Nov 2022 20:00:18 -0500
From:   Joel Fernandes <joel@...lfernandes.org>
To:     paulmck@...nel.org
Cc:     Frederic Weisbecker <frederic@...nel.org>, rcu@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@...a.com,
        rostedt@...dmis.org
Subject: Re: [PATCH v2 rcu 05/16] rcu: Refactor code a bit in rcu_nocb_do_flush_bypass()



> On Nov 23, 2022, at 12:54 PM, Paul E. McKenney <paulmck@...nel.org> wrote:
> 
> On Wed, Nov 23, 2022 at 04:59:29PM +0100, Frederic Weisbecker wrote:
>>> On Mon, Nov 21, 2022 at 05:04:10PM -0800, Paul E. McKenney wrote:
>>> From: "Joel Fernandes (Google)" <joel@...lfernandes.org>
>>> 
>>> This consolidates the code a bit and makes it cleaner. Functionally it
>>> is the same.
>>> 
>>> Reported-by: Paul E. McKenney <paulmck@...nel.org>
>>> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
>>> ---
>>> kernel/rcu/tree_nocb.h | 17 +++++++++--------
>>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>> 
>>> 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 *rdp, 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_head *rhp,
>>> +static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp_in,
>>>                     unsigned long j, bool lazy)
>>> {
>>>    struct rcu_cblist rcl;
>>> +    struct rcu_head *rhp = rhp_in;
>> 
>> Why that intermediate rhp_in?
> 
> 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 something else later in the function).

Thanks.



> 
>>>    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_data *rdp, struct rcu_head *rhp,
>>> 
>>>    /*
>>>     * 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 will
>>> +     * happen regardless. But queue it onto the bypass list first so that
>>> +     * 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 = NULL;
>>>    }
>>> +    rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
>>> +    WRITE_ONCE(rdp->lazy_len, 0);
>> 
>> Reviewed-by: Frederic Weisbecker <frederic@...nel.org>
> 
> Thank you!  I will apply this on my next rebase.
> 
>                            Thanx, Paul
> 
>> Thanks.
>> 
>>> 
>>>    rcu_segcblist_insert_pend_cbs(&rdp->cblist, &rcl);
>>>    WRITE_ONCE(rdp->nocb_bypass_first, j);
>>> -- 
>>> 2.31.1.189.g2e36527f23
>>>