> On Sep 26, 2022, at 6:37 PM, Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
>
> On Mon, Sep 26, 2022 at 09:07:12PM +0000, Joel Fernandes wrote:
>> Hi Paul,
>>
>> 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);
>>>>>>>
>>>>>>> This WRITE_ONCE() can be dropped out of the "if" statement, correct?
>>>>>>
>>>>>> Yes will update.
>>>>>
>>>>> Thank you!
>>>>>
>>>>>>> If so, this could be an "if" statement with two statements in its "then"
>>>>>>> clause, no "else" clause, and two statements following the "if" statement.
>>>>>>
>>>>>> I don’t think we can get rid of the else part but I’ll see what it looks like.
>>>>>
>>>>> In the function header, s/rhp/rhp_in/, then:
>>>>>
>>>>> struct rcu_head *rhp = rhp_in;
>>>>>
>>>>> And then:
>>>>>
>>>>> if (lazy && rhp) {
>>>>> rcu_cblist_enqueue(&rdp->nocb_bypass, rhp);
>>>>> rhp = NULL;
>>>>
>>>> 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:
>>>>
>>>> if (lazy and rhp) then
>>>> 1. flush bypass CBs on to main list.
>>>> 2. queue new CB on to main list.
>>>
>>> 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.
>>>
>>> Or am I on the wrong branch of this "if" statement?
>>
>> 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?
>
> I don't think so...
>
> 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 and my patch will be equivalent then. However I find your snippet a bit confusing, as in it is not immediately obvious - why would we queue something on to a list, if we were about to flush it. But any way, it does make it a clever piece of code in some sense and I am ok with doing it this way ;-)
Thanks,
- Joel
>
>>>> else
>>>> 1. flush bypass CBs on to main list
>>>> 2. queue new CB on to bypass list.
>>>>
>>>>> }
>>>>> rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
>>>>> WRITE_ONCE(rdp->lazy_len, 0);
>>>>>
>>>>> Or did I mess something up?
>>>>
>>>> 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, the
>>>> order is reversed.
>>>
>>> Did I pick the correct branch of the "if" statement above? Or were you
>>> instead talking about the "else" clause?
>>>
>>> I would have been more worried about getting cblist->len right.
>>
>> Hmm, I think my concern was more the ordering of callbacks, and moving the
>> write to length should be Ok.
>
> OK, sounds good to me! ;-)
>
>>>> 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 if/else
>>>> and move it to after the if/else, that would be cleanest. Does that sound
>>>> good to you? Thanks!
>>>
>>> Let's first figure out whether or not we are talking past one another. ;-)
>>
>> Haha yeah :-)
>
> So were we? ;-)
>
> Thanx, Paul
