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 <[email protected]> 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 <[email protected]>
>
> --- 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |