[RFC/PATCH] 2.6.24-rcx: Make sys_poll() wait at least timeout ms

[RFC/PATCH] 2.6.24-rcx: Make sys_poll() wait at least timeout ms

[Karsten Wiese]

Hi,

while playing with jackd on 2.6.24-rcx, I found poll() timing out too early.
That is: earlier than its timeout argument specified.
Setting poll()'s timeout argument to "required timeout" + "1 jiffy in ms"
fixed it. Patch below should fix it too. Correct?
Untested.
Otherwise 2.6.24-rc5 ticks just fine here, thanks.

      Karsten
 
------------->
Make sys_poll() wait at least timeout ms

schedule_timeout(jiffies) waits for at least jiffies - 1.
Add 1 jiffie to the timeout_jiffies calculated in sys_poll() to wait at least
timeout_msecs, like poll() manpage says.

Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>
---
 fs/select.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index 47f4792..5633fe9 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -739,7 +739,7 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 			timeout_jiffies = -1;
 		else
 #endif
-			timeout_jiffies = msecs_to_jiffies(timeout_msecs);
+			timeout_jiffies = msecs_to_jiffies(timeout_msecs) + 1;
 	} else {
 		/* Infinite (< 0) or no (0) timeout */
 		timeout_jiffies = timeout_msecs;
-- 
1.5.3.6

Re: [RFC/PATCH] 2.6.24-rcx: Make sys_poll() wait at least timeout ms

[Robert Hancock]

Karsten Wiese wrote:
> Hi,
> 
> while playing with jackd on 2.6.24-rcx, I found poll() timing out too early.
> That is: earlier than its timeout argument specified.
> Setting poll()'s timeout argument to "required timeout" + "1 jiffy in ms"
> fixed it. Patch below should fix it too. Correct?
> Untested.
> Otherwise 2.6.24-rc5 ticks just fine here, thanks.
> 
>       Karsten
>  
> ------------->
> Make sys_poll() wait at least timeout ms
> 
> schedule_timeout(jiffies) waits for at least jiffies - 1.
> Add 1 jiffie to the timeout_jiffies calculated in sys_poll() to wait at least
> timeout_msecs, like poll() manpage says.
> 
> Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>
> ---
>  fs/select.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/select.c b/fs/select.c
> index 47f4792..5633fe9 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -739,7 +739,7 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
>  			timeout_jiffies = -1;
>  		else
>  #endif
> -			timeout_jiffies = msecs_to_jiffies(timeout_msecs);
> +			timeout_jiffies = msecs_to_jiffies(timeout_msecs) + 1;
>  	} else {
>  		/* Infinite (< 0) or no (0) timeout */
>  		timeout_jiffies = timeout_msecs;

That seems fishy. What is your value of HZ and what is the timeout value 
that was passed in the bad case?

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [RFC/PATCH] 2.6.24-rcx: Make sys_poll() wait at least timeout ms

[Karsten Wiese]

Am Mittwoch, 19. Dezember 2007 schrieb Robert Hancock:
> 
> That seems fishy. What is your value of HZ and what is the timeout value 
> that was passed in the bad case?

HZ set to 250, timeout to 4ms.
Time spent in poll() taken by clock_gettime(CLOCK_MONOTONIC, &time)
before and after poll()call: i.e 62us.
Time measured with hpet gave 166us once.

      Karsten

Re: [RFC/PATCH] 2.6.24-rcx: Make sys_poll() wait at least timeout ms

[Robert Hancock]

Karsten Wiese wrote:
> Am Mittwoch, 19. Dezember 2007 schrieb Robert Hancock:
>> That seems fishy. What is your value of HZ and what is the timeout value 
>> that was passed in the bad case?
> 
> HZ set to 250, timeout to 4ms.
> Time spent in poll() taken by clock_gettime(CLOCK_MONOTONIC, &time)
> before and after poll()call: i.e 62us.
> Time measured with hpet gave 166us once.

msecs_to_jiffies (kernel/time.c) has this:

#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
	/*
	 * HZ is equal to or smaller than 1000, and 1000 is a nice
	 * round multiple of HZ, divide with the factor between them,
	 * but round upwards:
	 */
	return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);

With HZ=250 and m=4 this gives 7/4 or only 1 jiffy, which is not more 
than 4ms, but if we are already at near the end of the current jiffy it 
could be much less than that (potentially almost no time at all).

Maybe we could convert poll to use a hrtimer for this instead?

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [RFC/PATCH] 2.6.24-rcx: Make sys_poll() wait at least timeout ms

[Karsten Wiese]

Am Mittwoch, 19. Dezember 2007 schrieb Robert Hancock:
> Karsten Wiese wrote:
> > Am Mittwoch, 19. Dezember 2007 schrieb Robert Hancock:
> >> That seems fishy. What is your value of HZ and what is the timeout value 
> >> that was passed in the bad case?
> > 
> > HZ set to 250, timeout to 4ms.
> > Time spent in poll() taken by clock_gettime(CLOCK_MONOTONIC, &time)
> > before and after poll()call: i.e 62us.
> > Time measured with hpet gave 166us once.
> 
> msecs_to_jiffies (kernel/time.c) has this:
> 
> #if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
> 	/*
> 	 * HZ is equal to or smaller than 1000, and 1000 is a nice
> 	 * round multiple of HZ, divide with the factor between them,
> 	 * but round upwards:
> 	 */
> 	return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
> 
> With HZ=250 and m=4 this gives 7/4 or only 1 jiffy, which is not more 
> than 4ms, but if we are already at near the end of the current jiffy it 
> could be much less than that (potentially almost no time at all).
> 
> Maybe we could convert poll to use a hrtimer for this instead?

That wouldn't fix configs without hrtimers.

The linux manpage reflects current behaviour:
from man2/poll.2.gz
"The timeout argument specifies an upper limit on the time for which
poll() will block, in milliseconds."
To achieve "at least" timeouts we'd have to add a jiffy's ms to the
timeout in userspace.

I'd like to let sys_poll() behave according to posix's manpage:
from man3p/poll.3p.gz
"poll() shall wait at least timeout milliseconds"
Thats easier to specify in userspace. No need to know the running kernel's
HZ.

Above posix/linux difference also shows in select() manpages.
epoll_wait() (linux only) manpage also says "maximum time of timeout"
A more complete patch would tweek (p)poll (p)select and epoll_(p)wait.
There are possibly more syscalls affected that I'm not aware off.

Is there upstream interest?

      Karsten