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

Vadim Lobanov
2005-09-25T07:08:06Z

On Sat, 24 Sep 2005, Andrew Morton wrote:

> Davide Libenzi <davidel@xmailserver.org> wrote:
> >
> > 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.
>
> OK, well I've rather lost the plot with all the patches flying around.
>
> I now have one single patch against epoll.c, below. Please confirm that
> this is what was intended. If not, I'll drop it and let's start again.
>

I hate to be the squeaky wheel here, but the attached patch is not 100%
right -

>
> From: Davide Libenzi <davidel@xmailserver.org>
>
> The sys_epoll_wait() function was not handling correctly negative timeouts
> (besides -1), and like sys_poll(), was comparing millisec to secs in
> testing the upper timeout limit.
>
> Signed-off-by: Davide Libenzi <davidel@xmailserver.org>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> ---
>
> fs/eventpoll.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff -puN fs/eventpoll.c~sys_epoll_wait-fix-handling-of-negative-timeouts fs/eventpoll.c
> --- devel/fs/eventpoll.c~sys_epoll_wait-fix-handling-of-negative-timeouts 2005-09-24 23:01:00.000000000 -0700
> +++ devel-akpm/fs/eventpoll.c 2005-09-24 23:02:50.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)

This should instead be:
#define EP_MAX_MSTIMEO min(1000ULL * MAX_SCHEDULE_TIMEOUT / HZ, (LONG_MAX - 999ULL) / HZ)
Here's why:
We want to avoid overflow of (timeout * HZ + 999), or, in other words,
the case where (timeout * HZ + 999) >= LONG_MAX
Unwrapping the equation, we get timeout >= (LONG_MAX - 999) / HZ

The original code isn't _wrong_, but more restrictive than it should be.
In any case, better to fix up the base patch now, before all the other
patches go in. I could do this, or Davide can... it's all good. :-)

> +
> +
> struct epoll_filefd {
> struct file *file;
> int fd;
> @@ -1506,8 +1510,8 @@ static int ep_poll(struct eventpoll *ep,
> * and the overflow condition. The passed timeout is in milliseconds,
> * that why (t * HZ) / 1000.
> */
> - jtimeout = timeout == -1 || timeout > (MAX_SCHEDULE_TIMEOUT - 1000) / HZ ?
> - MAX_SCHEDULE_TIMEOUT: (timeout * HZ + 999) / 1000;
> + jtimeout = (timeout < 0 || timeout >= EP_MAX_MSTIMEO) ?
> + MAX_SCHEDULE_TIMEOUT : (timeout * HZ + 999) / 1000;
>
> retry:
> write_lock_irqsave(&ep->lock, flags);
> _
>
> -

-Vadim Lobanov