On Mon, 2012-10-29 at 14:28 +0100, Frederic Weisbecker wrote: > Context requirements on irq work claim are not entirely > clear. But it appears that we can try to claim a work that > may be already claimed by another CPU. > > If so then the early check on IRQ_WORK_PENDING in > irq_work_claim() is racy because another CPU may be > changing the flags concurrently and we have nothing > to synchronize against that. So the value we deal with > may be stale for a while already. > > To fix this, start with our best wish as the initial > value for the work flags and feed cmpxchg with it. But > only do the check against IRQ_WORK_PENDING flag with the > cmpxchg result. > > Nonetheless, if the work is not pending but our best wish > was wrong, restart with the old value returned by cmpxchg. > > 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]> > Cc: Steven Rostedt <[email protected]> > Cc: Paul Gortmaker <[email protected]> > --- > kernel/irq_work.c | 17 ++++++++++++----- > 1 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/kernel/irq_work.c b/kernel/irq_work.c > index 1588e3b..679c13e 100644 > --- a/kernel/irq_work.c > +++ b/kernel/irq_work.c > @@ -34,15 +34,22 @@ 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; > > + /* > + * Can't check IRQ_WORK_PENDING bit right now because the work > + * can be running on another CPU and we are not sync with its > + * changes to work flags. Only cmpxchg can reliably check for us. > + */ > + flags = work->flags & ~IRQ_WORK_PENDING; > for (;;) { > - flags = work->flags;
I wonder if the bug is just a memory barrier missing here? But that also suggests that the other CPU used a memory barrier too (or cmpxchg() which implies one). But this change looks fine too. -- Steve > - 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/
