linux-kernel - Re: [PATCH v2 1/8] rcu: Introduce call_rcu

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: <aef0f8a6-cde6-18f3-16ab-7e07a4413f31@joelfernandes.org>
Date:   Tue, 12 Jul 2022 16:53:48 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     paulmck@...nel.org
Cc:     rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
        rushikesh.s.kadam@...el.com, urezki@...il.com,
        neeraj.iitr10@...il.com, frederic@...nel.org, rostedt@...dmis.org,
        vineeth@...byteword.org
Subject: Re: [PATCH v2 1/8] rcu: Introduce call_rcu_lazy() API implementation


On 7/10/2022 12:03 PM, Paul E. McKenney wrote:
[..]
>>>> +	// Note that if the bypass list has lazy CBs, and the main list is
>>>> +	// empty, and rhp happens to be non-lazy, then we end up flushing all
>>>> +	// the lazy CBs to the main list as well. That's the right thing to do,
>>>> +	// since we are kick-starting RCU GP processing anyway for the non-lazy
>>>> +	// one, we can just reuse that GP for the already queued-up lazy ones.
>>>> +	if ((rdp->nocb_nobypass_count < nocb_nobypass_lim_per_jiffy && !lazy) ||
>>>> +	    (lazy && n_lazy_cbs >= qhimark)) {
>>>>  		rcu_nocb_lock(rdp);
>>>>  		*was_alldone = !rcu_segcblist_pend_cbs(&rdp->cblist);
>>>>  		if (*was_alldone)
>>>>  			trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
>>>> -					    TPS("FirstQ"));
>>>> -		WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, j));
>>>> +					    lazy ? TPS("FirstLazyQ") : TPS("FirstQ"));
>>>> +		WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, j, false));
>>>
>>> The "false" here instead of "lazy" is because the caller is to do the
>>> enqueuing, correct?
>>
>> There is no difference between using false or lazy here, because the bypass
>> flush is not also enqueuing the lazy callback, right?
>>
>> We can also pass lazy instead of false if that's less confusing.
>>
>> Or maybe I missed the issue you're raising?
> 
> I am mostly checking up on your intended meaning of "lazy" in various
> contexts.  It could mean only that the caller requested laziness, or in
> some cases it could mean that the callback actually will be lazy.
> 
> I can rationalize the "false" above as a "don't care" in this case
> because (as you say) there is not callback.  In which case this code
> is OK as is, as long as the header comment for rcu_nocb_flush_bypass()
> clearly states that this parameter has meaning only when there really
> is a callback being queued.

I decided to change this and the below to "lazy" variable instead of
true/false, as the code is cleaner and less confusing IMO. It makes
sense to me and in my testing it works fine. Hope that's Ok with you.

About changing the lazy length count to a flag, one drawback of doing
that is, say if there are some non-lazy CBs in the bypass list, then the
lazy shrinker will end up reporting an inaccurate count. Also
considering that it might be harder to add the count back later say if
we need it for tracing, I would say lets leave it as is. I will keep the
counter for v3 and we can discuss. Does that sound good to you?

I think some more testing, checkpatch running etc and I should be good
to send v3 :)

Thanks!

 - Joel


> 
>>>>  		WARN_ON_ONCE(rcu_cblist_n_cbs(&rdp->nocb_bypass));
>>>>  		return false; // Caller must enqueue the callback.
>>>>  	}
>>>>  
>>>>  	// If ->nocb_bypass has been used too long or is too full,
>>>>  	// flush ->nocb_bypass to ->cblist.
>>>> -	if ((ncbs && j != READ_ONCE(rdp->nocb_bypass_first)) ||
>>>> -	    ncbs >= qhimark) {
>>>> +	if ((ncbs && j != READ_ONCE(rdp->nocb_bypass_first)) || ncbs >= qhimark) {
>>>>  		rcu_nocb_lock(rdp);
>>>> -		if (!rcu_nocb_flush_bypass(rdp, rhp, j)) {
>>>> +		if (!rcu_nocb_flush_bypass(rdp, rhp, j, true)) {
>>>
>>> But shouldn't this "true" be "lazy"?  I don't see how we are guaranteed
>>> that the callback is in fact lazy at this point in the code.  Also,
>>> there is not yet a guarantee that the caller will do the enqueuing.
>>> So what am I missing?
>>
>> Sorry I screwed this part up. I think I meant 'false' here, if the list grew
>> too big- then I think I would prefer if the new lazy CB instead is treated as
>> non-lazy. But if that's too confusing, I will just pass 'lazy' instead. What
>> do you think?
> 
> Good point, if we are choosing to override the laziness requested by the
> caller, then it should say "true".  It would be good to have a comment
> saying that is what we are doing, correct?
> 
>> Will reply more to the rest of the comments soon, thanks!
> 
> Sounds good!  (Hey, wouldn't want you to be bored!)
> 
> 							Thanx, Paul