[patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Davide Libenzi]

This is just an example about how to add asyncfd support to the current
KAIO code.
Patch is small and comments says it all about my doubts.
I made a quick test program to verify the patch, and it runs fine here:

http://www.xmailserver.org/asyncfd-aio-test.c

The test program uses poll(2), but it'd, of course, work with epoll too.
This can allow to schedule both block I/O and other poll-able devices
requests, and wait for results using select/poll/epoll.



Signed-off-by: Davide Libenzi <davidel@xmailserver.org>



- Davide



Index: linux-2.6.20.ep2/fs/aio.c
===================================================================
--- linux-2.6.20.ep2.orig/fs/aio.c	2007-03-14 09:41:42.000000000 -0700
+++ linux-2.6.20.ep2/fs/aio.c	2007-03-14 10:31:53.000000000 -0700
@@ -30,6 +30,7 @@
 #include <linux/highmem.h>
 #include <linux/workqueue.h>
 #include <linux/security.h>
+#include <linux/asyncfd.h>
 
 #include <asm/kmap_types.h>
 #include <asm/uaccess.h>
@@ -422,6 +423,7 @@
 	req->private = NULL;
 	req->ki_iovec = NULL;
 	INIT_LIST_HEAD(&req->ki_run_list);
+	req->ki_asyncfd = ERR_PTR(-EINVAL);
 
 	/* Check if the completion queue has enough free space to
 	 * accept an event from this io.
@@ -463,6 +465,8 @@
 {
 	assert_spin_locked(&ctx->ctx_lock);
 
+	if (!IS_ERR(req->ki_asyncfd))
+		fput(req->ki_asyncfd);
 	if (req->ki_dtor)
 		req->ki_dtor(req);
 	if (req->ki_iovec != &req->ki_inline_vec)
@@ -947,6 +951,30 @@
 		return 1;
 	}
 
+	/*
+	 * Check if the user asked us to deliver the result through an
+	 * asyncfd. Note that asyncfd_add_results() may sleep. It seems
+	 * OK looking at the code, but I'm not sure since inside a USB driver,
+	 * aio_complete() is called with a spinlock held. !!CHECK
+	 */
+	if (unlikely(!IS_ERR(iocb->ki_asyncfd))) {
+		struct asyncfd_result asr;
+
+		asr.cookie = iocb->ki_user_data;
+		asr.obj = (unsigned long) iocb->ki_obj.user;
+		asr.res = res;
+		asr.res2 = res2;
+		if ((ret = asyncfd_add_results(iocb->ki_asyncfd, &asr, 1)) != 1) {
+			/*
+			 * Here I dunno what to do in case the userspace result
+			 * ring is full, or if -EFAULT is returned. !!CHECK
+			 */
+
+		}
+		spin_lock_irqsave(&ctx->ctx_lock, flags);
+		goto put_rq;
+	}
+
 	info = &ctx->ring_info;
 
 	/* add a completion event to the ring buffer.
@@ -1556,6 +1584,18 @@
 		fput(file);
 		return -EAGAIN;
 	}
+	if (iocb->aio_resfd != 0) {
+		/*
+		 * If the aio_resfd field of the iocb is not zero, get an
+		 * instance of the file* now. This will be the place to deliver
+		 * AIO results to.
+		 */
+		req->ki_asyncfd = asyncfd_fget((int) iocb->aio_resfd);
+		if (IS_ERR(req->ki_asyncfd)) {
+			ret = PTR_ERR(req->ki_asyncfd);
+			goto out_put_req;
+		}
+	}
 
 	req->ki_filp = file;
 	ret = put_user(req->ki_key, &user_iocb->aio_key);
Index: linux-2.6.20.ep2/include/linux/aio.h
===================================================================
--- linux-2.6.20.ep2.orig/include/linux/aio.h	2007-03-14 09:42:16.000000000 -0700
+++ linux-2.6.20.ep2/include/linux/aio.h	2007-03-14 10:17:24.000000000 -0700
@@ -119,6 +119,12 @@
 
 	struct list_head	ki_list;	/* the aio core uses this
 						 * for cancellation */
+
+	/*
+	 * If the aio_resfd field of the userspace iocb is not zero,
+	 * this is the underlying file* to deliver event to.
+	 */
+	struct file		*ki_asyncfd;
 };
 
 #define is_sync_kiocb(iocb)	((iocb)->ki_key == KIOCB_SYNC_KEY)
Index: linux-2.6.20.ep2/include/linux/aio_abi.h
===================================================================
--- linux-2.6.20.ep2.orig/include/linux/aio_abi.h	2007-03-14 09:42:51.000000000 -0700
+++ linux-2.6.20.ep2/include/linux/aio_abi.h	2007-03-14 09:46:11.000000000 -0700
@@ -84,7 +84,11 @@
 
 	/* extra parameters */
 	__u64	aio_reserved2;	/* TODO: use this for a (struct sigevent *) */
-	__u64	aio_reserved3;
+	__u32	aio_reserved3;
+	/*
+	 * If different from 0, this is an asyncfd to deliver AIO results to
+	 */
+	__u32	aio_resfd;
 }; /* 64 bytes */
 
 #undef IFBIG

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Benjamin LaHaise]

On Wed, Mar 14, 2007 at 03:19:21PM -0700, Davide Libenzi wrote:
> +	/*
> +	 * Check if the user asked us to deliver the result through an
> +	 * asyncfd. Note that asyncfd_add_results() may sleep. It seems
> +	 * OK looking at the code, but I'm not sure since inside a USB driver,
> +	 * aio_complete() is called with a spinlock held. !!CHECK
> +	 */

That won't work.  aio_complete() is supposed to be irq safe.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Davide Libenzi]

On Wed, 14 Mar 2007, Benjamin LaHaise wrote:

> On Wed, Mar 14, 2007 at 03:19:21PM -0700, Davide Libenzi wrote:
> > +	/*
> > +	 * Check if the user asked us to deliver the result through an
> > +	 * asyncfd. Note that asyncfd_add_results() may sleep. It seems
> > +	 * OK looking at the code, but I'm not sure since inside a USB driver,
> > +	 * aio_complete() is called with a spinlock held. !!CHECK
> > +	 */
> 
> That won't work.  aio_complete() is supposed to be irq safe.

Can you point me to a kernel path that ends up calling aio_complete() in a 
do-not-sleep mode?
The offender I see is drivers/usb/gadget/inode.c that calls it with a 
spinlock held.
The aio_run_iocb function seem to release/reacquire the lock before 
calling aio_complete().



- Davide

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Davide Libenzi]

On Wed, 14 Mar 2007, Benjamin LaHaise wrote:

> On Wed, Mar 14, 2007 at 04:24:54PM -0700, Davide Libenzi wrote:
> > Can you point me to a kernel path that ends up calling aio_complete() in a 
> > do-not-sleep mode?
> 
> If you remove that invariant, then it is very difficult for device drivers 
> and other code to make use of aio_complete().
> 
> > The offender I see is drivers/usb/gadget/inode.c that calls it with a 
> > spinlock held.
> 
> Which was from irq context last time I checked.
> 
> > The aio_run_iocb function seem to release/reacquire the lock before 
> > calling aio_complete().
> 
> That implies nothing -- aio_complete() has to acquire ctx_lock and cannot 
> be called holding the lock.  Sure, it could probably be split into 
> __aio_complete() and have aio_complete() wrap it acquiring the lock.

Yeah, of course. I do not plan revolutions. Just asking if it's a possible 
thing to do. I can mlock the userspace ring, if imposing that burden over 
aio_complete() is seen as too heavy.



- Davide

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Benjamin LaHaise]

On Wed, Mar 14, 2007 at 04:24:54PM -0700, Davide Libenzi wrote:
> Can you point me to a kernel path that ends up calling aio_complete() in a 
> do-not-sleep mode?

If you remove that invariant, then it is very difficult for device drivers 
and other code to make use of aio_complete().

> The offender I see is drivers/usb/gadget/inode.c that calls it with a 
> spinlock held.

Which was from irq context last time I checked.

> The aio_run_iocb function seem to release/reacquire the lock before 
> calling aio_complete().

That implies nothing -- aio_complete() has to acquire ctx_lock and cannot 
be called holding the lock.  Sure, it could probably be split into 
__aio_complete() and have aio_complete() wrap it acquiring the lock.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Davide Libenzi]

On Wed, 14 Mar 2007, Davide Libenzi wrote:

> On Wed, 14 Mar 2007, Benjamin LaHaise wrote:
> 
> > On Wed, Mar 14, 2007 at 04:24:54PM -0700, Davide Libenzi wrote:
> > > Can you point me to a kernel path that ends up calling aio_complete() in a 
> > > do-not-sleep mode?
> > 
> > If you remove that invariant, then it is very difficult for device drivers 
> > and other code to make use of aio_complete().
> > 
> > > The offender I see is drivers/usb/gadget/inode.c that calls it with a 
> > > spinlock held.
> > 
> > Which was from irq context last time I checked.

The drivers/usb/gadget/inode.c case seems to be easily fixeable AFAICS, in 
the ep_aio_complete() function.
I was more under the impression that aio_complete() was more of a tasklet 
kind of domain.



- Davide

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Benjamin LaHaise]

On Wed, Mar 14, 2007 at 04:41:58PM -0700, Davide Libenzi wrote:
> Yeah, of course. I do not plan revolutions. Just asking if it's a possible 
> thing to do. I can mlock the userspace ring, if imposing that burden over 
> aio_complete() is seen as too heavy.

I'm not sure I follow what you're doing -- why isn't asyncfd merely calling 
io_getevents() instead of reinventing everything the ringbuffer does?  The 
aio ringbuffer is already locked in memory.  Fwiw, the aio ringbuffer was 
originally wired up to a file descriptor, but that gave way to the actual 
syscall in order to enforce proper typechecking and typical usage scenarios 
with timeouts.

Also, there have been patches floating around for aio_poll and a way to get 
epoll wakeups into the aio event queue.  They deserve serious consideration 
if this asyncfd seems necessary.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Davide Libenzi]

On Wed, 14 Mar 2007, Benjamin LaHaise wrote:

> On Wed, Mar 14, 2007 at 04:41:58PM -0700, Davide Libenzi wrote:
> > Yeah, of course. I do not plan revolutions. Just asking if it's a possible 
> > thing to do. I can mlock the userspace ring, if imposing that burden over 
> > aio_complete() is seen as too heavy.
> 
> I'm not sure I follow what you're doing -- why isn't asyncfd merely calling 
> io_getevents() instead of reinventing everything the ringbuffer does?  The 
> aio ringbuffer is already locked in memory.  Fwiw, the aio ringbuffer was 
> originally wired up to a file descriptor, but that gave way to the actual 
> syscall in order to enforce proper typechecking and typical usage scenarios 
> with timeouts.

The purpose of asyncfd is to provide a pollable (by the mean of 
f_op->poll) device that can be hosted inside a standard select/poll/epoll 
wait subsystem, and that, at the same time, provide a zero-copy way for 
kernel code (KAIO and syslets/threadlets were my thought) to deliver 
results to userspace.



> Also, there have been patches floating around for aio_poll and a way to get 
> epoll wakeups into the aio event queue.  They deserve serious consideration 
> if this asyncfd seems necessary.

I don't want to talk about the AIO poll code, because last time I saw it, 
it did not look shiny.
But I think we can agree that ppl needs to have a way to wait for both 
block I/O (covered by either KAIO or syslets/threadlets) and all the other 
world (covered by epoll). This has been pretty clear for me, looking at 
the continuous request I got to provide block I/O completions through 
epoll, and looking at the hackage that ppl has currently to do in 
userspace to achieve that.
Now that I'm seeing I can wait for both block and net I/O, I got excited ;)



- Davide

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Davide Libenzi]

On Wed, 14 Mar 2007, Linus Torvalds wrote:

> On Wed, 14 Mar 2007, Davide Libenzi wrote:
> > >
> > > That won't work.  aio_complete() is supposed to be irq safe.
> > 
> > Can you point me to a kernel path that ends up calling aio_complete() in a 
> > do-not-sleep mode?
> 
> All of them.
> 
> It's called from dio_bio_end_aio(), which is the bi_end_io function for an 
> AIO action. Which in turn is called at IO completion time. 
> 
> Which is basically _always_ interrupt context.
> 
> So you cannot sleep. It's not about holding spinlocks (which it might well 
> do as well). It's about a much more fundamental issue: you can only sleep 
> in process context, not from interrupts.

Ack! Gotcha. Sigh! :)



- Davide

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Linus Torvalds]

On Wed, 14 Mar 2007, Davide Libenzi wrote:
> >
> > That won't work.  aio_complete() is supposed to be irq safe.
> 
> Can you point me to a kernel path that ends up calling aio_complete() in a 
> do-not-sleep mode?

All of them.

It's called from dio_bio_end_aio(), which is the bi_end_io function for an 
AIO action. Which in turn is called at IO completion time. 

Which is basically _always_ interrupt context.

So you cannot sleep. It's not about holding spinlocks (which it might well 
do as well). It's about a much more fundamental issue: you can only sleep 
in process context, not from interrupts.

		Linus

Re: [patch 13/13] signalfd/timerfd/asyncfd v5 - KAIO asyncfd support (example/maybe-broken) ...

[Davide Libenzi]

On Wed, 14 Mar 2007, Davide Libenzi wrote:

> On Wed, 14 Mar 2007, Benjamin LaHaise wrote:
> 
> > On Wed, Mar 14, 2007 at 04:41:58PM -0700, Davide Libenzi wrote:
> > > Yeah, of course. I do not plan revolutions. Just asking if it's a possible 
> > > thing to do. I can mlock the userspace ring, if imposing that burden over 
> > > aio_complete() is seen as too heavy.
> > 
> > I'm not sure I follow what you're doing -- why isn't asyncfd merely calling 
> > io_getevents() instead of reinventing everything the ringbuffer does?  The 
> > aio ringbuffer is already locked in memory.  Fwiw, the aio ringbuffer was 
> > originally wired up to a file descriptor, but that gave way to the actual 
> > syscall in order to enforce proper typechecking and typical usage scenarios 
> > with timeouts.
> 
> The purpose of asyncfd is to provide a pollable (by the mean of 
> f_op->poll) device that can be hosted inside a standard select/poll/epoll 
> wait subsystem, and that, at the same time, provide a zero-copy way for 
> kernel code (KAIO and syslets/threadlets were my thought) to deliver 
> results to userspace.

But, yeah. It can end up calling io_getevents() instead of doing it's own 
thing. That'd make it even slimmer ;)



- Davide