Messages in this thread | | | Subject | Re: [PATCH 2/2] irq_work: Fix racy IRQ_WORK_BUSY flag setting | From | Steven Rostedt <> | Date | Wed, 31 Oct 2012 08:59:50 -0400 |
| |
On Wed, 2012-10-31 at 20:04 +0900, anish kumar wrote:
> > /* > > * Claim the entry so that no one else will poke at it. > > */ > > static bool irq_work_claim(struct irq_work *work) > > { > > unsigned long flags, nflags; > > > > for (;;) { > > flags = work->flags; > > if (flags & IRQ_WORK_PENDING) > > return false; > > nflags = flags | IRQ_WORK_FLAGS; > nflags = 1 | 3 > nflags = 2 | 3 > In both cases the result would be same.If I am right then wouldn't this > operation be redundant?
Right. Actually we could change the new loop to:
for (;;) { oflags = cmpxchg(&work->flags, flags, IRQ_WORK_FLAGS); if (oflags == flags) break; if (oflags & IRQ_WORK_PENDING) return false; flags = oflags; cpu_relax(); }
> > if (cmpxchg(&work->flags, flags, nflags) == flags) > > break; > > cpu_relax(); > > } > > > > return true; > > } > >
> > > > This now does: > > > > CPU 1 CPU 2 > > ----- ----- > > (flags = 0) > > cmpxchg(flags, 0, IRQ_WORK_FLAGS) > > (flags = 3) > > [...] > > xchg(&flags, IRQ_WORK_BUSY) > > (flags = 2) > > func() > > oflags = cmpxchg(&flags, flags, nflags); > > (sees flags = 2) > > if (flags & IRQ_WORK_PENDING) > > (not true) > > (loop) > > cmpxchg(flags, 2, 0); > > (flags = 2) > > flags = 3 > > > > > > > > With both patch 1 and 2 you fixed the bug. > This is the best explanation anyone can put forward for this problem.
Frederic,
Would you like to add my explanation to your change log? You can add the entire thing, which I think would explain a lot to people.
Thanks,
-- Steve
| |