[PATCH] Add ssleep_interruptible()

[PATCH] Add ssleep_interruptible()

[Nishanth Aravamudan]

Description: Adds ssleep_interruptible() to allow longer delays to occur
in TASK_INTERRUPTIBLE, similarly to ssleep(). To be consistent with
msleep_interruptible(), ssleep_interruptible() returns the remaining time
left in the delay in terms of seconds. This required dividing the return
value of msleep_interruptible() by 1000, thus a cast to (unsigned long)
to prevent any floating point issues.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

--- 2.6.10-rc1-vanilla/include/linux/delay.h	2004-10-30 15:34:03.000000000 -0700
+++ 2.6.10-rc1/include/linux/delay.h	2004-11-01 12:06:11.000000000 -0800
@@ -46,4 +46,9 @@ static inline void ssleep(unsigned int s
 	msleep(seconds * 1000);
 }
 
+static inline unsigned long ssleep_interruptible(unsigned int seconds)
+{
+	return (unsigned long)(msleep_interruptible(seconds * 1000) / 1000);
+}
+
 #endif /* defined(_LINUX_DELAY_H) */

Re: [PATCH] Add ssleep_interruptible()

[Nishanth Aravamudan]

On Mon, Nov 01, 2004 at 12:07:49PM -0800, Nishanth Aravamudan wrote:
> Description: Adds ssleep_interruptible() to allow longer delays to occur
> in TASK_INTERRUPTIBLE, similarly to ssleep(). To be consistent with
> msleep_interruptible(), ssleep_interruptible() returns the remaining time
> left in the delay in terms of seconds. This required dividing the return
> value of msleep_interruptible() by 1000, thus a cast to (unsigned long)
> to prevent any floating point issues.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> --- 2.6.10-rc1-vanilla/include/linux/delay.h	2004-10-30 
> 15:34:03.000000000 -0700
> +++ 2.6.10-rc1/include/linux/delay.h	2004-11-01 12:06:11.000000000 -0800
> @@ -46,4 +46,9 @@ static inline void ssleep(unsigned int s
> 	msleep(seconds * 1000);
> }
> 
> +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> +{
> +	return (unsigned long)(msleep_interruptible(seconds * 1000) / 1000);
> +}
> +
> #endif /* defined(_LINUX_DELAY_H) */

After a discussion on IRC, I believe it is pretty clear that this
function has serious issues. Mainly, that if I request a delay of 1
second, but msleep_interruptible() returns after 1 millisecond, then
ssleep_interruptible() will return 0, claiming the entire delay was
used (due to rounding).

Perhaps we should just be satisfied with milliseconds being the grossest
(in contrast to fine) measure of time, at least in terms of
interruptible delays. ssleep() is unaffected by this problem, of course.

Please revert this patch, if applied, as well as any of the other
patches I sent using ssleep_interruptible() [only a handful].

Thanks,
Nish

Re: [PATCH] Add ssleep_interruptible()

[Horms]

On Tue, Nov 16, 2004 at 05:30:59PM -0800, Nishanth Aravamudan wrote:
> On Mon, Nov 01, 2004 at 12:07:49PM -0800, Nishanth Aravamudan wrote:
> > Description: Adds ssleep_interruptible() to allow longer delays to occur
> > in TASK_INTERRUPTIBLE, similarly to ssleep(). To be consistent with
> > msleep_interruptible(), ssleep_interruptible() returns the remaining time
> > left in the delay in terms of seconds. This required dividing the return
> > value of msleep_interruptible() by 1000, thus a cast to (unsigned long)
> > to prevent any floating point issues.
> > 
> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > 
> > --- 2.6.10-rc1-vanilla/include/linux/delay.h	2004-10-30 
> > 15:34:03.000000000 -0700
> > +++ 2.6.10-rc1/include/linux/delay.h	2004-11-01 12:06:11.000000000 -0800
> > @@ -46,4 +46,9 @@ static inline void ssleep(unsigned int s
> > 	msleep(seconds * 1000);
> > }
> > 
> > +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> > +{
> > +	return (unsigned long)(msleep_interruptible(seconds * 1000) / 1000);
> > +}
> > +
> > #endif /* defined(_LINUX_DELAY_H) */
> 
> After a discussion on IRC, I believe it is pretty clear that this
> function has serious issues. Mainly, that if I request a delay of 1
> second, but msleep_interruptible() returns after 1 millisecond, then
> ssleep_interruptible() will return 0, claiming the entire delay was
> used (due to rounding).
> 
> Perhaps we should just be satisfied with milliseconds being the grossest
> (in contrast to fine) measure of time, at least in terms of
> interruptible delays. ssleep() is unaffected by this problem, of course.
> 
> Please revert this patch, if applied, as well as any of the other
> patches I sent using ssleep_interruptible() [only a handful].

Would making sure that the time slept was always rounded up to
the nearest second resolve this problem. I believe that rounding
up is a common approach to resolving this type of problem when
changing clock resolution.

I am thinking of something like this.

===== include/linux/delay.h 1.6 vs edited =====
--- 1.6/include/linux/delay.h	2004-09-03 18:08:32 +09:00
+++ edited/include/linux/delay.h	2004-11-22 11:47:03 +09:00
@@ -46,4 +46,10 @@ static inline void ssleep(unsigned int s
 	msleep(seconds * 1000);
 }
 
+static inline unsigned long ssleep_interruptible(unsigned int seconds)
+{
+	return (unsigned long)((msleep_interruptible(seconds * 1000) + 999) / 
+			1000);
+}
+
 #endif /* defined(_LINUX_DELAY_H) */

-- 
Horms

Re: [PATCH] Add ssleep_interruptible()

[Nishanth Aravamudan]

On Mon, Nov 22, 2004 at 11:48:05AM +0900, Horms wrote:
> On Tue, Nov 16, 2004 at 05:30:59PM -0800, Nishanth Aravamudan wrote:
> > On Mon, Nov 01, 2004 at 12:07:49PM -0800, Nishanth Aravamudan wrote:
> > > Description: Adds ssleep_interruptible() to allow longer delays to occur
> > > in TASK_INTERRUPTIBLE, similarly to ssleep(). To be consistent with
> > > msleep_interruptible(), ssleep_interruptible() returns the remaining time
> > > left in the delay in terms of seconds. This required dividing the return
> > > value of msleep_interruptible() by 1000, thus a cast to (unsigned long)
> > > to prevent any floating point issues.
> > > 
> > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > > 
> > > --- 2.6.10-rc1-vanilla/include/linux/delay.h	2004-10-30 
> > > 15:34:03.000000000 -0700
> > > +++ 2.6.10-rc1/include/linux/delay.h	2004-11-01 12:06:11.000000000 -0800
> > > @@ -46,4 +46,9 @@ static inline void ssleep(unsigned int s
> > > 	msleep(seconds * 1000);
> > > }
> > > 
> > > +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> > > +{
> > > +	return (unsigned long)(msleep_interruptible(seconds * 1000) / 1000);
> > > +}
> > > +
> > > #endif /* defined(_LINUX_DELAY_H) */
> > 
> > After a discussion on IRC, I believe it is pretty clear that this
> > function has serious issues. Mainly, that if I request a delay of 1
> > second, but msleep_interruptible() returns after 1 millisecond, then
> > ssleep_interruptible() will return 0, claiming the entire delay was
> > used (due to rounding).
> > 
> > Perhaps we should just be satisfied with milliseconds being the grossest
> > (in contrast to fine) measure of time, at least in terms of
> > interruptible delays. ssleep() is unaffected by this problem, of course.
> > 
> > Please revert this patch, if applied, as well as any of the other
> > patches I sent using ssleep_interruptible() [only a handful].
> 
> Would making sure that the time slept was always rounded up to
> the nearest second resolve this problem. I believe that rounding
> up is a common approach to resolving this type of problem when
> changing clock resolution.
> 
> I am thinking of something like this.
> 
> ===== include/linux/delay.h 1.6 vs edited =====
> --- 1.6/include/linux/delay.h	2004-09-03 18:08:32 +09:00
> +++ edited/include/linux/delay.h	2004-11-22 11:47:03 +09:00
> @@ -46,4 +46,10 @@ static inline void ssleep(unsigned int s
>  	msleep(seconds * 1000);
>  }
>  
> +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> +{
> +	return (unsigned long)((msleep_interruptible(seconds * 1000) + 999) / 
> +			1000);

This is a good idea, but I have two issues:

1) A major reason for having msecs_to_jiffies() and co. is to avoid
having to do this type of conversion ourselves. A weak argument,
admittedly, but just something to keep in mind.

2) This still has a serious logical flaw: If I request 1 second of
sleep, and I don't sleep the entire time, then it is now guaranteed that
I will think I did not sleep at all (ie. ssleep_interruptible() will
return 1). That's just another version of the original issue.

I just don't think it's useful to have this coarse of granularity, at
least in terms of interruptible sleep.

-Nish

Re: [PATCH] Add ssleep_interruptible()

[Horms]

On Mon, Nov 22, 2004 at 09:19:22AM -0800, Nishanth Aravamudan wrote:
> On Mon, Nov 22, 2004 at 11:48:05AM +0900, Horms wrote:
> > On Tue, Nov 16, 2004 at 05:30:59PM -0800, Nishanth Aravamudan wrote:
> > > On Mon, Nov 01, 2004 at 12:07:49PM -0800, Nishanth Aravamudan wrote:
> > > > Description: Adds ssleep_interruptible() to allow longer delays to occur
> > > > in TASK_INTERRUPTIBLE, similarly to ssleep(). To be consistent with
> > > > msleep_interruptible(), ssleep_interruptible() returns the remaining time
> > > > left in the delay in terms of seconds. This required dividing the return
> > > > value of msleep_interruptible() by 1000, thus a cast to (unsigned long)
> > > > to prevent any floating point issues.
> > > > 
> > > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > > > 
> > > > --- 2.6.10-rc1-vanilla/include/linux/delay.h	2004-10-30 
> > > > 15:34:03.000000000 -0700
> > > > +++ 2.6.10-rc1/include/linux/delay.h	2004-11-01 12:06:11.000000000 -0800
> > > > @@ -46,4 +46,9 @@ static inline void ssleep(unsigned int s
> > > > 	msleep(seconds * 1000);
> > > > }
> > > > 
> > > > +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> > > > +{
> > > > +	return (unsigned long)(msleep_interruptible(seconds * 1000) / 1000);
> > > > +}
> > > > +
> > > > #endif /* defined(_LINUX_DELAY_H) */
> > > 
> > > After a discussion on IRC, I believe it is pretty clear that this
> > > function has serious issues. Mainly, that if I request a delay of 1
> > > second, but msleep_interruptible() returns after 1 millisecond, then
> > > ssleep_interruptible() will return 0, claiming the entire delay was
> > > used (due to rounding).
> > > 
> > > Perhaps we should just be satisfied with milliseconds being the grossest
> > > (in contrast to fine) measure of time, at least in terms of
> > > interruptible delays. ssleep() is unaffected by this problem, of course.
> > > 
> > > Please revert this patch, if applied, as well as any of the other
> > > patches I sent using ssleep_interruptible() [only a handful].
> > 
> > Would making sure that the time slept was always rounded up to
> > the nearest second resolve this problem. I believe that rounding
> > up is a common approach to resolving this type of problem when
> > changing clock resolution.
> > 
> > I am thinking of something like this.
> > 
> > ===== include/linux/delay.h 1.6 vs edited =====
> > --- 1.6/include/linux/delay.h	2004-09-03 18:08:32 +09:00
> > +++ edited/include/linux/delay.h	2004-11-22 11:47:03 +09:00
> > @@ -46,4 +46,10 @@ static inline void ssleep(unsigned int s
> >  	msleep(seconds * 1000);
> >  }
> >  
> > +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> > +{
> > +	return (unsigned long)((msleep_interruptible(seconds * 1000) + 999) / 
> > +			1000);
> 
> This is a good idea, but I have two issues:
> 
> 1) A major reason for having msecs_to_jiffies() and co. is to avoid
> having to do this type of conversion ourselves. A weak argument,
> admittedly, but just something to keep in mind.
> 
> 2) This still has a serious logical flaw: If I request 1 second of
> sleep, and I don't sleep the entire time, then it is now guaranteed that
> I will think I did not sleep at all (ie. ssleep_interruptible() will
> return 1). That's just another version of the original issue.
> 
> I just don't think it's useful to have this coarse of granularity, at
> least in terms of interruptible sleep.

If it is unacceptable to neither underestimate or overestimate the
duration of a sleep to the nearest second (the unit of granularity of
the sleep in this case) then I agree. That is unless you want to request
a sleep in seconds but have the duration returned in milliseconds. But
if that is the case then it is probably more sensible to just use
msleep_interruptible() and be done with it.

-- 
Horms

Re: [PATCH] Add ssleep_interruptible()

[Nishanth Aravamudan]

On Wed, Nov 24, 2004 at 11:01:13AM +0900, Horms wrote:
> On Mon, Nov 22, 2004 at 09:19:22AM -0800, Nishanth Aravamudan wrote:
> > On Mon, Nov 22, 2004 at 11:48:05AM +0900, Horms wrote:
> > > On Tue, Nov 16, 2004 at 05:30:59PM -0800, Nishanth Aravamudan wrote:
> > > > On Mon, Nov 01, 2004 at 12:07:49PM -0800, Nishanth Aravamudan wrote:
> > > > > Description: Adds ssleep_interruptible() to allow longer delays to occur
> > > > > in TASK_INTERRUPTIBLE, similarly to ssleep(). To be consistent with
> > > > > msleep_interruptible(), ssleep_interruptible() returns the remaining time
> > > > > left in the delay in terms of seconds. This required dividing the return
> > > > > value of msleep_interruptible() by 1000, thus a cast to (unsigned long)
> > > > > to prevent any floating point issues.
> > > > > 
> > > > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > > > > 
> > > > > --- 2.6.10-rc1-vanilla/include/linux/delay.h	2004-10-30 
> > > > > 15:34:03.000000000 -0700
> > > > > +++ 2.6.10-rc1/include/linux/delay.h	2004-11-01 12:06:11.000000000 -0800
> > > > > @@ -46,4 +46,9 @@ static inline void ssleep(unsigned int s
> > > > > 	msleep(seconds * 1000);
> > > > > }
> > > > > 
> > > > > +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> > > > > +{
> > > > > +	return (unsigned long)(msleep_interruptible(seconds * 1000) / 1000);
> > > > > +}
> > > > > +
> > > > > #endif /* defined(_LINUX_DELAY_H) */
> > > > 
> > > > After a discussion on IRC, I believe it is pretty clear that this
> > > > function has serious issues. Mainly, that if I request a delay of 1
> > > > second, but msleep_interruptible() returns after 1 millisecond, then
> > > > ssleep_interruptible() will return 0, claiming the entire delay was
> > > > used (due to rounding).
> > > > 
> > > > Perhaps we should just be satisfied with milliseconds being the grossest
> > > > (in contrast to fine) measure of time, at least in terms of
> > > > interruptible delays. ssleep() is unaffected by this problem, of course.
> > > > 
> > > > Please revert this patch, if applied, as well as any of the other
> > > > patches I sent using ssleep_interruptible() [only a handful].
> > > 
> > > Would making sure that the time slept was always rounded up to
> > > the nearest second resolve this problem. I believe that rounding
> > > up is a common approach to resolving this type of problem when
> > > changing clock resolution.
> > > 
> > > I am thinking of something like this.
> > > 
> > > ===== include/linux/delay.h 1.6 vs edited =====
> > > --- 1.6/include/linux/delay.h	2004-09-03 18:08:32 +09:00
> > > +++ edited/include/linux/delay.h	2004-11-22 11:47:03 +09:00
> > > @@ -46,4 +46,10 @@ static inline void ssleep(unsigned int s
> > >  	msleep(seconds * 1000);
> > >  }
> > >  
> > > +static inline unsigned long ssleep_interruptible(unsigned int seconds)
> > > +{
> > > +	return (unsigned long)((msleep_interruptible(seconds * 1000) + 999) / 
> > > +			1000);
> > 
> > This is a good idea, but I have two issues:
> > 
> > 1) A major reason for having msecs_to_jiffies() and co. is to avoid
> > having to do this type of conversion ourselves. A weak argument,
> > admittedly, but just something to keep in mind.
> > 
> > 2) This still has a serious logical flaw: If I request 1 second of
> > sleep, and I don't sleep the entire time, then it is now guaranteed that
> > I will think I did not sleep at all (ie. ssleep_interruptible() will
> > return 1). That's just another version of the original issue.
> > 
> > I just don't think it's useful to have this coarse of granularity, at
> > least in terms of interruptible sleep.
> 
> If it is unacceptable to neither underestimate or overestimate the
> duration of a sleep to the nearest second (the unit of granularity of
> the sleep in this case) then I agree.

This is kind of my position. Overestimating leads to the potential, if a
loop is used by the caller, of never leaving the loop, e.g.

timeout = 1;
while (timeout) {
	timeout = ssleep_interruptible(timeout);
}

Underestimating leads to leaving the loop too early, because the caller
thinks a full second has expired and thus a signal was *not* received in
on *full* second, typically leading to an error condition.

> That is unless you want to request
> a sleep in seconds but have the duration returned in milliseconds. But
> if that is the case then it is probably more sensible to just use
> msleep_interruptible() and be done with it.

Exactly, I think that an API which has a parameter in seconds and a
return value in milliseconds is pretty bad. Makes things very confusing
and really msleep_interruptible() is the same, just a difference of
parameter units, then.

-Nish