Re: [PATCH] rcu/tree: Force quiescent state on callback overload

[Neeraj Upadhyay <neeraju@codeaurora.org>]

Hi Paul,

On 6/23/2020 4:23 AM, Paul E. McKenney wrote:
> On Mon, Jun 22, 2020 at 09:16:24AM +0530, Neeraj Upadhyay wrote:
>> Hi Paul,
>>
>> On 6/22/2020 8:43 AM, Paul E. McKenney wrote:
>>> On Mon, Jun 22, 2020 at 01:30:31AM +0530, Neeraj Upadhyay wrote:
>>>> Hi Paul,
>>>>
>>>> On 6/22/2020 1:20 AM, Paul E. McKenney wrote:
>>>>> On Mon, Jun 22, 2020 at 12:07:27AM +0530, Neeraj Upadhyay wrote:
>>>>>> On callback overload, we want to force quiescent state immediately,
>>>>>> for the first and second fqs. Enforce the same, by including
>>>>>> RCU_GP_FLAG_OVLD flag, in fqsstart check.
>>>>>>
>>>>>> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
>>>>>
>>>>> Good catch!
>>>>>
>>>>> But what did you do to verify that this change does the right thing?
>>>>>
>>>>> 						Thanx, Paul
>>>>>
>>>>
>>>> I haven't done a runtime verification of this code path; I posted this,
>>>> based on review of this code.
>>>
>>> My concern is that under overload, the FQS scans would happen continuously
>>> rather than accelerating only the first such scan in a given grace period.
>>> This would of course result in a CPU-bound grace-period kthread, which
>>> users might not be all that happy with.
>>>
>>> Or am I missing something subtle that prevents this?
>>
>> Looks like under overload, only the first and second scans are accelerated?
>>
>>      gf = 0;
>>      if (first_gp_fqs) {
>>           first_gp_fqs = false;
>>            gf = rcu_state.cbovld ? RCU_GP_FLAG_OVLD : 0;
>>      }
> 
> Very good, it does sound like you understand this, and it matches my
> analysis and passes light testing, so I queued this one.  I did improve
> the commit log, please check below.  The added detail is helpful to people
> (including ourselves, by the way) who might need to look at this commit
> some time in the future.
> 

Thanks; patch looks good; I will try to put more efforts on commit log
for future patches.

> If you have an x86 system lying around, running rcutorture is quite
> straightforward.  Non-x86 systems can also run rcutorture, if nothing
> else by using modprobe and rmmod as described here:
> 
> https://paulmck.livejournal.com/57769.html
> 
> The scripting described in the latter part of this document has worked
> on ARMv8 and PowerPC, and might still work for all I know.

I will set it up at my end; the livejournal is pretty detailed! Thanks
for sharing this!


Thanks
Neeraj

> 
> 						Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 9482524d7dd0aea5d32a6efa2979223eea07c029
> Author: Neeraj Upadhyay <neeraju@codeaurora.org>
> Date:   Mon Jun 22 00:07:27 2020 +0530
> 
>      rcu/tree: Force quiescent state on callback overload
>      
>      On callback overload, it is necessary to quickly detect idle CPUs,
>      and rcu_gp_fqs_check_wake() checks for this condition.  Unfortunately,
>      the code following the call to this function does not repeat this check,
>      which means that in reality no actual quiescent-state forcing, instead
>      only a couple of quick and pointless wakeups at the beginning of the
>      grace period.
>      
>      This commit therefore adds a check for the RCU_GP_FLAG_OVLD flag in
>      the post-wakeup "if" statement in rcu_gp_fqs_loop().
>      
>      Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
>      Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index d0988a1..6226bfb 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1865,7 +1865,7 @@ static void rcu_gp_fqs_loop(void)
>   			break;
>   		/* If time for quiescent-state forcing, do it. */
>   		if (!time_after(rcu_state.jiffies_force_qs, jiffies) ||
> -		    (gf & RCU_GP_FLAG_FQS)) {
> +		    (gf & (RCU_GP_FLAG_FQS | RCU_GP_FLAG_OVLD))) {
>   			trace_rcu_grace_period(rcu_state.name, rcu_state.gp_seq,
>   					       TPS("fqsstart"));
>   			rcu_gp_fqs(first_gp_fqs);
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by The Linux Foundation