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

[Nish Aravamudan]

On 9/23/05, Willy Tarreau <willy@xxxxxxxxx> wrote:
> Hi Davide,
>
> On Fri, Sep 23, 2005 at 11:13:30AM -0700, Davide Libenzi wrote:
> >
> > 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@xxxxxxxxxxxxxxx>
> >
> >
> > - Davide
>
> > --- a/fs/eventpoll.c  2005-09-23 10:56:57.000000000 -0700
> > +++ b/fs/eventpoll.c  2005-09-23 11:00:06.000000000 -0700
> > @@ -1507,7 +1507,7 @@
> >        * and the overflow condition. The passed timeout is in milliseconds,
> >        * that why (t * HZ) / 1000.
> >        */
> > -     jtimeout = timeout == -1 || timeout > (MAX_SCHEDULE_TIMEOUT - 1000) / HZ ?
> > +     jtimeout = timeout < 0 || (timeout / 1000) >= (MAX_SCHEDULE_TIMEOUT / HZ) ?
> >               MAX_SCHEDULE_TIMEOUT: (timeout * HZ + 999) / 1000;
>
> Here, I'm not certain that gcc will optimize the divide. It would be better
> anyway to write this which is equivalent, and a pure integer comparison :
>
> +       jtimeout = timeout < 0 || timeout >= 1000 * MAX_SCHEDULE_TIMEOUT / HZ ?
> >               MAX_SCHEDULE_TIMEOUT: (timeout * HZ + 999) / 1000;

Just a question here, maybe it's dumb.

* and / have the same priority in the order of operations, yes? If so,
won't the the 1000 * MAX_SCHEDULE_TIMEOUT overflow
(MAX_SCHEDULE_TIMEOUT is LONG_MAX)? I really think this code just move
to the same thing that sys_poll() does to avoid overlflow (I fixed the
bug Alexey was experiencing, so I think the changes are safe now). In
any case, this code is approaching unreadable with lots of jiffies
<--> human-time units manipulations done in non-standard ways, which
the updated sys_poll() also tries to avoid.

Thanks,
Nish
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/