Message ID | 1424455977-21903-6-git-send-email-fabf@skynet.be |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Fabian Frederick wrote:
: Use helper functions to access current->state.
: Direct assignments are prone to races and therefore buggy.
:
: current->state = TASK_RUNNING is replaced by __set_current_state()
:
: Thanks to Peter Zijlstra for the exact definition of the problem.
OK, thanks. Although I wonder whether real users of COSA (an ISA-based
WAN card) do exist.
Acked-By: Jan "Yenya" Kasprzak <kas@fi.muni.cz>
-Y.
:
: Suggested-By: Peter Zijlstra <peterz@infradead.org>
: Signed-off-by: Fabian Frederick <fabf@skynet.be>
: ---
: drivers/net/wan/cosa.c | 12 ++++++------
: 1 file changed, 6 insertions(+), 6 deletions(-)
:
: diff --git a/drivers/net/wan/cosa.c b/drivers/net/wan/cosa.c
: index 83c39e2..88d121d 100644
: --- a/drivers/net/wan/cosa.c
: +++ b/drivers/net/wan/cosa.c
: @@ -806,21 +806,21 @@ static ssize_t cosa_read(struct file *file,
: spin_lock_irqsave(&cosa->lock, flags);
: add_wait_queue(&chan->rxwaitq, &wait);
: while (!chan->rx_status) {
: - current->state = TASK_INTERRUPTIBLE;
: + set_current_state(TASK_INTERRUPTIBLE);
: spin_unlock_irqrestore(&cosa->lock, flags);
: schedule();
: spin_lock_irqsave(&cosa->lock, flags);
: if (signal_pending(current) && chan->rx_status == 0) {
: chan->rx_status = 1;
: remove_wait_queue(&chan->rxwaitq, &wait);
: - current->state = TASK_RUNNING;
: + __set_current_state(TASK_RUNNING);
: spin_unlock_irqrestore(&cosa->lock, flags);
: mutex_unlock(&chan->rlock);
: return -ERESTARTSYS;
: }
: }
: remove_wait_queue(&chan->rxwaitq, &wait);
: - current->state = TASK_RUNNING;
: + __set_current_state(TASK_RUNNING);
: kbuf = chan->rxdata;
: count = chan->rxsize;
: spin_unlock_irqrestore(&cosa->lock, flags);
: @@ -890,14 +890,14 @@ static ssize_t cosa_write(struct file *file,
: spin_lock_irqsave(&cosa->lock, flags);
: add_wait_queue(&chan->txwaitq, &wait);
: while (!chan->tx_status) {
: - current->state = TASK_INTERRUPTIBLE;
: + set_current_state(TASK_INTERRUPTIBLE);
: spin_unlock_irqrestore(&cosa->lock, flags);
: schedule();
: spin_lock_irqsave(&cosa->lock, flags);
: if (signal_pending(current) && chan->tx_status == 0) {
: chan->tx_status = 1;
: remove_wait_queue(&chan->txwaitq, &wait);
: - current->state = TASK_RUNNING;
: + __set_current_state(TASK_RUNNING);
: chan->tx_status = 1;
: spin_unlock_irqrestore(&cosa->lock, flags);
: up(&chan->wsem);
: @@ -905,7 +905,7 @@ static ssize_t cosa_write(struct file *file,
: }
: }
: remove_wait_queue(&chan->txwaitq, &wait);
: - current->state = TASK_RUNNING;
: + __set_current_state(TASK_RUNNING);
: up(&chan->wsem);
: spin_unlock_irqrestore(&cosa->lock, flags);
: kfree(kbuf);
: --
: 2.1.0
Hello. On 02/20/2015 09:12 PM, Fabian Frederick wrote: > Use helper functions to access current->state. > Direct assignments are prone to races and therefore buggy. > current->state = TASK_RUNNING is replaced by __set_current_state() You sometimes use __set_current_state() and sometimes set_current_state(). > Thanks to Peter Zijlstra for the exact definition of the problem. > Suggested-By: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Fabian Frederick <fabf@skynet.be> [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 20 February 2015 at 19:34 Sergei Shtylyov > <sergei.shtylyov@cogentembedded.com> wrote: > > > Hello. > > On 02/20/2015 09:12 PM, Fabian Frederick wrote: > > > Use helper functions to access current->state. > > Direct assignments are prone to races and therefore buggy. > > > current->state = TASK_RUNNING is replaced by __set_current_state() > > You sometimes use __set_current_state() and sometimes set_current_state(). Hello Sergei, Peter suggested to use __set_current_state() for TASK_RUNNING : http://marc.info/?l=linux-kernel&m=142442259719216&w=2 Regards, Fabian > > > Thanks to Peter Zijlstra for the exact definition of the problem. > > > Suggested-By: Peter Zijlstra <peterz@infradead.org> > > Signed-off-by: Fabian Frederick <fabf@skynet.be> > > [...] > > WBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 20, 2015 at 09:34:28PM +0300, Sergei Shtylyov wrote: > Hello. > > On 02/20/2015 09:12 PM, Fabian Frederick wrote: > > >Use helper functions to access current->state. > >Direct assignments are prone to races and therefore buggy. > > >current->state = TASK_RUNNING is replaced by __set_current_state() > > You sometimes use __set_current_state() and sometimes set_current_state(). It depends on which state; setting yourself TASK_RUNNING is free of wakeup races -- you're already running after all, so it can safely use __set_current_state(). Setting a blocking state otoh needs set_current_state() which issues a full memory barriers with the store (critically in this case, effectively after the store) such that it orders the state store with a subsequent load in the condition check if it really needs to go to sleep. In full: current->state = TASK_UNINTERRUPTIBLE; wait = false; smp_mb(); smp_wmb(); if (wait) p->state = TASK_RUNNING; schedule(); Without that smp_mb(); the following order is possible: if (wait) wait = false; smp_wmb(); p->state = TASK_RUNNING; current->state = TASK_UNINTERRUPTIBLE; schedule(); And we'll wait forever more.. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2015 09:51 PM, Fabian Frederick wrote: >>> Use helper functions to access current->state. >>> Direct assignments are prone to races and therefore buggy. >>> current->state = TASK_RUNNING is replaced by __set_current_state() >> You sometimes use __set_current_state() and sometimes set_current_state(). > Hello Sergei, > Peter suggested to use __set_current_state() for TASK_RUNNING : > http://marc.info/?l=linux-kernel&m=142442259719216&w=2 I didn't even question your decisions, I (like Peter) just wanted a more coherent change-log. Thanks to Peter for the explanations though. :-) > Regards, > Fabian >>> Thanks to Peter Zijlstra for the exact definition of the problem. >>> Suggested-By: Peter Zijlstra <peterz@infradead.org> >>> Signed-off-by: Fabian Frederick <fabf@skynet.be> >> [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 20 February 2015 at 19:58 Peter Zijlstra <peterz@infradead.org> wrote: > > > On Fri, Feb 20, 2015 at 09:34:28PM +0300, Sergei Shtylyov wrote: > > Hello. > > > > On 02/20/2015 09:12 PM, Fabian Frederick wrote: > > > > >Use helper functions to access current->state. > > >Direct assignments are prone to races and therefore buggy. > > > > >current->state = TASK_RUNNING is replaced by __set_current_state() > > > > You sometimes use __set_current_state() and sometimes > >set_current_state(). > > It depends on which state; setting yourself TASK_RUNNING is free of > wakeup races -- you're already running after all, so it can safely use > __set_current_state(). > > Setting a blocking state otoh needs set_current_state() which issues a > full memory barriers with the store (critically in this case, > effectively after the store) such that it orders the state store with a > subsequent load in the condition check if it really needs to go to > sleep. > > > In full: > > current->state = TASK_UNINTERRUPTIBLE; wait = false; > smp_mb(); smp_wmb(); > if (wait) p->state = TASK_RUNNING; > schedule(); > > Without that smp_mb(); the following order is possible: > > if (wait) > wait = false; > smp_wmb(); > p->state = TASK_RUNNING; > current->state = TASK_UNINTERRUPTIBLE; > schedule(); > > And we'll wait forever more.. Do I have to add more comments in changelogs or is it OK for you ? Maybe something like: " current->state = TASK_RUNNING can safely be converted to __set_current_state() as we're already in that state. Other assignments are converted to set_current_state() (which uses set_mb()). " Regards, Fabian > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Fabian Frederick <fabf@skynet.be> Date: Fri, 20 Feb 2015 19:12:56 +0100 > Use helper functions to access current->state. > Direct assignments are prone to races and therefore buggy. > > current->state = TASK_RUNNING is replaced by __set_current_state() > > Thanks to Peter Zijlstra for the exact definition of the problem. > > Suggested-By: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Fabian Frederick <fabf@skynet.be> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wan/cosa.c b/drivers/net/wan/cosa.c index 83c39e2..88d121d 100644 --- a/drivers/net/wan/cosa.c +++ b/drivers/net/wan/cosa.c @@ -806,21 +806,21 @@ static ssize_t cosa_read(struct file *file, spin_lock_irqsave(&cosa->lock, flags); add_wait_queue(&chan->rxwaitq, &wait); while (!chan->rx_status) { - current->state = TASK_INTERRUPTIBLE; + set_current_state(TASK_INTERRUPTIBLE); spin_unlock_irqrestore(&cosa->lock, flags); schedule(); spin_lock_irqsave(&cosa->lock, flags); if (signal_pending(current) && chan->rx_status == 0) { chan->rx_status = 1; remove_wait_queue(&chan->rxwaitq, &wait); - current->state = TASK_RUNNING; + __set_current_state(TASK_RUNNING); spin_unlock_irqrestore(&cosa->lock, flags); mutex_unlock(&chan->rlock); return -ERESTARTSYS; } } remove_wait_queue(&chan->rxwaitq, &wait); - current->state = TASK_RUNNING; + __set_current_state(TASK_RUNNING); kbuf = chan->rxdata; count = chan->rxsize; spin_unlock_irqrestore(&cosa->lock, flags); @@ -890,14 +890,14 @@ static ssize_t cosa_write(struct file *file, spin_lock_irqsave(&cosa->lock, flags); add_wait_queue(&chan->txwaitq, &wait); while (!chan->tx_status) { - current->state = TASK_INTERRUPTIBLE; + set_current_state(TASK_INTERRUPTIBLE); spin_unlock_irqrestore(&cosa->lock, flags); schedule(); spin_lock_irqsave(&cosa->lock, flags); if (signal_pending(current) && chan->tx_status == 0) { chan->tx_status = 1; remove_wait_queue(&chan->txwaitq, &wait); - current->state = TASK_RUNNING; + __set_current_state(TASK_RUNNING); chan->tx_status = 1; spin_unlock_irqrestore(&cosa->lock, flags); up(&chan->wsem); @@ -905,7 +905,7 @@ static ssize_t cosa_write(struct file *file, } } remove_wait_queue(&chan->txwaitq, &wait); - current->state = TASK_RUNNING; + __set_current_state(TASK_RUNNING); up(&chan->wsem); spin_unlock_irqrestore(&cosa->lock, flags); kfree(kbuf);
Use helper functions to access current->state. Direct assignments are prone to races and therefore buggy. current->state = TASK_RUNNING is replaced by __set_current_state() Thanks to Peter Zijlstra for the exact definition of the problem. Suggested-By: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Fabian Frederick <fabf@skynet.be> --- drivers/net/wan/cosa.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)