On Fri, 28 Jul 2017 20:54:16 +0800 Boqun Feng <boqun.feng@xxxxxxxxx> wrote: > Hi Jonathan, > > FWIW, there is wakeup-missing issue in swake_up() and swake_up_all(): > > https://marc.info/?l=linux-kernel&m=149750022019663 > > and RCU begins to use swait/wake last year, so I thought this could be > relevant. > > Could you try the following patch and see if it works? Thanks. Sadly seems to be a no... Just splatted before I could even get the tracing set up. Back to staring at logs and hoping something will stand out! Jonathan > > Regards, > Boqun > > ------------------>8 > Subject: [PATCH] swait: Remove the lockless swait_active() check in > swake_up*() > > Steven Rostedt reported a potential race in RCU core because of > swake_up(): > > CPU0 CPU1 > ---- ---- > __call_rcu_core() { > > spin_lock(rnp_root) > need_wake = __rcu_start_gp() { > rcu_start_gp_advanced() { > gp_flags = FLAG_INIT > } > } > > rcu_gp_kthread() { > swait_event_interruptible(wq, > gp_flags & FLAG_INIT) { > spin_lock(q->lock) > > *fetch wq->task_list here! * > > list_add(wq->task_list, q->task_list) > spin_unlock(q->lock); > > *fetch old value of gp_flags here * > > spin_unlock(rnp_root) > > rcu_gp_kthread_wake() { > swake_up(wq) { > swait_active(wq) { > list_empty(wq->task_list) > > } * return false * > > if (condition) * false * > schedule(); > > In this case, a wakeup is missed, which could cause the rcu_gp_kthread > waits for a long time. > > The reason of this is that we do a lockless swait_active() check in > swake_up(). To fix this, we can either 1) add a smp_mb() in swake_up() > before swait_active() to provide the proper order or 2) simply remove > the swait_active() in swake_up(). > > The solution 2 not only fixes this problem but also keeps the swait and > wait API as close as possible, as wake_up() doesn't provide a full > barrier and doesn't do a lockless check of the wait queue either. > Moreover, there are users already using swait_active() to do their quick > checks for the wait queues, so it make less sense that swake_up() and > swake_up_all() do this on their own. > > This patch then removes the lockless swait_active() check in swake_up() > and swake_up_all(). > > Reported-by: Steven Rostedt <rostedt@xxxxxxxxxxx> > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx> > --- > kernel/sched/swait.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c > index 3d5610dcce11..2227e183e202 100644 > --- a/kernel/sched/swait.c > +++ b/kernel/sched/swait.c > @@ -33,9 +33,6 @@ void swake_up(struct swait_queue_head *q) > { > unsigned long flags; > > - if (!swait_active(q)) > - return; > - > raw_spin_lock_irqsave(&q->lock, flags); > swake_up_locked(q); > raw_spin_unlock_irqrestore(&q->lock, flags); > @@ -51,9 +48,6 @@ void swake_up_all(struct swait_queue_head *q) > struct swait_queue *curr; > LIST_HEAD(tmp); > > - if (!swait_active(q)) > - return; > - > raw_spin_lock_irq(&q->lock); > list_splice_init(&q->task_list, &tmp); > while (!list_empty(&tmp)) { -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
