Re: [PATCH] rcu: Add "expedite always" switch

[Paul E. McKenney]

On Fri, Oct 05, 2012 at 12:00:53AM +0300, Antti P Miettinen wrote:
> Add a module parameter for always using expedited RCU primitives.

Cool, looks quite close!!!  Please see below for a couple of questions and
comments.

> Signed-off-by: Antti P Miettinen <amiettinen@xxxxxxxxxx>
> ---
>  include/linux/rcupdate.h |    2 ++
>  kernel/rcupdate.c        |    4 ++++
>  kernel/rcutiny_plugin.h  |    5 ++++-
>  kernel/rcutree.c         |   10 ++++++++--
>  kernel/rcutree_plugin.h  |    5 ++++-
>  kernel/srcu.c            |    4 +++-
>  6 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 7c968e4..b37efae 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -45,6 +45,8 @@
>  #include <linux/bug.h>
>  #include <linux/compiler.h>
> 
> +extern int rcu_expedited;
> +

This needs to move to kernel/rcu.h, otherwise it is being exported to
the entire kernel.  Don't get me wrong, if someone in the kernel needs
to be able switch to and from expedited mode, I am OK with their doing
that -- but via a proper API rather than a bare store to a variable.  ;-)

(That said, the networking guys are doing just fine by having
synchronize_net() call either synchronize_rcu_expedited() or
synchronize_rcu(), depending on internal networking locking state.
So I do not believe that we need an API, not yet, anyway.)

>  #ifdef CONFIG_RCU_TORTURE_TEST
>  extern int rcutorture_runnable; /* for sysctl */
>  #endif /* #ifdef CONFIG_RCU_TORTURE_TEST */
> diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> index 29ca1c6..6057e58 100644
> --- a/kernel/rcupdate.c
> +++ b/kernel/rcupdate.c
> @@ -46,12 +46,16 @@
>  #include <linux/export.h>
>  #include <linux/hardirq.h>
>  #include <linux/delay.h>
> +#include <linux/module.h>
> 
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/rcu.h>
> 
>  #include "rcu.h"
> 
> +int rcu_expedited;
> +module_param(rcu_expedited, int, 0644);

Ah, the joys of exported-to-userspace API design...  ;-)

Summary: Please s/0644/0/ and add a KERNEL_ATTR_RW()-style sysfs interface.

If you want the details, please read on!

This exports the ability to switch expedited mode on and off at boot
time and also via sysfs.  However, this approach to the sysfs interface
does not gracefully handle multiple things enabling and disabling
expedited mode concurrently.  There are a couple of possible approaches:

1.	Look at current use cases.  These seem to be networking
	applications such as wireshark that reportedly cause the kernel
	to do a large number of RCU grace periods when starting up.
	These would be best served by an enter/exit style that
	increments and decrements a counter.

2.	Of course, applications can abort without properly cleaning
	up, which would argue for some sort of open/close protocol
	relying on the fact that all the application's fds are
	closed on any sort of exit.  But that leaves scripts out.

3.	One way to handle the scripts is to use redirection tricks,
	opening (say) FD 9 to the device.

4.	Do the bare minimum initially, so that when/if other use
	cases appear, they can be accommodated -- but using real
	information about what they really need.

>From what I can see, #4 is the only approach with a decent chance.
So, what is the bare minimum?

a.	Boot-time setting, allowing small embedded systems to
	unconditionally speed up grace periods.  You have this with the
	module_param() above.  Setting the permissions to zero would
	disable the userspace direct-set ability that is likely to get
	us into backwards-compatibility hot water in the future.

b.	Maybe sysfs setting.  Initially, this can be simple "on" and "off",
	exported with root-only access (like you have above).  If more
	elaborate use cases appear, this might become the adminstrative
	override.  The key point is that this needs to go through some
	sort of separate function (for example, via the KERNEL_ATTR_RW()
	macro) so that if it needs to change in the future, we can do
	the change easily.

Or am I missing something?

The rest looks good!

							Thanx, Paul

> +
>  #ifdef CONFIG_PREEMPT_RCU
> 
>  /*
> diff --git a/kernel/rcutiny_plugin.h b/kernel/rcutiny_plugin.h
> index 3d01902..f85016a 100644
> --- a/kernel/rcutiny_plugin.h
> +++ b/kernel/rcutiny_plugin.h
> @@ -706,7 +706,10 @@ void synchronize_rcu(void)
>  		return;
> 
>  	/* Once we get past the fastpath checks, same code as rcu_barrier(). */
> -	rcu_barrier();
> +	if (rcu_expedited)
> +		synchronize_rcu_expedited();
> +	else
> +		rcu_barrier();
>  }
>  EXPORT_SYMBOL_GPL(synchronize_rcu);
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 4fb2376..744c117 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -2221,7 +2221,10 @@ void synchronize_sched(void)
>  			   "Illegal synchronize_sched() in RCU-sched read-side critical section");
>  	if (rcu_blocking_is_gp())
>  		return;
> -	wait_rcu_gp(call_rcu_sched);
> +	if (rcu_expedited)
> +		synchronize_sched_expedited();
> +	else
> +		wait_rcu_gp(call_rcu_sched);
>  }
>  EXPORT_SYMBOL_GPL(synchronize_sched);
> 
> @@ -2242,7 +2245,10 @@ void synchronize_rcu_bh(void)
>  			   "Illegal synchronize_rcu_bh() in RCU-bh read-side critical section");
>  	if (rcu_blocking_is_gp())
>  		return;
> -	wait_rcu_gp(call_rcu_bh);
> +	if (rcu_expedited)
> +		synchronize_rcu_bh_expedited();
> +	else
> +		wait_rcu_gp(call_rcu_bh);
>  }
>  EXPORT_SYMBOL_GPL(synchronize_rcu_bh);
> 
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index f921154..0e9ca8b 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -679,7 +679,10 @@ void synchronize_rcu(void)
>  			   "Illegal synchronize_rcu() in RCU read-side critical section");
>  	if (!rcu_scheduler_active)
>  		return;
> -	wait_rcu_gp(call_rcu);
> +	if (rcu_expedited)
> +		synchronize_rcu_expedited();
> +	else
> +		wait_rcu_gp(call_rcu);
>  }
>  EXPORT_SYMBOL_GPL(synchronize_rcu);
> 
> diff --git a/kernel/srcu.c b/kernel/srcu.c
> index 97c465e..00c4169 100644
> --- a/kernel/srcu.c
> +++ b/kernel/srcu.c
> @@ -464,7 +464,9 @@ static void __synchronize_srcu(struct srcu_struct *sp, int trycount)
>   */
>  void synchronize_srcu(struct srcu_struct *sp)
>  {
> -	__synchronize_srcu(sp, SYNCHRONIZE_SRCU_TRYCOUNT);
> +	__synchronize_srcu(sp, rcu_expedited
> +			   ? SYNCHRONIZE_SRCU_EXP_TRYCOUNT
> +			   : SYNCHRONIZE_SRCU_TRYCOUNT);
>  }
>  EXPORT_SYMBOL_GPL(synchronize_srcu);
> 
> -- 
> 1.7.4.1
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/