Skip to contentSkip to conversation

aio: use cmpxchg in aio_read_evt() instead of aio_ring_info->ring_lock

PATCH In review Nikanth Karthikesan 2 participants 3 messages Jun 26, 2008 – Jul 1, 2008

Subsystems (2)
VFS Aio
lore
Share & tools
lore

Nikanth Karthikesan

author
#0 lore raw

PATCH 1/1 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]>

---

fs/aio.c +2 -4
index 27b7181..09e216f 100644
@@ -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);

include/linux/aio.h +0 -1
index b51ddd2..c204290 100644
@@ -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;
#1 lore raw
Quoted 1 line · Nikanth Karthikesan

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

author
#2 lore raw
Quoted 7 lines · On Monday 30 June 2008 23:33:52 Zach Brown wrote:
Quoted 1 line · Nikanth Karthikesan

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