2008-06-26 11:07:42

by Nikanth Karthikesan

Subject: [PATCH] aio: use cmpxchg in aio_read_evt() instead of aio_ring_info->ring_lock

Use cmpxchg in aio_read_evt() and remove the aio_ring_info->ring_lock

Signed-off-by: Nikanth Karthikesan <[email protected]>

---

diff --git a/fs/aio.c b/fs/aio.c
index 27b7181..09e216f 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -256,7 +256,6 @@ static struct kioctx *ioctx_alloc(unsigned nr_events)

atomic_set(&ctx->users, 1);
spin_lock_init(&ctx->ctx_lock);
- spin_lock_init(&ctx->ring_info.ring_lock);
init_waitqueue_head(&ctx->wait);

INIT_LIST_HEAD(&ctx->active_reqs);
@@ -988,14 +987,13 @@ put_rq:
/* aio_read_evt
* Pull an event off of the ioctx's event ring. Returns the number of
* events fetched (0 or 1 ;-)
- * FIXME: make this use cmpxchg.
- * TODO: make the ringbuffer user mmap()able (requires FIXME).
+ * TODO: make the ringbuffer user mmap()able.
*/
static int aio_read_evt(struct kioctx *ioctx, struct io_event *ent)
{
struct aio_ring_info *info = &ioctx->ring_info;
struct aio_ring *ring;
- unsigned long head;
+ unsigned long head, old, prev;
int ret = 0;

ring = kmap_atomic(info->ring_pages[0], KM_USER0);
@@ -1006,19 +1004,23 @@ static int aio_read_evt(struct kioctx *ioctx, struct
io_event *ent)
if (ring->head == ring->tail)
goto out;

- spin_lock(&info->ring_lock);
+ do {
+ struct io_event *evp;
+ ret = 0;
+ old = ring->head;
+ head = old % info->nr;

- head = ring->head % info->nr;
- if (head != ring->tail) {
- struct io_event *evp = aio_ring_event(info, head, KM_USER1);
+ if (head == ring->tail)
+ goto out;
+
+ evp = aio_ring_event(info, head, KM_USER1);
*ent = *evp;
head = (head + 1) % info->nr;
- smp_mb(); /* finish reading the event before updatng the head */
- ring->head = head;
+ smp_mb(); /* finish reading the event before updating the head */
+ prev = cmpxchg(&ring->head, old, head);
ret = 1;
put_aio_ring_event(evp, KM_USER1);
- }
- spin_unlock(&info->ring_lock);
+ } while (prev != old);

out:
kunmap_atomic(ring, KM_USER0);
diff --git a/include/linux/aio.h b/include/linux/aio.h
index b51ddd2..c204290 100644
--- a/include/linux/aio.h
+++ b/include/linux/aio.h
@@ -169,7 +169,6 @@ struct aio_ring_info {
unsigned long mmap_size;

struct page **ring_pages;
- spinlock_t ring_lock;
long nr_pages;

unsigned nr, tail;

2008-06-30 18:04:19

by Zach Brown

Subject: Re: [PATCH] aio: use cmpxchg in aio_read_evt() instead of aio_ring_info->ring_lock

Nikanth Karthikesan wrote:
> Use cmpxchg in aio_read_evt() and remove the aio_ring_info->ring_lock

I don't think this is safe because the cmpxhg() can't differentiate the
0 it read at one point with a 0 that it reads in the future after other
racing threads have done enough work on the ring index to wrap it.

- z

2008-07-01 08:23:55

by Nikanth Karthikesan

Subject: Re: [PATCH] aio: use cmpxchg in aio_read_evt() instead of aio_ring_info->ring_lock

On Monday 30 June 2008 23:33:52 Zach Brown wrote:
> Nikanth Karthikesan wrote:
> > Use cmpxchg in aio_read_evt() and remove the aio_ring_info->ring_lock
>
> I don't think this is safe because the cmpxhg() can't differentiate the
> 0 it read at one point with a 0 that it reads in the future after other
> racing threads have done enough work on the ring index to wrap it.
>

Yes, this is unsafe. Thanks for the review.

Thanks
Nikanth Karthikesan