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 v6 1/4] rcu: Make call_rcu() lazy to save power
Date:   Mon, 26 Sep 2022 19:33:17 -0400
Message-Id: <A78761F4-5922-418A-AFA3-01101C399778@joelfernandes.org>
References: <20220926223751.GZ4196@paulmck-ThinkPad-P17-Gen-1>
Cc:     rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
        rushikesh.s.kadam@intel.com, urezki@gmail.com,
        neeraj.iitr10@gmail.com, frederic@kernel.org, rostedt@goodmis.org
In-Reply-To: <20220926223751.GZ4196@paulmck-ThinkPad-P17-Gen-1>
To:     paulmck@kernel.org
Precedence: bulk


> On Sep 26, 2022, at 6:37 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
>=20
> =EF=BB=BFOn Mon, Sep 26, 2022 at 09:07:12PM +0000, Joel Fernandes wrote:
>> Hi Paul,
>>=20
>> On Mon, Sep 26, 2022 at 10:42:40AM -0700, Paul E. McKenney wrote:
>> [..]
>>>>>>>> +        WRITE_ONCE(rdp->lazy_len, 0);
>>>>>>>> +    } else {
>>>>>>>> +        rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
>>>>>>>> +        WRITE_ONCE(rdp->lazy_len, 0);
>>>>>>>=20
>>>>>>> This WRITE_ONCE() can be dropped out of the "if" statement, correct?=

>>>>>>=20
>>>>>> Yes will update.
>>>>>=20
>>>>> Thank you!
>>>>>=20
>>>>>>> If so, this could be an "if" statement with two statements in its "t=
hen"
>>>>>>> clause, no "else" clause, and two statements following the "if" stat=
ement.
>>>>>>=20
>>>>>> I don=E2=80=99t think we can get rid of the else part but I=E2=80=99l=
l see what it looks like.
>>>>>=20
>>>>> In the function header, s/rhp/rhp_in/, then:
>>>>>=20
>>>>>    struct rcu_head *rhp =3D rhp_in;
>>>>>=20
>>>>> And then:
>>>>>=20
>>>>>    if (lazy && rhp) {
>>>>>        rcu_cblist_enqueue(&rdp->nocb_bypass, rhp);
>>>>>        rhp =3D NULL;
>>>>=20
>>>> This enqueues on to the bypass list, where as if lazy && rhp, I want to=
 queue
>>>> the new rhp on to the main cblist. So the pseudo code in my patch is:
>>>>=20
>>>> if (lazy and rhp) then
>>>>    1. flush bypass CBs on to main list.
>>>>    2. queue new CB on to main list.
>>>=20
>>> And the difference is here, correct?  I enqueue to the bypass list,
>>> which is then flushed (in order) to the main list.  In contrast, you
>>> flush the bypass list, then enqueue to the main list.  Either way,
>>> the callback referenced by rhp ends up at the end of ->cblist.
>>>=20
>>> Or am I on the wrong branch of this "if" statement?
>>=20
>> But we have to flush first, and then queue the new one. Otherwise wouldn'=
t
>> the callbacks be invoked out of order? Or did I miss something?
>=20
> I don't think so...
>=20
> We want the new callback to be last, right?  One way to do that is to
> flush the bypass, then queue the new callback onto ->cblist.  Another way
> to do that is to enqueue the new callback onto the end of the bypass,
> then flush the bypass.  Why wouldn't these result in the same order?

Yes you are right, sorry. I was fixated on the main list. Both your snippet a=
nd my patch will be equivalent then. However I find your snippet a bit confu=
sing, as in it is not immediately obvious - why would we queue something on t=
o a list, if we were about to flush it. But any way, it does make it a cleve=
r piece of code in some sense and I am ok with doing it this way ;-)

Thanks,

  - Joel


>=20
>>>> else
>>>>    1. flush bypass CBs on to main list
>>>>    2. queue new CB on to bypass list.
>>>>=20
>>>>>    }
>>>>>    rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
>>>>>    WRITE_ONCE(rdp->lazy_len, 0);
>>>>>=20
>>>>> Or did I mess something up?
>>>>=20
>>>> So the rcu_cblist_flush_enqueue() has to happen before the
>>>> rcu_cblist_enqueue() to preserve the ordering of flushing into the main=
 list,
>>>> and queuing on to the main list for the "if". Where as in your snip, th=
e
>>>> order is reversed.
>>>=20
>>> Did I pick the correct branch of the "if" statement above?  Or were you
>>> instead talking about the "else" clause?
>>>=20
>>> I would have been more worried about getting cblist->len right.
>>=20
>> Hmm, I think my concern was more the ordering of callbacks, and moving th=
e
>> write to length should be Ok.
>=20
> OK, sounds good to me!  ;-)
>=20
>>>> If I consolidate it then, it looks like the following. However, it is a=
 bit
>>>> more unreadable. I could instead just take the WRITE_ONCE out of both i=
f/else
>>>> and move it to after the if/else, that would be cleanest. Does that sou=
nd
>>>> good to you? Thanks!
>>>=20
>>> Let's first figure out whether or not we are talking past one another.  ;=
-)
>>=20
>> Haha yeah :-)
>=20
> So were we?  ;-)
>=20
>                            Thanx, Paul