Re: [patch] sys_epoll_wait() timeout saga ...

[Davide Libenzi <davidel@xmailserver.org>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1851 bytes --]

On Sat, 24 Sep 2005, Willy Tarreau wrote:

> On Sat, Sep 24, 2005 at 08:10:32AM -0700, Davide Libenzi wrote:
>>> +       jtimeout = timeout < 0 || \
>>> +                    timeout >= (1000ULL * MAX_SCHEDULE_TIMEOUT / HZ) ||
>>> \
>>> +                    timeout >= (LONG_MAX / HZ - 1000) ?
>>>                  MAX_SCHEDULE_TIMEOUT: (timeout * HZ + 999) / 1000;
>>>
>>> as both are constants, they can be optimized. Otherwise, we can resort to
>>> using a MAX() macro to reduce this to only one test which will catch all
>>> corner cases.
>>
>> Using the MIN() macro would be better so we have a single check, and the
>> compiler optimize that automatically.
>
> you're right, it's MIN() not MAX() ;-)
> Anyway, I've checked the code and the compiler does a single test with -O2.
>
>> Or we can force 'timeout * HZ' to use ULL math. I don't think it makes a lot of difference for something that is in a likely sleep path ;)
>
> "likely", yes, but not necessarily. Under a high load, you can have enough
> events queued so that epoll() will not wait at all. I've already encountered
> such cases during benchmarks, and I noticed that epoll() took more time than
> select() for small numbers of FDs (something like 20% below 100 FDs), but of
> course, it is considerably faster above. So turning the multiply to an ULL
> may increase this overhead on some architectures, while the double check
> will leave the code identical.

The attached patch uses the kernel min() macro, that is optimized has 
single compare by gcc-O2. Andrew, this goes over (hopefully ;) the bits 
you already have in -mm.

PS: It might be possible to move the currently epoll-local EP_MAX_MSTIMEO 
macro, to a MAX_LONG_MSTIMEO (or whatever name you like) somewhere in the 
tree, to be used where needed.


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


- Davide



[-- Attachment #2: Type: TEXT/plain, Size: 799 bytes --]

--- a/fs/eventpoll.c	2005-09-24 11:07:04.000000000 -0700
+++ b/fs/eventpoll.c	2005-09-24 11:11:06.000000000 -0700
@@ -101,6 +101,10 @@
 /* Maximum number of poll wake up nests we are allowing */
 #define EP_MAX_POLLWAKE_NESTS 4
 
+/* Maximum msec timeout value storeable in a long int */
+#define EP_MAX_MSTIMEO min(1000ULL * MAX_SCHEDULE_TIMEOUT / HZ, LONG_MAX / HZ - 1000ULL)
+
+
 struct epoll_filefd {
 	struct file *file;
 	int fd;
@@ -1507,8 +1511,7 @@
 	 * and the overflow condition. The passed timeout is in milliseconds,
 	 * that why (t * HZ) / 1000.
 	 */
-	jtimeout = (timeout < 0 ||
-		    (timeout / 1000) >= (MAX_SCHEDULE_TIMEOUT / HZ)) ?
+	jtimeout = (timeout < 0 || timeout >= EP_MAX_MSTIMEO) ?
 		MAX_SCHEDULE_TIMEOUT: (timeout * HZ + 999) / 1000;
 
 retry: