Re: [PATCH] rcu: Benefit from expedited grace period in __wait_rcu_gp

["Paul E. McKenney" <paulmck@linux.ibm.com>]

On Fri, Oct 19, 2018 at 02:49:05AM +0200, KarimAllah Ahmed wrote:
> When expedited grace-period is set, both synchronize_sched
> synchronize_rcu_bh can be optimized to have a significantly lower latency.
> 
> Improve wait_rcu_gp handling to also account for expedited grace-period.
> The downside is that wait_rcu_gp will not wait anymore for all RCU variants
> concurrently when an expedited grace-period is set, however, given the
> improved latency it does not really matter.
> 
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>

Cute!

Unfortunately, there are a few problems with this patch:

1.	I will be eliminating synchronize_rcu_mult() due to the fact that
	the upcoming RCU flavor consolidation eliminates its sole caller.
	See 5fc9d4e000b1 ("rcu: Eliminate synchronize_rcu_mult()")
	in my -rcu tree.  This would of course also eliminate the effects
	of this patch.

2.	The real-time guys' users are not going to be at all happy
	with the IPIs resulting from the _expedited() API members.
	Yes, they can boot with rcupdate.rcu_normal=1, but they don't
	always need that big a hammer, and use of this kernel parameter
	can slow down boot, hibernation, suspend, network configuration,
	and much else besides.	We therefore don't want them to have to
	use rcupdate.rcu_normal=1 unless absolutely necessary.

3.	If the real-time guys' users were to have booted with
	rcupdate.rcu_normal=1, then synchronize_sched_expedited()
	would invoke _synchronize_rcu_expedited, which would invoke
	wait_rcu_gp(), which would invoke _wait_rcu_gp() which would
	invoke __wait_rcu_gp(), which, given your patch, would in turn
	invoke synchronize_sched_expedited().  This situation could
	well prevent their systems from meeting their response-time
	requirements.

So I cannot accept this patch nor for that matter any similar patch.

But what were you really trying to get done here?  If you were thinking
of adding another synchronize_rcu_mult(), the flavor consolidation will
make that unnecessary in most cases.  If you are trying to speed up
CPU-hotplug operations, I suggest using the rcu_expedited sysctl variable
when taking a CPU offline.  If something else, please let me know what
it is so that we can work out how the problem might best be solved.

							Thanx, Paul

> ---
>  kernel/rcu/update.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 68fa19a..44b8817 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -392,13 +392,27 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
>  			might_sleep();
>  			continue;
>  		}
> -		init_rcu_head_on_stack(&rs_array[i].head);
> -		init_completion(&rs_array[i].completion);
> +
>  		for (j = 0; j < i; j++)
>  			if (crcu_array[j] == crcu_array[i])
>  				break;
> -		if (j == i)
> -			(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
> +		if (j != i)
> +			continue;
> +
> +		if ((crcu_array[i] == call_rcu_sched ||
> +		     crcu_array[i] == call_rcu_bh)
> +		    && rcu_gp_is_expedited()) {
> +			if (crcu_array[i] == call_rcu_sched)
> +				synchronize_sched_expedited();
> +			else
> +				synchronize_rcu_bh_expedited();
> +
> +			continue;
> +		}
> +
> +		init_rcu_head_on_stack(&rs_array[i].head);
> +		init_completion(&rs_array[i].completion);
> +		(crcu_array[i])(&rs_array[i].head, wakeme_after_rcu);
>  	}
> 
>  	/* Wait for all callbacks to be invoked. */
> @@ -407,11 +421,19 @@ void __wait_rcu_gp(bool checktiny, int n, call_rcu_func_t *crcu_array,
>  		    (crcu_array[i] == call_rcu ||
>  		     crcu_array[i] == call_rcu_bh))
>  			continue;
> +
> +		if ((crcu_array[i] == call_rcu_sched ||
> +		     crcu_array[i] == call_rcu_bh)
> +		    && rcu_gp_is_expedited())
> +			continue;
> +
>  		for (j = 0; j < i; j++)
>  			if (crcu_array[j] == crcu_array[i])
>  				break;
> -		if (j == i)
> -			wait_for_completion(&rs_array[i].completion);
> +		if (j != i)
> +			continue;
> +
> +		wait_for_completion(&rs_array[i].completion);
>  		destroy_rcu_head_on_stack(&rs_array[i].head);
>  	}
>  }
> -- 
> 2.7.4
>