| Message ID | 20210602133040.271625424@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sched: Cleanup task_struct::state | expand |
On Wed, Jun 02, 2021 at 03:12:26PM +0200, Peter Zijlstra wrote: > Remove broken task->state references and let wake_up_process() DTRT. > > The anti-pattern in these patches breaks the ordering of ->state vs > COND as described in the comment near set_current_state() and can lead > to missed wakeups: > > (OoO load, observes RUNNING)<-. > for (;;) { | > t->state = UNINTERRUPTIBLE; | > smp_mb(); ,-----> ,' (OoO load, observed !COND) > | | > | | COND = 1; > | `- if (t->state != RUNNING) > | wake_up_process(t); // not done > if (COND) ---------' > break; > schedule(); // forever waiting > } > t->state = TASK_RUNNING; > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > drivers/net/ethernet/qualcomm/qca_spi.c | 6 ++---- > drivers/usb/gadget/udc/max3420_udc.c | 15 +++++---------- > drivers/usb/host/max3421-hcd.c | 3 +-- > kernel/softirq.c | 2 +- > 4 files changed, 9 insertions(+), 17 deletions(-) For USB stuff: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Wed, Jun 02, 2021 at 03:12:26PM +0200, Peter Zijlstra wrote: > Remove broken task->state references and let wake_up_process() DTRT. > > The anti-pattern in these patches breaks the ordering of ->state vs > COND as described in the comment near set_current_state() and can lead > to missed wakeups: > > (OoO load, observes RUNNING)<-. > for (;;) { | > t->state = UNINTERRUPTIBLE; | > smp_mb(); ,-----> ,' (OoO load, observed !COND) > | | > | | COND = 1; > | `- if (t->state != RUNNING) > | wake_up_process(t); // not done > if (COND) ---------' > break; > schedule(); // forever waiting > } > t->state = TASK_RUNNING; > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > drivers/net/ethernet/qualcomm/qca_spi.c | 6 ++---- > drivers/usb/gadget/udc/max3420_udc.c | 15 +++++---------- > drivers/usb/host/max3421-hcd.c | 3 +-- > kernel/softirq.c | 2 +- > 4 files changed, 9 insertions(+), 17 deletions(-) Acked-by: Will Deacon <will@kernel.org> I couldn't spot any others. Will
On Wed, 02 Jun 2021, Peter Zijlstra wrote: >Remove broken task->state references and let wake_up_process() DTRT. > >The anti-pattern in these patches breaks the ordering of ->state vs >COND as described in the comment near set_current_state() and can lead >to missed wakeups: > > (OoO load, observes RUNNING)<-. > for (;;) { | > t->state = UNINTERRUPTIBLE; | > smp_mb(); ,-----> ,' (OoO load, observed !COND) > | | > | | COND = 1; > | `- if (t->state != RUNNING) > | wake_up_process(t); // not done > if (COND) ---------' > break; > schedule(); // forever waiting > } > t->state = TASK_RUNNING; > >Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Davidlohr Bueso <dbueso@suse.de>
--- a/drivers/net/ethernet/qualcomm/qca_spi.c +++ b/drivers/net/ethernet/qualcomm/qca_spi.c @@ -653,8 +653,7 @@ qcaspi_intr_handler(int irq, void *data) struct qcaspi *qca = data; qca->intr_req++; - if (qca->spi_thread && - qca->spi_thread->state != TASK_RUNNING) + if (qca->spi_thread) wake_up_process(qca->spi_thread); return IRQ_HANDLED; @@ -777,8 +776,7 @@ qcaspi_netdev_xmit(struct sk_buff *skb, netif_trans_update(dev); - if (qca->spi_thread && - qca->spi_thread->state != TASK_RUNNING) + if (qca->spi_thread) wake_up_process(qca->spi_thread); return NETDEV_TX_OK; --- a/drivers/usb/gadget/udc/max3420_udc.c +++ b/drivers/usb/gadget/udc/max3420_udc.c @@ -509,8 +509,7 @@ static irqreturn_t max3420_vbus_handler( ? USB_STATE_POWERED : USB_STATE_NOTATTACHED); spin_unlock_irqrestore(&udc->lock, flags); - if (udc->thread_task && - udc->thread_task->state != TASK_RUNNING) + if (udc->thread_task) wake_up_process(udc->thread_task); return IRQ_HANDLED; @@ -529,8 +528,7 @@ static irqreturn_t max3420_irq_handler(i } spin_unlock_irqrestore(&udc->lock, flags); - if (udc->thread_task && - udc->thread_task->state != TASK_RUNNING) + if (udc->thread_task) wake_up_process(udc->thread_task); return IRQ_HANDLED; @@ -1093,8 +1091,7 @@ static int max3420_wakeup(struct usb_gad spin_unlock_irqrestore(&udc->lock, flags); - if (udc->thread_task && - udc->thread_task->state != TASK_RUNNING) + if (udc->thread_task) wake_up_process(udc->thread_task); return ret; } @@ -1117,8 +1114,7 @@ static int max3420_udc_start(struct usb_ udc->todo |= UDC_START; spin_unlock_irqrestore(&udc->lock, flags); - if (udc->thread_task && - udc->thread_task->state != TASK_RUNNING) + if (udc->thread_task) wake_up_process(udc->thread_task); return 0; @@ -1137,8 +1133,7 @@ static int max3420_udc_stop(struct usb_g udc->todo |= UDC_START; spin_unlock_irqrestore(&udc->lock, flags); - if (udc->thread_task && - udc->thread_task->state != TASK_RUNNING) + if (udc->thread_task) wake_up_process(udc->thread_task); return 0; --- a/drivers/usb/host/max3421-hcd.c +++ b/drivers/usb/host/max3421-hcd.c @@ -1169,8 +1169,7 @@ max3421_irq_handler(int irq, void *dev_i struct spi_device *spi = to_spi_device(hcd->self.controller); struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd); - if (max3421_hcd->spi_thread && - max3421_hcd->spi_thread->state != TASK_RUNNING) + if (max3421_hcd->spi_thread) wake_up_process(max3421_hcd->spi_thread); if (!test_and_set_bit(ENABLE_IRQ, &max3421_hcd->todo)) disable_irq_nosync(spi->irq); --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -76,7 +76,7 @@ static void wakeup_softirqd(void) /* Interrupts are disabled: no need to stop preemption */ struct task_struct *tsk = __this_cpu_read(ksoftirqd); - if (tsk && tsk->state != TASK_RUNNING) + if (tsk) wake_up_process(tsk); }
Remove broken task->state references and let wake_up_process() DTRT. The anti-pattern in these patches breaks the ordering of ->state vs COND as described in the comment near set_current_state() and can lead to missed wakeups: (OoO load, observes RUNNING)<-. for (;;) { | t->state = UNINTERRUPTIBLE; | smp_mb(); ,-----> ,' (OoO load, observed !COND) | | | | COND = 1; | `- if (t->state != RUNNING) | wake_up_process(t); // not done if (COND) ---------' break; schedule(); // forever waiting } t->state = TASK_RUNNING; Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- drivers/net/ethernet/qualcomm/qca_spi.c | 6 ++---- drivers/usb/gadget/udc/max3420_udc.c | 15 +++++---------- drivers/usb/host/max3421-hcd.c | 3 +-- kernel/softirq.c | 2 +- 4 files changed, 9 insertions(+), 17 deletions(-)