Re: [PATCH 5/5] aio: Refactor aio_read_evt, use cmxchg(), fix bug

[Kent Overstreet]

On Tue, Oct 09, 2012 at 11:37:53AM -0700, Zach Brown wrote:
> On Mon, Oct 08, 2012 at 11:39:20PM -0700, Kent Overstreet wrote:
> > Bunch of cleanup
> 
> Ugh.  That's way too much noisy change for one patch with no
> description.  Break it up into functional pieces and actually describe
> them.

Heh, I actually didn't mean to send these patches out to lkml just yet
(though now I'm glad I did). There definitely is too much in this patch,
it kinda snowballed.

> > events off the ringbuffer without racing with io_getevents().
> 
> Are you sure this is safe in the presence of wrapping indices?  It's
> been a very long time since I've looked at this, but I could never
> convince myself that it was safe.
> 
> What I'm worried about is cmpxchg()s caller sampling, say, and index of
> 0, having another task GO NUTS and wrap that index all the way around
> back to 0, and then having that initial cmpchg() caller see the wrapped
> 0 index and think that's nothing's changed in the interim.
> 
> Is this a problem?

Ohhhh, yeah it is - ABA. That's easily solved though, we do the head %
nr when we're using the head pointer, and let the stored value use the
full 32 bits.

If libaio is the only thing in userspace looking at the ringbuffer, and
if I'm looking at the latest libaio code this shouldn't break
anything...

> (I wish I could find the comment I wrote in a very old patch to tear out
> the mapped ring entirely.. It was a great list of its design mistakes.)

Sticking the head and tail pointers on the same cacheline is one of my
main complaints. If you could find that comment I'd be interested in
reading it, though.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/