On Fri, 2012-11-02 at 18:35 +0100, Frederic Weisbecker wrote: > The IRQ_WORK_BUSY flag is set right before we execute the > work. Once this flag value is set, the work enters a > claimable state again. > > So if we have specific data to compute in our work, we ensure it's > either handled by another CPU or locally by enqueuing the work again. > This state machine is guanranteed by atomic operations on the flags. > > So when we set IRQ_WORK_BUSY without using an xchg-like operation, > we break this guarantee as in the following summarized scenario: > > CPU 1 CPU 2 > ----- ----- > (flags = 0) > old_flags = flags; > (flags = 0) > cmpxchg(flags, old_flags, > old_flags | IRQ_WORK_FLAGS) > (flags = 3) > [...] > flags = IRQ_WORK_BUSY > (flags = 2) > func() > (sees flags = 3) > cmpxchg(flags, old_flags, > old_flags | > IRQ_WORK_FLAGS) > (give up) > > cmpxchg(flags, 2, 0); > (flags = 0) > > CPU 1 claims a work and executes it, so it sets IRQ_WORK_BUSY and > the work is again in a claimable state. Now CPU 2 has new data to process > and try to claim that work but it may see a stale value of the flags > and think the work is still pending somewhere that will handle our data. > This is because CPU 1 doesn't set IRQ_WORK_BUSY atomically. > > As a result, the data expected to be handle by CPU 2 won't get handled. > > To fix this, use xchg() to set IRQ_WORK_BUSY, this way we ensure the CPU 2 > will see the correct value with cmpxchg() using the expected ordering. > > Changelog-heavily-inspired-by: Steven Rostedt <rost...@goodmis.org> > Signed-off-by: Frederic Weisbecker <fweis...@gmail.com> > Cc: Peter Zijlstra <pet...@infradead.org> > Cc: Ingo Molnar <mi...@kernel.org> > Cc: Thomas Gleixner <t...@linutronix.de> > Cc: Andrew Morton <a...@linux-foundation.org>
Acked-by: Steven Rostedt <rost...@goodmis.org> -- Steve > Cc: Paul Gortmaker <paul.gortma...@windriver.com> > Cc: Anish Kumar <anish198519851...@gmail.com> > --- > kernel/irq_work.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/kernel/irq_work.c b/kernel/irq_work.c > index 1588e3b..57be1a6 100644 > --- a/kernel/irq_work.c > +++ b/kernel/irq_work.c > @@ -119,8 +119,11 @@ void irq_work_run(void) > /* > * Clear the PENDING bit, after this point the @work > * can be re-used. > + * Make it immediately visible so that other CPUs trying > + * to claim that work don't rely on us to handle their data > + * while we are in the middle of the func. > */ > - work->flags = IRQ_WORK_BUSY; > + xchg(&work->flags, IRQ_WORK_BUSY); > work->func(work); > /* > * Clear the BUSY bit and return to the free state if -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/