[patch] aio: streamline read events after woken up

[patch] aio: streamline read events after woken up

[Chen, Kenneth W]

The read event loop in the blocking path is also inefficient. For every
event it reap (if not blocking), it does the following in a loop:

      while (i < nr) {
            prepare_to_wait_exclusive
            aio_read_evt
            finish_wait
            ...
      }

Given the previous patch "aio: add per task aio wait event condition"
that we properly wake up event waiting process knowing that we have
enough events to reap, it's just plain waste of time to insert itself
into a wait queue, and then immediately remove itself from the wait
queue for *every* event reap iteration.

This patch factors out the wait queue insertion/deletion out of the event
reap loop, streamlines the event reaping after the process wakes up.


Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>

--- ./fs/aio.c.orig	2006-12-24 17:04:52.000000000 -0800
+++ ./fs/aio.c	2006-12-24 17:05:10.000000000 -0800
@@ -1174,42 +1174,40 @@ retry:
 	}
 
 	aio_init_wait(&wait);
+wait:
+	prepare_to_wait_exclusive(&ctx->wait, &wait.wait, TASK_INTERRUPTIBLE);
+	ret = aio_read_evt(ctx, &ent);
+	if (!ret) {
+		wait.nr_wait = min_nr - i;
+		schedule();
+		if (signal_pending(tsk))
+			ret = -EINTR;
+	}
+	finish_wait(&ctx->wait, &wait.wait);
+
+	if (ret < 0)
+		goto out_cleanup;
+
 	while (likely(i < nr)) {
-		do {
-			prepare_to_wait_exclusive(&ctx->wait, &wait.wait,
-						  TASK_INTERRUPTIBLE);
-			ret = aio_read_evt(ctx, &ent);
-			if (ret)
-				break;
-			if (min_nr <= i)
-				break;
-			ret = 0;
-			if (to.timed_out)	/* Only check after read evt */
-				break;
-			wait.nr_wait = min_nr - i;
-			schedule();
-			if (signal_pending(tsk)) {
-				ret = -EINTR;
+		if (ret) {
+			if (unlikely(copy_to_user(event, &ent, sizeof(ent)))) {
+				dprintk("aio: lost an event due to EFAULT.\n");
+				ret = -EFAULT;
 				break;
 			}
-			/*ret = aio_read_evt(ctx, &ent);*/
-		} while (1) ;
-		finish_wait(&ctx->wait, &wait.wait);
-
-		if (unlikely(ret <= 0))
-			break;
+			event++;
+			i++;
+		}
 
-		ret = -EFAULT;
-		if (unlikely(copy_to_user(event, &ent, sizeof(ent)))) {
-			dprintk("aio: lost an event due to EFAULT.\n");
+		ret = aio_read_evt(ctx, &ent);
+		if (unlikely(!ret)) {
+			if (i < min_nr && !to.timed_out)
+				goto wait;
 			break;
 		}
-
-		/* Good, event copied to userland, update counts. */
-		event ++;
-		i ++;
 	}
 
+out_cleanup:
 	if (timeout)
 		clear_timeout(&to);
 out:

Re: [patch] aio: streamline read events after woken up

[Zach Brown]

> Given the previous patch "aio: add per task aio wait event condition"
> that we properly wake up event waiting process knowing that we have
> enough events to reap, it's just plain waste of time to insert itself
> into a wait queue, and then immediately remove itself from the wait
> queue for *every* event reap iteration.

Hmm, I dunno.  It seems like we're still left with a pretty silly loop.

Would it be reasonable to have a loop that copied multiple events at  
a time?  We could use some __copy_to_user_inatomic(), it didn't exist  
when this stuff was first written.

- z

RE: [patch] aio: streamline read events after woken up

[Chen, Kenneth W]

Zach Brown wrote on Tuesday, January 02, 2007 5:06 PM
> To: Chen, Kenneth W
> > Given the previous patch "aio: add per task aio wait event condition"
> > that we properly wake up event waiting process knowing that we have
> > enough events to reap, it's just plain waste of time to insert itself
> > into a wait queue, and then immediately remove itself from the wait
> > queue for *every* event reap iteration.
> 
> Hmm, I dunno.  It seems like we're still left with a pretty silly loop.
> 
> Would it be reasonable to have a loop that copied multiple events at  
> a time?  We could use some __copy_to_user_inatomic(), it didn't exist  
> when this stuff was first written.

It sounds reasonable, but I think it will be complicated because of
kmap_atomic on the ring buffer, along with tail wraps around ring
buffer index there.  By then, most of you would probably veto the
patch anyway ;-)

Re: [patch] aio: streamline read events after woken up

[Zach Brown]

> buffer index there.  By then, most of you would probably veto the
> patch anyway ;-)

haha, touche :)

I still think it'd be the right thing, though.  We can let the patch  
speak for itself :).

- z