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

Davide Libenzi
2005-09-24T18:16:51Z

[-- 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;