[PATCH] Poll microoptimizations.

[PATCH] Poll microoptimizations.

[Vadim Lobanov]

Patch to provide some microoptimizations for the poll() system call
implementation. The loop that traverses over the "struct pollfd" entries
was moved from do_pollfd() to its single caller do_poll(), so that
do_pollfd() no longer mucks around with the "count" and the "pt"
variables that should belong to do_poll() alone. This saves unnecessary
levels of indirection. Modifications were run tested.

Signed-off-by: Vadim Lobanov <vlobanov@speakeasy.net>

diff -Npru linux-2.6.17-rc1/fs/select.c linux-new/fs/select.c
--- linux-2.6.17-rc1/fs/select.c	2006-04-12 20:31:54.000000000 -0700
+++ linux-new/fs/select.c	2006-04-13 18:54:14.000000000 -0700
@@ -544,37 +544,30 @@ struct poll_list {

 #define POLLFD_PER_PAGE  ((PAGE_SIZE-sizeof(struct poll_list)) / sizeof(struct pollfd))

-static void do_pollfd(unsigned int num, struct pollfd * fdpage,
-	poll_table ** pwait, int *count)
+static int do_pollfd(struct pollfd * pollfd, poll_table * pwait)
 {
-	int i;
+	unsigned int mask;
+	int fd;

-	for (i = 0; i < num; i++) {
-		int fd;
-		unsigned int mask;
-		struct pollfd *fdp;
-
-		mask = 0;
-		fdp = fdpage+i;
-		fd = fdp->fd;
-		if (fd >= 0) {
-			int fput_needed;
-			struct file * file = fget_light(fd, &fput_needed);
-			mask = POLLNVAL;
-			if (file != NULL) {
-				mask = DEFAULT_POLLMASK;
-				if (file->f_op && file->f_op->poll)
-					mask = file->f_op->poll(file, *pwait);
-				mask &= fdp->events | POLLERR | POLLHUP;
-				fput_light(file, fput_needed);
-			}
-			if (mask) {
-				*pwait = NULL;
-				(*count)++;
-			}
+	mask = 0;
+	fd = pollfd->fd;
+	if (fd >= 0) {
+		int fput_needed;
+		struct file * file;
+
+		file = fget_light(fd, &fput_needed);
+		mask = POLLNVAL;
+		if (file != NULL) {
+			mask = DEFAULT_POLLMASK;
+			if (file->f_op && file->f_op->poll)
+				mask = file->f_op->poll(file, pwait);
+			mask &= pollfd->events | POLLERR | POLLHUP;
+			fput_light(file, fput_needed);
 		}
-		fdp->revents = mask;
 	}
+	pollfd->revents = mask;
+
+	return (mask != 0);
 }

 static int do_poll(unsigned int nfds,  struct poll_list *list,
@@ -592,10 +585,19 @@ static int do_poll(unsigned int nfds,  s
 		long __timeout;

 		set_current_state(TASK_INTERRUPTIBLE);
-		walk = list;
-		while(walk != NULL) {
-			do_pollfd( walk->len, walk->entries, &pt, &count);
-			walk = walk->next;
+		for (walk = list; walk != NULL; walk = walk->next) {
+			struct pollfd * pfd, * pfd_end;
+
+			pfd = walk->entries;
+			pfd_end = pfd + walk->len;
+			for (; pfd != pfd_end; pfd++) {
+				int ev;
+
+				ev = do_pollfd(pfd, pt);
+				count += ev;
+				ev--;
+				pt = (poll_table*)((unsigned long)pt & ev);
+			}
 		}
 		pt = NULL;
 		if (count || !*timeout || signal_pending(current))

Re: [PATCH] Poll microoptimizations.

[Vadim Lobanov]

On Thu, 13 Apr 2006, Vadim Lobanov wrote:

> Patch to provide some microoptimizations for the poll() system call
> implementation. The loop that traverses over the "struct pollfd" entries
> was moved from do_pollfd() to its single caller do_poll(), so that
> do_pollfd() no longer mucks around with the "count" and the "pt"
> variables that should belong to do_poll() alone. This saves unnecessary
> levels of indirection. Modifications were run tested.

As a further thought, the do_pollfd() function should now be small
enough that it is a candidate for inlining. Given the increased number
of calls to it, this makes sense, so we can just stick the inline
keyword in front of it. Updated patch attached.

Signed-off-by: Vadim Lobanov <vlobanov@speakeasy.net>

diff -Npru linux-2.6.17-rc1/fs/select.c linux-new/fs/select.c
--- linux-2.6.17-rc1/fs/select.c	2006-04-12 20:31:54.000000000 -0700
+++ linux-new/fs/select.c	2006-04-13 18:54:14.000000000 -0700
@@ -544,37 +544,30 @@ struct poll_list {

 #define POLLFD_PER_PAGE  ((PAGE_SIZE-sizeof(struct poll_list)) / sizeof(struct pollfd))

-static void do_pollfd(unsigned int num, struct pollfd * fdpage,
-	poll_table ** pwait, int *count)
+static inline int do_pollfd(struct pollfd * pollfd, poll_table * pwait)
 {
-	int i;
+	unsigned int mask;
+	int fd;

-	for (i = 0; i < num; i++) {
-		int fd;
-		unsigned int mask;
-		struct pollfd *fdp;
-
-		mask = 0;
-		fdp = fdpage+i;
-		fd = fdp->fd;
-		if (fd >= 0) {
-			int fput_needed;
-			struct file * file = fget_light(fd, &fput_needed);
-			mask = POLLNVAL;
-			if (file != NULL) {
-				mask = DEFAULT_POLLMASK;
-				if (file->f_op && file->f_op->poll)
-					mask = file->f_op->poll(file, *pwait);
-				mask &= fdp->events | POLLERR | POLLHUP;
-				fput_light(file, fput_needed);
-			}
-			if (mask) {
-				*pwait = NULL;
-				(*count)++;
-			}
+	mask = 0;
+	fd = pollfd->fd;
+	if (fd >= 0) {
+		int fput_needed;
+		struct file * file;
+
+		file = fget_light(fd, &fput_needed);
+		mask = POLLNVAL;
+		if (file != NULL) {
+			mask = DEFAULT_POLLMASK;
+			if (file->f_op && file->f_op->poll)
+				mask = file->f_op->poll(file, pwait);
+			mask &= pollfd->events | POLLERR | POLLHUP;
+			fput_light(file, fput_needed);
 		}
-		fdp->revents = mask;
 	}
+	pollfd->revents = mask;
+
+	return (mask != 0);
 }

 static int do_poll(unsigned int nfds,  struct poll_list *list,
@@ -592,10 +585,19 @@ static int do_poll(unsigned int nfds,  s
 		long __timeout;

 		set_current_state(TASK_INTERRUPTIBLE);
-		walk = list;
-		while(walk != NULL) {
-			do_pollfd( walk->len, walk->entries, &pt, &count);
-			walk = walk->next;
+		for (walk = list; walk != NULL; walk = walk->next) {
+			struct pollfd * pfd, * pfd_end;
+
+			pfd = walk->entries;
+			pfd_end = pfd + walk->len;
+			for (; pfd != pfd_end; pfd++) {
+				int ev;
+
+				ev = do_pollfd(pfd, pt);
+				count += ev;
+				ev--;
+				pt = (poll_table*)((unsigned long)pt & ev);
+			}
 		}
 		pt = NULL;
 		if (count || !*timeout || signal_pending(current))

Re: [PATCH] Poll microoptimizations.

[Andrew Morton]

Vadim Lobanov <vlobanov@speakeasy.net> wrote:
>
> Patch to provide some microoptimizations for the poll() system call
> implementation. The loop that traverses over the "struct pollfd" entries
> was moved from do_pollfd() to its single caller do_poll(), so that
> do_pollfd() no longer mucks around with the "count" and the "pt"
> variables that should belong to do_poll() alone. This saves unnecessary
> levels of indirection. Modifications were run tested.
> 
> 
> diff -Npru linux-2.6.17-rc1/fs/select.c linux-new/fs/select.c
> --- linux-2.6.17-rc1/fs/select.c	2006-04-12 20:31:54.000000000 -0700
> +++ linux-new/fs/select.c	2006-04-13 18:54:14.000000000 -0700
> @@ -544,37 +544,30 @@ struct poll_list {
> 
>  #define POLLFD_PER_PAGE  ((PAGE_SIZE-sizeof(struct poll_list)) / sizeof(struct pollfd))
> 
> -static void do_pollfd(unsigned int num, struct pollfd * fdpage,
> -	poll_table ** pwait, int *count)
> +static int do_pollfd(struct pollfd * pollfd, poll_table * pwait)

Please omit the space after the asterisk:

	static int do_pollfd(struct pollfd *pollfd, poll_table *pwait)

because it doesn't impart any information, it is sightly misleading, it
wastes screen real-estate and we should be consistent.

>  {
> -	int i;
> +	unsigned int mask;
> +	int fd;
> 
> -	for (i = 0; i < num; i++) {
> -		int fd;
> -		unsigned int mask;
> -		struct pollfd *fdp;
> -
> -		mask = 0;
> -		fdp = fdpage+i;
> -		fd = fdp->fd;
> -		if (fd >= 0) {
> -			int fput_needed;
> -			struct file * file = fget_light(fd, &fput_needed);
> -			mask = POLLNVAL;
> -			if (file != NULL) {
> -				mask = DEFAULT_POLLMASK;
> -				if (file->f_op && file->f_op->poll)
> -					mask = file->f_op->poll(file, *pwait);
> -				mask &= fdp->events | POLLERR | POLLHUP;
> -				fput_light(file, fput_needed);
> -			}
> -			if (mask) {
> -				*pwait = NULL;
> -				(*count)++;
> -			}
> +	mask = 0;
> +	fd = pollfd->fd;
> +	if (fd >= 0) {
> +		int fput_needed;
> +		struct file * file;
> +
> +		file = fget_light(fd, &fput_needed);
> +		mask = POLLNVAL;
> +		if (file != NULL) {
> +			mask = DEFAULT_POLLMASK;
> +			if (file->f_op && file->f_op->poll)
> +				mask = file->f_op->poll(file, pwait);
> +			mask &= pollfd->events | POLLERR | POLLHUP;
> +			fput_light(file, fput_needed);
>  		}
> -		fdp->revents = mask;
>  	}
> +	pollfd->revents = mask;
> +
> +	return (mask != 0);
>  }

So do_poll_fd() returns either 0 or 1.

>  static int do_poll(unsigned int nfds,  struct poll_list *list,
> @@ -592,10 +585,19 @@ static int do_poll(unsigned int nfds,  s
>  		long __timeout;
> 
>  		set_current_state(TASK_INTERRUPTIBLE);
> -		walk = list;
> -		while(walk != NULL) {
> -			do_pollfd( walk->len, walk->entries, &pt, &count);
> -			walk = walk->next;
> +		for (walk = list; walk != NULL; walk = walk->next) {
> +			struct pollfd * pfd, * pfd_end;
> +
> +			pfd = walk->entries;
> +			pfd_end = pfd + walk->len;
> +			for (; pfd != pfd_end; pfd++) {
> +				int ev;
> +
> +				ev = do_pollfd(pfd, pt);

`ev' is either 0 or 1.

> +				count += ev;
> +				ev--;

`ev' is either -1 or 0.

> +				pt = (poll_table*)((unsigned long)pt & ev);

So as long as the sign-extension works as we hope (which I think it will),
`pt' is either unaltered or is NULL.

Yuk.  Sorry, no.

Re: [PATCH] Poll microoptimizations.

[Vadim Lobanov]

On Fri, 14 Apr 2006, Andrew Morton wrote:

> Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> >
> > Patch to provide some microoptimizations for the poll() system call
> > implementation. The loop that traverses over the "struct pollfd" entries
> > was moved from do_pollfd() to its single caller do_poll(), so that
> > do_pollfd() no longer mucks around with the "count" and the "pt"
> > variables that should belong to do_poll() alone. This saves unnecessary
> > levels of indirection. Modifications were run tested.
> >
> >
> > diff -Npru linux-2.6.17-rc1/fs/select.c linux-new/fs/select.c
> > --- linux-2.6.17-rc1/fs/select.c	2006-04-12 20:31:54.000000000 -0700
> > +++ linux-new/fs/select.c	2006-04-13 18:54:14.000000000 -0700
> > @@ -544,37 +544,30 @@ struct poll_list {
> >
> >  #define POLLFD_PER_PAGE  ((PAGE_SIZE-sizeof(struct poll_list)) / sizeof(struct pollfd))
> >
> > -static void do_pollfd(unsigned int num, struct pollfd * fdpage,
> > -	poll_table ** pwait, int *count)
> > +static int do_pollfd(struct pollfd * pollfd, poll_table * pwait)
>
> Please omit the space after the asterisk:
>
> 	static int do_pollfd(struct pollfd *pollfd, poll_table *pwait)
>
> because it doesn't impart any information, it is sightly misleading, it
> wastes screen real-estate and we should be consistent.

Will do. For better or worse, I was following the original do_pollfd()
declaration, which had the spaces in there.

> >  {
> > -	int i;
> > +	unsigned int mask;
> > +	int fd;
> >
> > -	for (i = 0; i < num; i++) {
> > -		int fd;
> > -		unsigned int mask;
> > -		struct pollfd *fdp;
> > -
> > -		mask = 0;
> > -		fdp = fdpage+i;
> > -		fd = fdp->fd;
> > -		if (fd >= 0) {
> > -			int fput_needed;
> > -			struct file * file = fget_light(fd, &fput_needed);
> > -			mask = POLLNVAL;
> > -			if (file != NULL) {
> > -				mask = DEFAULT_POLLMASK;
> > -				if (file->f_op && file->f_op->poll)
> > -					mask = file->f_op->poll(file, *pwait);
> > -				mask &= fdp->events | POLLERR | POLLHUP;
> > -				fput_light(file, fput_needed);
> > -			}
> > -			if (mask) {
> > -				*pwait = NULL;
> > -				(*count)++;
> > -			}
> > +	mask = 0;
> > +	fd = pollfd->fd;
> > +	if (fd >= 0) {
> > +		int fput_needed;
> > +		struct file * file;
> > +
> > +		file = fget_light(fd, &fput_needed);
> > +		mask = POLLNVAL;
> > +		if (file != NULL) {
> > +			mask = DEFAULT_POLLMASK;
> > +			if (file->f_op && file->f_op->poll)
> > +				mask = file->f_op->poll(file, pwait);
> > +			mask &= pollfd->events | POLLERR | POLLHUP;
> > +			fput_light(file, fput_needed);
> >  		}
> > -		fdp->revents = mask;
> >  	}
> > +	pollfd->revents = mask;
> > +
> > +	return (mask != 0);
> >  }
>
> So do_poll_fd() returns either 0 or 1.

Correct. It returns true if an event was seen, false otherwise. Should I
stick a comment in front of the function saying this?

> >  static int do_poll(unsigned int nfds,  struct poll_list *list,
> > @@ -592,10 +585,19 @@ static int do_poll(unsigned int nfds,  s
> >  		long __timeout;
> >
> >  		set_current_state(TASK_INTERRUPTIBLE);
> > -		walk = list;
> > -		while(walk != NULL) {
> > -			do_pollfd( walk->len, walk->entries, &pt, &count);
> > -			walk = walk->next;
> > +		for (walk = list; walk != NULL; walk = walk->next) {
> > +			struct pollfd * pfd, * pfd_end;
> > +
> > +			pfd = walk->entries;
> > +			pfd_end = pfd + walk->len;
> > +			for (; pfd != pfd_end; pfd++) {
> > +				int ev;
> > +
> > +				ev = do_pollfd(pfd, pt);
>
> `ev' is either 0 or 1.

Correct.

> > +				count += ev;
> > +				ev--;
>
> `ev' is either -1 or 0.

Correct.

> > +				pt = (poll_table*)((unsigned long)pt & ev);
>
> So as long as the sign-extension works as we hope (which I think it will),
> `pt' is either unaltered or is NULL.

Correct - this mimicks the original logic exactly, which "kills" pt as
soon as a single event was seen. If I understand correctly, this was
done so that none of the following pollfd entries register themselves in
the poll_table, just to be immediately removed when the poll() syscall
terminates.

I can put in a comment to explain what the code is doing, or if you
think that the bitmasking itself is "yuk", then I can easily transform
the code into an explicit "if () {}" block. :)

> Yuk.  Sorry, no.

Thank you for the review. The comments above are easy to address. Do you
like the main concept behind the patch? Should I correct and resubmit?

- Vadim Lobanov

Re: [PATCH] Poll microoptimizations.

[Andrew Morton]

Vadim Lobanov <vlobanov@speakeasy.net> wrote:
>
> I can put in a comment to explain what the code is doing, or if you
> think that the bitmasking itself is "yuk", then I can easily transform
> the code into an explicit "if () {}" block. :)

yes please.

> > Yuk.  Sorry, no.
> 
> Thank you for the review. The comments above are easy to address. Do you
> like the main concept behind the patch? Should I correct and resubmit?

I don't really understand it yet.

Yes, please resend and feel free to a) add comments in places where we can
help people to understand the code and b) convert any code which gets
touched to be coding-style-friendly.  (I usually recommend that we do that
even if the surrounding code uses different conventions - eventually
everything will be fixed ;))

Re: [PATCH] Poll microoptimizations.

[Vadim Lobanov]

On Fri, 14 Apr 2006, Andrew Morton wrote:

> Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> >
> > I can put in a comment to explain what the code is doing, or if you
> > think that the bitmasking itself is "yuk", then I can easily transform
> > the code into an explicit "if () {}" block. :)
>
> yes please.
>
> > > Yuk.  Sorry, no.
> >
> > Thank you for the review. The comments above are easy to address. Do you
> > like the main concept behind the patch? Should I correct and resubmit?
>
> I don't really understand it yet.

It's really a bit of (subjective) cleanup, that just incidentally
happens to save us a few extra clock cycles here and there. In the
current code, the "count" and "pt" variables are modified both in the
function where they're declared (do_poll()), AND also indirectly in a
different function (do_pollfd()). The patch moves all handling of these
variables to the function that declares and "owns" them (do_poll()).

> Yes, please resend and feel free to a) add comments in places where we can
> help people to understand the code and b) convert any code which gets
> touched to be coding-style-friendly.  (I usually recommend that we do that
> even if the surrounding code uses different conventions - eventually
> everything will be fixed ;))

I couldn't agree more on this particular point. The only thing that
stops me is that noone can ever agree on the coding style, even if it is
spelled out in the Documentation/ directory (witness the periodic flame
wars on this list). Helps to have a thick skin, and I'm slowly getting
to that point. ;)

I'll correct, comment, and resend the patch when I get a chance.

- Vadim Lobanov

[PATCH] Poll cleanups/microoptimizations.

[Vadim Lobanov]

Poll cleanup and microoptimization patch.

The "count" and "pt" variables are declared and modified by do_poll(),
as well as accessed and written indirectly in the do_pollfd()
subroutine. This patch pulls all handling of these variables into the
do_poll() function, thereby eliminating the odd use of indirection in
do_pollfd(). This is done by pulling the "struct pollfd" traversal loop
from do_pollfd() into its only caller do_poll(). As an added bonus, the
patch saves a few clock cycles, and also adds comments to make the code
easier to follow.

Signed-off-by: Vadim Lobanov <vlobanov@speakeasy.net>

diff -Npru linux-2.6.17-rc1/fs/select.c linux-new/fs/select.c
--- linux-2.6.17-rc1/fs/select.c	2006-04-12 20:31:54.000000000 -0700
+++ linux-new/fs/select.c	2006-04-14 19:12:37.000000000 -0700
@@ -544,37 +544,38 @@ struct poll_list {

 #define POLLFD_PER_PAGE  ((PAGE_SIZE-sizeof(struct poll_list)) / sizeof(struct pollfd))

-static void do_pollfd(unsigned int num, struct pollfd * fdpage,
-	poll_table ** pwait, int *count)
+/*
+ * Fish for pollable events on the pollfd->fd file descriptor. We're only
+ * interested in events matching the pollfd->events mask, and the result
+ * matching that mask is both recorded in pollfd->revents and returned. The
+ * pwait poll_table will be used by the fd-provided poll handler for waiting,
+ * if non-NULL.
+ */
+static inline unsigned int do_pollfd(struct pollfd *pollfd, poll_table *pwait)
 {
-	int i;
+	unsigned int mask;
+	int fd;

-	for (i = 0; i < num; i++) {
-		int fd;
-		unsigned int mask;
-		struct pollfd *fdp;
-
-		mask = 0;
-		fdp = fdpage+i;
-		fd = fdp->fd;
-		if (fd >= 0) {
-			int fput_needed;
-			struct file * file = fget_light(fd, &fput_needed);
-			mask = POLLNVAL;
-			if (file != NULL) {
-				mask = DEFAULT_POLLMASK;
-				if (file->f_op && file->f_op->poll)
-					mask = file->f_op->poll(file, *pwait);
-				mask &= fdp->events | POLLERR | POLLHUP;
-				fput_light(file, fput_needed);
-			}
-			if (mask) {
-				*pwait = NULL;
-				(*count)++;
-			}
+	mask = 0;
+	fd = pollfd->fd;
+	if (fd >= 0) {
+		int fput_needed;
+		struct file * file;
+
+		file = fget_light(fd, &fput_needed);
+		mask = POLLNVAL;
+		if (file != NULL) {
+			mask = DEFAULT_POLLMASK;
+			if (file->f_op && file->f_op->poll)
+				mask = file->f_op->poll(file, pwait);
+			/* Mask out unneeded events. */
+			mask &= pollfd->events | POLLERR | POLLHUP;
+			fput_light(file, fput_needed);
 		}
-		fdp->revents = mask;
 	}
+	pollfd->revents = mask;
+
+	return mask;
 }

 static int do_poll(unsigned int nfds,  struct poll_list *list,
@@ -592,11 +593,29 @@ static int do_poll(unsigned int nfds,  s
 		long __timeout;

 		set_current_state(TASK_INTERRUPTIBLE);
-		walk = list;
-		while(walk != NULL) {
-			do_pollfd( walk->len, walk->entries, &pt, &count);
-			walk = walk->next;
+		for (walk = list; walk != NULL; walk = walk->next) {
+			struct pollfd * pfd, * pfd_end;
+
+			pfd = walk->entries;
+			pfd_end = pfd + walk->len;
+			for (; pfd != pfd_end; pfd++) {
+				/*
+				 * Fish for events. If we found one, record it
+				 * and kill the poll_table, so we don't
+				 * needlessly register any other waiters after
+				 * this. They'll get immediately deregistered
+				 * when we break out and return.
+				 */
+				if (do_pollfd(pfd, pt)) {
+					count++;
+					pt = NULL;
+				}
+			}
 		}
+		/*
+		 * All waiters have already been registered, so don't provide
+		 * a poll_table to them on the next loop iteration.
+		 */
 		pt = NULL;
 		if (count || !*timeout || signal_pending(current))
 			break;