Re: [patch] convert aio event reap to use atomic-op instead of spin_lock

["Ken Chen" <kenchen@google.com>]

On 4/11/07, Zach Brown <zach.brown@oracle.com> wrote:
> First, I'll NAK this and all AIO patches until the patch description
> says that it's been run through the regression tests that we've started
> collecting in autotest.  They're trivial to run, never fear:

OK.  I will run those regression tests.


> > Resurrect an old patch that uses atomic operation to update ring buffer
> > index on AIO event queue.  This work allows futher application/libaio
> > optimization to run fast path io_getevents in user space.
>
> I have mixed feelings.  I think the userspace getevents support was
> poorly designed and the simple fact that we've gone this long without it
> says just how desperately the feature isn't needed.

I kept on getting requests from application developers who want that
feature.  My initial patch was dated back May 2004.


> We're allocating more ring elements
> than userspace asked for and than were accounted for in aio_max_nr.
> That cost, however teeny, will be measured against the benefit.

I noticed that while writing the patch.  We have the same bug right
now that nr_events is enlarged to consume the entire mmap'ed ring
buffer pages.  But yet, we don't account those in aio_max_nr.  I
didn't fix that up in this patch because I was going to do a separate
patch on that.


> > -     /* Compensate for the ring buffer's head/tail overlap entry */
> > -     nr_events += 2; /* 1 is required, 2 for good luck */
> > +     /* round nr_event to next power of 2 */
> > +     nr_events = roundup_pow_of_two(nr_events);
>
> Is that buggy?  How will the code tell if head == tail means a full ring
> or an empty ring?  The old code added that extra event to let it tell
> the ring was full before head == tail and it would think it's empty
> again, I think.  I'm guessing roundup(nr_events + 1) is needed.

I don't think it's a bug.  akpm taught me the trick: we don't wrap the
index around the ring size in this scheme, so the extra space is not
needed.


> The ring page, which is mmaped to userspace at some weird address, was
> just written through a kmap.  Do we need a flush_dcache_page()?  This
> isn't this patch's problem, but it'd need to be addressed if we're using
> the ring from userspace.

I will look into this aside from this patch.

- Ken