lkml.org 
[lkml]   [2005]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [UPDATE PATCH][Bug 5132] fix sys_poll() large timeout handling
On 12.09.2005 [10:30:38 -0400], Peter Staubach wrote:
> Nishanth Aravamudan wrote:
>
> >On 09.09.2005 [19:36:21 -0700], Andrew Morton wrote:
> >
> >
> >>Nishanth Aravamudan <nacc@us.ibm.com> wrote:
> >>
> >>
> >>>+ /*
> >>>+ * We compare HZ with 1000 to work out which side of the
> >>>+ * expression needs conversion. Because we want to avoid
> >>>+ * converting any value to a numerically higher value, which
> >>>+ * could overflow.
> >>>+ */
> >>>+#if HZ > 1000
> >>>+ overflow = timeout_msecs >= jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT);
> >>>+#else
> >>>+ overflow = msecs_to_jiffies(timeout_msecs) >= MAX_SCHEDULE_TIMEOUT;
> >>>+#endif
> >>>+
> >>>+ /*
> >>>+ * If we would overflow in the conversion or a negative timeout
> >>>+ * is requested, sleep indefinitely.
> >>>+ */
> >>>+ if (overflow || timeout_msecs < 0)
> >>>+ timeout_jiffies = MAX_SCHEDULE_TIMEOUT;
> >>>
> >>>
> >>Do we need to test (timeout_msecs < 0) here? If we make timeout_msecs
> >>unsigned long then I think `overflow' will always be correct.
> >>
> >>
> >
> >Even though poll is explicitly allowed to take negative values, as per
> >my man-page:
> >
> >"#include <sys/poll.h>
> >
> >int poll(struct pollfd *ufds, unsigned int nfds, int timeout);
> >
> >...
> >
> >A negative value means infinite timeout."
> >
> >Would we have a local variable to store timeout_msecs as well? Or do we
> >want to make a userspace-visible change like this? I don't have a
> >preference, I just want to make sure I understand.
> >
>
> Actually, given this, isn't the interface for sys_poll() incorrectly
> defined?
> Shouldn't the timeout argument be an int, instead of a long?
>
> And, if we make it an int, then can't we do the math correctly for all
> possible values of the timeout? The patch could look like:
>
> Signed-off-by: Peter Staubach <staubach@redhat.com>
>

> --- linux-2.6.13/fs/select.c.org 2005-08-28 19:41:01.000000000 -0400
> +++ linux-2.6.13/fs/select.c 2005-09-12 10:19:30.000000000 -0400
> @@ -457,25 +457,34 @@ static int do_poll(unsigned int nfds, s
> return count;
> }
>
> -asmlinkage long sys_poll(struct pollfd __user * ufds, unsigned int nfds, long timeout)
> +asmlinkage long sys_poll(struct pollfd __user * ufds, unsigned int nfds, int timeout_msecs)
> {
> struct poll_wqueues table;
> int fdcount, err;
> unsigned int i;
> struct poll_list *head;
> struct poll_list *walk;
> + long timeout;
> + int64_t lltimeout;
>
> /* Do a sanity check on nfds ... */
> if (nfds > current->files->max_fdset && nfds > OPEN_MAX)
> return -EINVAL;
>
> - if (timeout) {
> - /* Careful about overflow in the intermediate values */
> - if ((unsigned long) timeout < MAX_SCHEDULE_TIMEOUT / HZ)
> - timeout = (unsigned long)(timeout*HZ+999)/1000+1;
> - else /* Negative or overflow */
> + if (timeout_msecs) {
> + if (timeout_msecs < 0)
> timeout = MAX_SCHEDULE_TIMEOUT;
> - }
> + else {
> + lltimeout = (int64_t)timeout_msecs * HZ + 999;
> + do_div(lltimeout, 1000);

I don't think the embedded folks are going to be ok with adding a 64-bit
div in the poll() common-path... But otherwise the patch looks pretty
sane, except I think you want s64, not int64_t? I can't ever remember
myself :)

I agree the interface mght be mis-defined. And changing timeout_msecs()
to an integer is consistent with the size of millisecond-unit variables
used elsewhere in the kernel.

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

\
 
 \ /
  Last update: 2005-09-12 17:09    [W:0.062 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site