Re: [PATCH] rcu-tasks: Use rcuwait for the rcu_tasks_kthread().

[Joel Fernandes <joel@joelfernandes.org>]

On Fri, Mar 04, 2022 at 12:07:25PM +0100, Sebastian Andrzej Siewior wrote:
> The waitqueue used by rcu_tasks_kthread() has always only one waiter.
> With a guaranteed only one waiter, this can be replaced with rcuwait
> which is smaller and simpler. With rcuwait based wake counterpart, the
> irqwork function (call_rcu_tasks_iw_wakeup()) can be invoked hardirq
> context because it is only a wake up and no sleeping locks are involved
> (unlike the wait_queue_head).
> As a side effect, this is also one piece of the puzzle to pass the RCU
> selftest at early boot on PREEMPT_RT.
> 
> Replace wait_queue_head with rcuwait and let the irqwork run in hardirq
> context on PREEMPT_RT.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> On 2022-03-03 12:02:37 [-0800], Paul E. McKenney wrote:
> > There is indeed only ever one waiter.  
> > 
> > But rcuwait is going to acquire scheduler locks, correct?  It would be
> > better to avoid that potential deadlock.
> 
> This is what I had in mind. Does this work for you?

In theory, Sebasatian's idea makes sense to me. Since scheduler locks are not
sleepable even on -RT, using rcuwait-based wakeup is OK with the added
benefit that the no locking would be needed during the wake. Hence, this wake
up can happen even from hardirq context in an -RT kernel.

Correct me if I got that wrong though.

thanks,

 - Joel


>  kernel/rcu/tasks.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index d64f0b1d8cd3b..f804afb304135 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -46,7 +46,7 @@ struct rcu_tasks_percpu {
>  
>  /**
>   * struct rcu_tasks - Definition for a Tasks-RCU-like mechanism.
> - * @cbs_wq: Wait queue allowing new callback to get kthread's attention.
> + * @cbs_wait: RCU wait allowing a new callback to get kthread's attention.
>   * @cbs_gbl_lock: Lock protecting callback list.
>   * @kthread_ptr: This flavor's grace-period/callback-invocation kthread.
>   * @gp_func: This flavor's grace-period-wait function.
> @@ -77,7 +77,7 @@ struct rcu_tasks_percpu {
>   * @kname: This flavor's kthread name.
>   */
>  struct rcu_tasks {
> -	struct wait_queue_head cbs_wq;
> +	struct rcuwait cbs_wait;
>  	raw_spinlock_t cbs_gbl_lock;
>  	int gp_state;
>  	int gp_sleep;
> @@ -113,11 +113,11 @@ static void call_rcu_tasks_iw_wakeup(struct irq_work *iwp);
>  #define DEFINE_RCU_TASKS(rt_name, gp, call, n)						\
>  static DEFINE_PER_CPU(struct rcu_tasks_percpu, rt_name ## __percpu) = {			\
>  	.lock = __RAW_SPIN_LOCK_UNLOCKED(rt_name ## __percpu.cbs_pcpu_lock),		\
> -	.rtp_irq_work = IRQ_WORK_INIT(call_rcu_tasks_iw_wakeup),			\
> +	.rtp_irq_work = IRQ_WORK_INIT_HARD(call_rcu_tasks_iw_wakeup),			\
>  };											\
>  static struct rcu_tasks rt_name =							\
>  {											\
> -	.cbs_wq = __WAIT_QUEUE_HEAD_INITIALIZER(rt_name.cbs_wq),			\
> +	.cbs_wait = __RCUWAIT_INITIALIZER(rt_name.wait),				\
>  	.cbs_gbl_lock = __RAW_SPIN_LOCK_UNLOCKED(rt_name.cbs_gbl_lock),			\
>  	.gp_func = gp,									\
>  	.call_func = call,								\
> @@ -261,7 +261,7 @@ static void call_rcu_tasks_iw_wakeup(struct irq_work *iwp)
>  	struct rcu_tasks_percpu *rtpcp = container_of(iwp, struct rcu_tasks_percpu, rtp_irq_work);
>  
>  	rtp = rtpcp->rtpp;
> -	wake_up(&rtp->cbs_wq);
> +	rcuwait_wake_up(&rtp->cbs_wait);
>  }
>  
>  // Enqueue a callback for the specified flavor of Tasks RCU.
> @@ -509,7 +509,9 @@ static int __noreturn rcu_tasks_kthread(void *arg)
>  		set_tasks_gp_state(rtp, RTGS_WAIT_CBS);
>  
>  		/* If there were none, wait a bit and start over. */
> -		wait_event_idle(rtp->cbs_wq, (needgpcb = rcu_tasks_need_gpcb(rtp)));
> +		rcuwait_wait_event(&rtp->cbs_wait,
> +				   (needgpcb = rcu_tasks_need_gpcb(rtp)),
> +				   TASK_IDLE);
>  
>  		if (needgpcb & 0x2) {
>  			// Wait for one grace period.
> -- 
> 2.35.1
>