On Fri, Mar 04, 2022 at 11:21:31PM +0000, Joel Fernandes wrote:
> 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@xxxxxxxxxxxxx>
> > ---
> > 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.
Here is hoping. ;-)
Thanx, Paul
> 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
> >
