Skip to contentSkip to conversation

aio_poll in 2.6?

PATCH John Myers 3 participants 7 messages May 17, 2003 – May 19, 2003 1 message not in the archive

Subsystems (2)
VFS Aio
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

John Myers

author
#1 lore raw

It's basically waiting for someone to merge the patch. There were some people making unsubstantiated claims that it didn't scale, but the available benchmarks showed that it scaled perfectly across the parameters tested.

linux-aiopoll/fs/Makefile renamed from fs/Makefile +2 -1
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./fs/Makefile ../linux-aiopoll/fs/Makefile
@@ -10,7 +10,8 @@
 		namei.o fcntl.o ioctl.o readdir.o select.o fifo.o locks.o \
 		dcache.o inode.o attr.o bad_inode.o file.o dnotify.o \
 		filesystems.o namespace.o seq_file.o xattr.o libfs.o \
-		fs-writeback.o mpage.o direct-io.o aio.o eventpoll.o
+		fs-writeback.o mpage.o direct-io.o aio.o eventpoll.o \
+		aiopoll.o
 
 obj-$(CONFIG_COMPAT) += compat.o
linux-aiopoll/fs/aio.c renamed from fs/aio.c +18 -0
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./fs/aio.c ../linux-aiopoll/fs/aio.c
@@ -58,6 +58,8 @@
 
 static void aio_kick_handler(void *);
 
+int async_poll(struct kiocb *iocb, int events);
+
 /* aio_setup
  *	Creates the slab caches used by the aio routines, panic on
  *	failure as this is done early during the boot sequence.

@@ -984,6 +986,19 @@
 	return -EINVAL;
 }
 
+ssize_t generic_aio_poll(struct file *file, struct kiocb *req, struct iocb *iocb)
+{
+	unsigned events = iocb->aio_buf;
+
+	/* Did the user set any bits they weren't supposed to? (The 
+	 * above is actually a cast.
+	 */
+	if (unlikely(events != iocb->aio_buf))
+		return -EINVAL;
+	
+	return async_poll(req, events);
+}
+
 int FASTCALL(io_submit_one(struct kioctx *ctx, struct iocb *user_iocb,
 				  struct iocb *iocb));
 int io_submit_one(struct kioctx *ctx, struct iocb *user_iocb,

@@ -1070,6 +1085,9 @@
 		if (file->f_op->aio_fsync)
 			ret = file->f_op->aio_fsync(req, 0);
 		break;
+ 	case IOCB_CMD_POLL:
+ 		ret = generic_aio_poll(file, req, iocb);
+ 		break;
 	default:
 		dprintk("EINVAL: io_submit: no operation provided\n");
 		ret = -EINVAL;
linux-aiopoll/fs/aiopoll.c renamed from fs/aiopoll.c +175 -0

175 lines not on this page · raw message

linux-aiopoll/fs/select.c renamed from fs/select.c +0 -1
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./fs/select.c ../linux-aiopoll/fs/select.c
@@ -24,7 +24,6 @@
 #include <asm/uaccess.h>
 
 #define ROUND_UP(x,y) (((x)+(y)-1)/(y))
-#define DEFAULT_POLLMASK (POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM)
 
 struct poll_table_entry {
 	struct file * filp;
linux-aiopoll/include/linux/aio_abi.h renamed from include/linux/aio_abi.h +1 -1
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./include/linux/aio_abi.h ../linux-aiopoll/include/linux/aio_abi.h
@@ -38,8 +38,8 @@
 	IOCB_CMD_FDSYNC = 3,
 	/* These two are experimental.
 	 * IOCB_CMD_PREADX = 4,
-	 * IOCB_CMD_POLL = 5,
 	 */
+	IOCB_CMD_POLL = 5,
 	IOCB_CMD_NOOP = 6,
 };
linux-aiopoll/include/linux/poll.h renamed from include/linux/poll.h +2 -0
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./include/linux/poll.h ../linux-aiopoll/include/linux/poll.h
@@ -10,6 +10,8 @@
 #include <linux/mm.h>
 #include <asm/uaccess.h>
 
+#define DEFAULT_POLLMASK (POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM)
+
 struct poll_table_struct;
 
 /*
#2 lore raw
Quoted 5 lines · John Myers

It's basically waiting for someone to merge the patch. There were some people making unsubstantiated claims that it didn't scale, but the available benchmarks showed that it scaled perfectly across the parameters tested.

What is the testing status of this?

Have any real-life applications been converted?

Does it require libaio changes to test?

linux-aiopoll/fs/aio.c line 62

Quoted 3 lines
linux-aiopoll/fs/aio.c renamed from fs/aio.c +2 -0
+int async_poll(struct kiocb *iocb, int events);
+

growl. Please don't put function prototypes in .c files. It defeats typechecking. I've moved this to aio.h.

Quoted 10 lines
linux-aiopoll/fs/aiopoll.c renamed from fs/aiopoll.c +10 -0
+int async_poll(struct kiocb *iocb, int events)
+{
+	unsigned int mask;
+	struct async_poll_iocb *apiocb = kiocb_to_apiocb(iocb);
+
+	/* Fast path */
+	if (iocb->ki_filp->f_op && iocb->ki_filp->f_op->poll) {
+		mask = iocb->ki_filp->f_op->poll(iocb->ki_filp, NULL);
+		mask &= events | POLLERR | POLLHUP;
+		if (mask & events)

linux-aiopoll/fs/aiopoll.c line 142

Quoted 10 lines
linux-aiopoll/fs/aiopoll.c renamed from fs/aiopoll.c +10 -0
+			return events;
+	}
+
+	init_poll_funcptr(&apiocb->pt, async_poll_queue_proc);
+	apiocb->armed = &apiocb;
+	apiocb->outofmem = 0;
+	apiocb->events = events;
+	apiocb->ehead = NULL;
+
+	iocb->ki_users++;

There is no locking around this modification of ->ki_users. Is this correct?

linux-aiopoll/fs/aiopoll.c line 143

Quoted 1 line
linux-aiopoll/fs/aiopoll.c renamed from fs/aiopoll.c +1 -0
+	wmb();

Barriers always need comments explaining why they are there.

linux-aiopoll/fs/aiopoll.c line 156

Quoted 13 lines
linux-aiopoll/fs/aiopoll.c renamed from fs/aiopoll.c +13 -0
+
+	mask = DEFAULT_POLLMASK;
+	if (iocb->ki_filp->f_op && iocb->ki_filp->f_op->poll)
+		mask = iocb->ki_filp->f_op->poll(iocb->ki_filp, &apiocb->pt);
+	mask &= events | POLLERR | POLLHUP;
+ 	if (mask && xchg(&apiocb->armed, NULL)) {
+		async_poll_freewait(apiocb, NULL);
+		aio_complete(iocb, mask, 0);
+	}
+	if (unlikely(apiocb->outofmem) && xchg(&apiocb->armed, NULL)) {
+		async_poll_freewait(apiocb, NULL);
+		aio_put_req(iocb);
+		aio_put_req(iocb);

Is the double-put intentional?

author
#3 lore raw

Re: [PATCH] Re: aio_poll in 2.6

Quoted 1 line · Andrew Morton

What is the testing status of this?

I've beaten on it with my multithreaded test program on a 2-way Pentium II box.

Quoted 1 line

Have any real-life applications been converted?

Not that I'm aware of. My test program uses a real-life async i/o thread pool framework which I've converted to Linux aio, but that framework also depends on IOCB_CMD_NOOP (which Ben LaHaise has been sitting on for months).

Note that aio poll is primarily for programs which either have a thread pool architecture or which need to use the aio framework for other operations. Single threaded straight-networking applications would be better served by epoll.

Quoted 1 line

Does it require libaio changes to test?

Don't think so--one could create an IO_CMD_POLL iocb by hand. I just found it easier to change libaio.

Quoted 2 lines

growl. Please don't put function prototypes in .c files. It defeats typechecking. I've moved this to aio.h.

That's how the patch was before I got my hands on it. I would have moved it to poll.h, but whatever.

Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	iocb->ki_users++;

There is no locking around this modification of ->ki_users. Is this correct?

Yes. This is the submission path. No other thread can access that iocb until after the wmb().

Quoted 1 line

Barriers always need comments explaining why they are there.

I looked through LXR to find similar wmb() calls to mimic, but didn't find any non-trivially commented calls. I hope the comment I added is acceptable.

Quoted 6 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+	if (unlikely(apiocb->outofmem) && xchg(&apiocb->armed, NULL)) {
+		async_poll_freewait(apiocb, NULL);
+		aio_put_req(iocb);
+		aio_put_req(iocb);

Is the double-put intentional?

Yes it was intentional and yes it is a bug. One frees the reference held by the async_poll() call (obtained before the wmb() call) as protection against submit/complete and submit/cancel races. The other frees the reference held by the poll operation itself (obtained in aio_get_req()), but when the routine returns -ENOMEM that causes io_submit_one() to call aio_complete() which again frees that reference.

Corrected patch follows.

linux-aiopoll/fs/Makefile renamed from fs/Makefile +2 -1
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./fs/Makefile ../linux-aiopoll/fs/Makefile
@@ -10,7 +10,8 @@
 		namei.o fcntl.o ioctl.o readdir.o select.o fifo.o locks.o \
 		dcache.o inode.o attr.o bad_inode.o file.o dnotify.o \
 		filesystems.o namespace.o seq_file.o xattr.o libfs.o \
-		fs-writeback.o mpage.o direct-io.o aio.o eventpoll.o
+		fs-writeback.o mpage.o direct-io.o aio.o eventpoll.o \
+		aiopoll.o
 
 obj-$(CONFIG_COMPAT) += compat.o
linux-aiopoll/fs/aio.c renamed from fs/aio.c +16 -0
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./fs/aio.c ../linux-aiopoll/fs/aio.c
@@ -984,6 +984,19 @@
 	return -EINVAL;
 }
 
+ssize_t generic_aio_poll(struct file *file, struct kiocb *req, struct iocb *iocb)
+{
+	unsigned events = iocb->aio_buf;
+
+	/* Did the user set any bits they weren't supposed to? (The 
+	 * above is actually a cast.
+	 */
+	if (unlikely(events != iocb->aio_buf))
+		return -EINVAL;
+	
+	return async_poll(req, events);
+}
+
 int FASTCALL(io_submit_one(struct kioctx *ctx, struct iocb *user_iocb,
 				  struct iocb *iocb));
 int io_submit_one(struct kioctx *ctx, struct iocb *user_iocb,

@@ -1070,6 +1083,9 @@
 		if (file->f_op->aio_fsync)
 			ret = file->f_op->aio_fsync(req, 0);
 		break;
+ 	case IOCB_CMD_POLL:
+ 		ret = generic_aio_poll(file, req, iocb);
+ 		break;
 	default:
 		dprintk("EINVAL: io_submit: no operation provided\n");
 		ret = -EINVAL;
linux-aiopoll/fs/aiopoll.c renamed from aiopoll.c +178 -0

178 lines not on this page · raw message

linux-aiopoll/fs/select.c renamed from fs/select.c +0 -1
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./fs/select.c ../linux-aiopoll/fs/select.c
@@ -24,7 +24,6 @@
 #include <asm/uaccess.h>
 
 #define ROUND_UP(x,y) (((x)+(y)-1)/(y))
-#define DEFAULT_POLLMASK (POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM)
 
 struct poll_table_entry {
 	struct file * filp;
linux-aiopoll/include/linux/aio.h renamed from include/linux/aio.h +2 -0
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./include/linux/aio.h ../linux-aiopoll/include/linux/aio.h
@@ -166,4 +166,6 @@
 /* for sysctl: */
 extern unsigned aio_max_nr, aio_max_size, aio_max_pinned;
 
+extern int async_poll(struct kiocb *iocb, int events);
+
 #endif /* __LINUX__AIO_H */
linux-aiopoll/include/linux/aio_abi.h renamed from include/linux/aio_abi.h +1 -1
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./include/linux/aio_abi.h ../linux-aiopoll/include/linux/aio_abi.h
@@ -38,8 +38,8 @@
 	IOCB_CMD_FDSYNC = 3,
 	/* These two are experimental.
 	 * IOCB_CMD_PREADX = 4,
-	 * IOCB_CMD_POLL = 5,
 	 */
+	IOCB_CMD_POLL = 5,
 	IOCB_CMD_NOOP = 6,
 };
linux-aiopoll/include/linux/poll.h renamed from include/linux/poll.h +2 -0
diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet ./include/linux/poll.h ../linux-aiopoll/include/linux/poll.h
@@ -10,6 +10,8 @@
 #include <linux/mm.h>
 #include <asm/uaccess.h>
 
+#define DEFAULT_POLLMASK (POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM)
+
 struct poll_table_struct;
 
 /*
#4 lore raw

Re: [PATCH] Re: aio_poll in 2.6

Quoted 5 lines · John Myers
Quoted 1 line · Andrew Morton

What is the testing status of this?

I've beaten on it with my multithreaded test program on a 2-way Pentium II box.

Can someone beat this code up on an SMP PPC/PPC64 box - that would show up far more than x86 does

#5 lore raw

Re: [PATCH] Re: aio_poll in 2.6

Quoted 11 lines · Alan Cox
Quoted 5 lines · John Myers
Quoted 1 line · Andrew Morton

What is the testing status of this?

I've beaten on it with my multithreaded test program on a 2-way Pentium II box.

Can someone beat this code up on an SMP PPC/PPC64 box - that would show up far more than x86 does

I can do that, but would need the test app...

author
#6 lore raw

Re: [PATCH] Re: aio_poll in 2.6

Quoted 3 lines · Andrew Morton

I can do that, but would need the test app...

Unfortunately, the thread pool framework the test app uses is proprietary.

#7 lore raw

Re: [PATCH] Re: aio_poll in 2.6

Quoted 8 lines · [email protected] (John Myers) wrote:
Quoted 3 lines · Andrew Morton

I can do that, but would need the test app...

Unfortunately, the thread pool framework the test app uses is proprietary.

That is a problem.

Generally, adding new code in codepaths which lots of apps use is low-risk because problems will be found quickly.

But from a stabilising-the-kernel viewpoint, adding new code which does not require elevated privileges, and which is not used by any current applications and which doesn't even have a decent test suite is a big worry.

There is a little testcase in
http://www.kvack.org/~bcrl/aio/aio-userspace-18a.tar.gz but it is singly threaded.