Re: [PATCH] rcu: optimize rcutiny

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

On Thu, Apr 28, 2011 at 07:23:45AM +0200, Eric Dumazet wrote:
> rcu_sched_qs() currently calls local_irq_save()/local_irq_restore() up
> to three times.
> 
> Remove irq masking from rcu_qsctr_help() / invoke_rcu_kthread()
> and do it once in rcu_sched_qs() / rcu_bh_qs()
> 
> This generates smaller code as well.
> 
> # size kernel/rcutiny.old.o kernel/rcutiny.new.o
>    text	   data	    bss	    dec	    hex	filename
>    2314	    156	     24	   2494	    9be	kernel/rcutiny.old.o
>    2250	    156	     24	   2430	    97e	kernel/rcutiny.new.o
> 
> Fix an outdated comment for rcu_qsctr_help()
> Move invoke_rcu_kthread() definition before its use.

Looks very nice!  In theory, this does lengthen the time during which
interrupts are disabled, but in practice I believe that that this would
not be measurable.  Adding Thomas on CC in case I am mistaken about
the effect of longer irq-disable regions.

In the meantime, I have queued this, and either way, thank you, Eric!

							Thanx, Paul

> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  kernel/rcutiny.c |   42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> index 0c343b9..29eb349 100644
> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -40,7 +40,6 @@
>  static struct task_struct *rcu_kthread_task;
>  static DECLARE_WAIT_QUEUE_HEAD(rcu_kthread_wq);
>  static unsigned long have_rcu_kthread_work;
> -static void invoke_rcu_kthread(void);
> 
>  /* Forward declarations for rcutiny_plugin.h. */
>  struct rcu_ctrlblk;
> @@ -79,36 +78,45 @@ void rcu_exit_nohz(void)
>  #endif /* #ifdef CONFIG_NO_HZ */
> 
>  /*
> - * Helper function for rcu_qsctr_inc() and rcu_bh_qsctr_inc().
> - * Also disable irqs to avoid confusion due to interrupt handlers
> + * Helper function for rcu_sched_qs() and rcu_bh_qs().
> + * Also irqs are disabled to avoid confusion due to interrupt handlers
>   * invoking call_rcu().
>   */
>  static int rcu_qsctr_help(struct rcu_ctrlblk *rcp)
>  {
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
>  	if (rcp->rcucblist != NULL &&
>  	    rcp->donetail != rcp->curtail) {
>  		rcp->donetail = rcp->curtail;
> -		local_irq_restore(flags);
>  		return 1;
>  	}
> -	local_irq_restore(flags);
> 
>  	return 0;
>  }
> 
>  /*
> + * Wake up rcu_kthread() to process callbacks now eligible for invocation
> + * or to boost readers.
> + */
> +static void invoke_rcu_kthread(void)
> +{
> +	have_rcu_kthread_work = 1;
> +	wake_up(&rcu_kthread_wq);
> +}
> +
> +/*
>   * Record an rcu quiescent state.  And an rcu_bh quiescent state while we
>   * are at it, given that any rcu quiescent state is also an rcu_bh
>   * quiescent state.  Use "+" instead of "||" to defeat short circuiting.
>   */
>  void rcu_sched_qs(int cpu)
>  {
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
>  	if (rcu_qsctr_help(&rcu_sched_ctrlblk) +
>  	    rcu_qsctr_help(&rcu_bh_ctrlblk))
>  		invoke_rcu_kthread();
> +	local_irq_restore(flags);
>  }
> 
>  /*
> @@ -116,8 +124,12 @@ void rcu_sched_qs(int cpu)
>   */
>  void rcu_bh_qs(int cpu)
>  {
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
>  	if (rcu_qsctr_help(&rcu_bh_ctrlblk))
>  		invoke_rcu_kthread();
> +	local_irq_restore(flags);
>  }
> 
>  /*
> @@ -208,20 +220,6 @@ static int rcu_kthread(void *arg)
>  }
> 
>  /*
> - * Wake up rcu_kthread() to process callbacks now eligible for invocation
> - * or to boost readers.
> - */
> -static void invoke_rcu_kthread(void)
> -{
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
> -	have_rcu_kthread_work = 1;
> -	wake_up(&rcu_kthread_wq);
> -	local_irq_restore(flags);
> -}
> -
> -/*
>   * Wait for a grace period to elapse.  But it is illegal to invoke
>   * synchronize_sched() from within an RCU read-side critical section.
>   * Therefore, any legal call to synchronize_sched() is a quiescent
> 
>