Re: [PATCH] rcu: Allow to eliminate softirq processing from rcutree

[Paul E. McKenney]

On Thu, Mar 21, 2019 at 08:06:56AM -0400, Joel Fernandes wrote:
> On Wed, Mar 20, 2019 at 12:28:35PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2019-03-19 20:26:13 [-0400], Joel Fernandes wrote:
> > > > @@ -2769,19 +2782,121 @@ static void invoke_rcu_callbacks(struct rcu_data *rdp)
> > > >  {
> > > >  	if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
> > > >  		return;
> > > > -	if (likely(!rcu_state.boost)) {
> > > > -		rcu_do_batch(rdp);
> > > > -		return;
> > > > -	}
> > > > -	invoke_rcu_callbacks_kthread();
> > > > +	rcu_do_batch(rdp);
> > > 
> > > Looks like a nice change, but one question...
> > > 
> > > Consider the case where rcunosoftirq boot option is not passed.
> > > 
> > > Before, if RCU_BOOST=y, then callbacks would be invoked in rcuc threads if
> > > possible, by those threads being woken up from within the softirq context
> > > (in invoke_rcu_callbacks).
> > > 
> > > Now, if RCU_BOOST=y, then callbacks would only be invoked in softirq context
> > > and not in the threads at all. Because rcu_softirq_enabled = false, so the
> > > path executes:
> > >   rcu_read_unlock_special() ->
> > >         raise_softirq_irqsoff() ->
> > >                 rcu_process_callbacks_si() ->
> > >                         rcu_process_callbacks() ->
> > >                                 invoke_rcu_callbacks() ->
> > >                                         rcu_do_batch()
> > > 
> > > This seems like a behavioral change to me. This makes the callbacks always
> > > execute from the softirq context and not the threads when boosting is
> > > configured. IMO in the very least, such behavioral change should be
> > > documented in the change.
> > > 
> > > One way to fix this I think could be, if boosting is enabled, then set
> > > rcu_softirq_enabled to false by default so the callbacks are still executed
> > > in the rcuc threads.
> > > 
> > > Did I miss something? Sorry if I did, thanks!
> > 
> > So with all the swaps and reorder we talking about this change:
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 0a719f726e149..82810483bfc6c 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2306,20 +2306,6 @@ static void rcu_core_si(struct softirq_action *h)
> >  	rcu_core();
> >  }
> >  
> > -/*
> > - * Schedule RCU callback invocation.  If the running implementation of RCU
> > - * does not support RCU priority boosting, just do a direct call, otherwise
> > - * wake up the per-CPU kernel kthread.  Note that because we are running
> > - * on the current CPU with softirqs disabled, the rcu_cpu_kthread_task
> > - * cannot disappear out from under us.
> > - */
> > -static void invoke_rcu_callbacks(struct rcu_data *rdp)
> > -{
> > -	if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
> > -		return;
> > -	rcu_do_batch(rdp);
> > -}
> > -
> >  static void rcu_wake_cond(struct task_struct *t, int status)
> >  {
> >  	/*
> > @@ -2330,6 +2316,19 @@ static void rcu_wake_cond(struct task_struct *t, int status)
> >  		wake_up_process(t);
> >  }
> >  
> > +static void invoke_rcu_core_kthread(void)
> > +{
> > +	struct task_struct *t;
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +	__this_cpu_write(rcu_data.rcu_cpu_has_work, 1);
> > +	t = __this_cpu_read(rcu_data.rcu_cpu_kthread_task);
> > +	if (t != NULL && t != current)
> > +		rcu_wake_cond(t, __this_cpu_read(rcu_data.rcu_cpu_kthread_status));
> > +	local_irq_restore(flags);
> > +}
> > +
> >  static bool rcu_softirq_enabled = true;
> >  
> >  static int __init rcunosoftirq_setup(char *str)
> > @@ -2339,26 +2338,33 @@ static int __init rcunosoftirq_setup(char *str)
> >  }
> >  __setup("rcunosoftirq", rcunosoftirq_setup);
> >  
> > +/*
> > + * Schedule RCU callback invocation.  If the running implementation of RCU
> > + * does not support RCU priority boosting, just do a direct call, otherwise
> > + * wake up the per-CPU kernel kthread.  Note that because we are running
> > + * on the current CPU with softirqs disabled, the rcu_cpu_kthread_task
> > + * cannot disappear out from under us.
> > + */
> > +static void invoke_rcu_callbacks(struct rcu_data *rdp)
> > +{
> > +	if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
> > +		return;
> > +	if (rcu_state.boost || rcu_softirq_enabled)
> > +		invoke_rcu_core_kthread();
> 
> Here shouldn't it be this?
> 	if (rcu_state.boost || !rcu_softirq_enabled)
> 
> Also the rcu/dev branch has the following hunk where we unconditionally
> invoke rcu_do_batch even when boosting which would still have the issue I
> pointed. I would suggest Sebastian to post the latest v4 or v5 with all diff
> squashed, and then we do another round of review with latest patch, thanks!

I believe that -rcu has this change.  But it looks like there still are
failures, so yes, further review is necessary and deeply appreciated!

							Thanx, Paul

> 	@@ -2306,18 +2320,110 @@ static void invoke_rcu_callbacks(struct rcu_data *rdp)
>  {
>  	if (unlikely(!READ_ONCE(rcu_scheduler_fully_active)))
>  		return;
> -	if (likely(!rcu_state.boost)) {
> -		rcu_do_batch(rdp);
> -		return;
> -	}
> -	invoke_rcu_callbacks_kthread();
> +	rcu_do_batch(rdp);
> +}
> +
> 
> thanks,
> 
>  - Joel
> 
> 
> > +	rcu_do_batch(rdp);
> > +}
> > +
> >  /*
> >   * Wake up this CPU's rcuc kthread to do RCU core processing.
> >   */
> >  static void invoke_rcu_core(void)
> >  {
> > -	unsigned long flags;
> > -	struct task_struct *t;
> > -
> >  	if (!cpu_online(smp_processor_id()))
> >  		return;
> > -	if (rcu_softirq_enabled) {
> > +	if (rcu_softirq_enabled)
> >  		raise_softirq(RCU_SOFTIRQ);
> > -	} else {
> > -		local_irq_save(flags);
> > -		__this_cpu_write(rcu_data.rcu_cpu_has_work, 1);
> > -		t = __this_cpu_read(rcu_data.rcu_cpu_kthread_task);
> > -		if (t != NULL && t != current)
> > -			rcu_wake_cond(t, __this_cpu_read(rcu_data.rcu_cpu_kthread_status));
> > -		local_irq_restore(flags);
> > -	}
> > +	else
> > +		invoke_rcu_core_kthread();
> >  }
> >  
> >  static void rcu_cpu_kthread_park(unsigned int cpu)
> > @@ -2426,7 +2432,8 @@ static int __init rcu_spawn_core_kthreads(void)
> >  		per_cpu(rcu_data.rcu_cpu_has_work, cpu) = 0;
> >  	if (!IS_ENABLED(CONFIG_RCU_BOOST) && !rcu_softirq_enabled)
> >  		return 0;
> > -	WARN_ONCE(smpboot_register_percpu_thread(&rcu_cpu_thread_spec), "%s: Could not start rcub kthread, OOM is now expected behavior\n", __func__);
> > +	WARN_ONCE(smpboot_register_percpu_thread(&rcu_cpu_thread_spec),
> > +		  "%s: Could not start rcuc kthread, OOM is now expected behavior\n", __func__);
> >  	return 0;
> >  }
> >  early_initcall(rcu_spawn_core_kthreads);
> > -- 
> > 2.20.1
> > 
> > >  - Joel
> > 
> > Sebastian
>