On Fri, 2012-11-02 at 18:35 +0100, Frederic Weisbecker wrote: > Work claiming wants to be SMP-safe. > > And by the time we try to claim a work, if it is already executing > concurrently on another CPU, we want to succeed the claiming and queue > the work again because the other CPU may have missed the data we wanted > to handle in our work if it's about to complete there. > > This scenario is summarized below: > > CPU 1 CPU 2 > ----- ----- > (flags = 0) > cmpxchg(flags, 0, IRQ_WORK_FLAGS) > (flags = 3) > [...] > xchg(flags, IRQ_WORK_BUSY) > (flags = 2) > func() > if (flags & IRQ_WORK_PENDING) > (not true) > cmpxchg(flags, flags, > IRQ_WORK_FLAGS) > (flags = 3) > [...] > cmpxchg(flags, IRQ_WORK_BUSY, 0); > (fail, pending on CPU 2) > > This state machine is synchronized using [cmp]xchg() on the flags. > As such, the early IRQ_WORK_PENDING check in CPU 2 above is racy. > By the time we check it, we may be dealing with a stale value because > we aren't using an atomic accessor. As a result, CPU 2 may "see" > that the work is still pending on another CPU while it may be > actually completing the work function exection already, leaving > our data unprocessed. > > To fix this, we start by speculating about the value we wish to be > in the work->flags but we only make any conclusion after the value > returned by the cmpxchg() call that either claims the work or let > the current owner handle the pending work for us. > > Changelog-heavily-inspired-by: Steven Rostedt <[email protected]> > Signed-off-by: Frederic Weisbecker <[email protected]> > Cc: Peter Zijlstra <[email protected]> > Cc: Ingo Molnar <[email protected]> > Cc: Thomas Gleixner <[email protected]> > Cc: Andrew Morton <[email protected]>
Acked-by: Steven Rostedt <[email protected]> -- Steve > Cc: Paul Gortmaker <[email protected]> > Cc: Anish Kumar <[email protected]> > --- > kernel/irq_work.c | 16 +++++++++++----- > 1 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/kernel/irq_work.c b/kernel/irq_work.c > index 57be1a6..64eddd5 100644 > --- a/kernel/irq_work.c > +++ b/kernel/irq_work.c > @@ -34,15 +34,21 @@ static DEFINE_PER_CPU(struct llist_head, irq_work_list); > */ > static bool irq_work_claim(struct irq_work *work) > { > - unsigned long flags, nflags; > + unsigned long flags, oflags, nflags; > > + /* > + * Start with our best wish as a premise but only trust any > + * flag value after cmpxchg() result. > + */ > + flags = work->flags & ~IRQ_WORK_PENDING; > for (;;) { > - flags = work->flags; > - if (flags & IRQ_WORK_PENDING) > - return false; > nflags = flags | IRQ_WORK_FLAGS; > - if (cmpxchg(&work->flags, flags, nflags) == flags) > + oflags = cmpxchg(&work->flags, flags, nflags); > + if (oflags == flags) > break; > + if (oflags & IRQ_WORK_PENDING) > + return false; > + flags = oflags; > cpu_relax(); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
