* [patch 05/18] net/s2io: replace schedule_timeout() with msleep()
@ 2004-10-30 22:42 janitor
2004-10-31 11:09 ` Jeff Garzik
2004-11-01 20:07 ` [PATCH] Add ssleep_interruptible() Nishanth Aravamudan
0 siblings, 2 replies; 12+ messages in thread
From: janitor @ 2004-10-30 22:42 UTC (permalink / raw)
To: jgarzik; +Cc: netdev, janitor, nacc
Any comments would be appreciated.
Description: Use msleep() instead of schedule_timeout()
to guarantee the task delays as expected.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Signed-off-by: Maximilian Attems <janitor@sternwelten.at>
---
linux-2.6.10-rc1-max/drivers/net/s2io.c | 47 ++++++++++----------------------
1 files changed, 16 insertions(+), 31 deletions(-)
diff -puN drivers/net/s2io.c~msleep-drivers_net_s2io drivers/net/s2io.c
--- linux-2.6.10-rc1/drivers/net/s2io.c~msleep-drivers_net_s2io 2004-10-24 17:05:00.000000000 +0200
+++ linux-2.6.10-rc1-max/drivers/net/s2io.c 2004-10-24 17:05:00.000000000 +0200
@@ -555,8 +555,7 @@ static int initNic(struct s2io_nic *nic)
val64 = 0;
writeq(val64, &bar0->sw_reset);
val64 = readq(&bar0->sw_reset);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 2);
+ msleep(500);
/* Enable Receiving broadcasts */
val64 = readq(&bar0->mac_cfg);
@@ -803,8 +802,7 @@ static int initNic(struct s2io_nic *nic)
dev->name);
return -1;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
time++;
}
@@ -838,8 +836,7 @@ static int initNic(struct s2io_nic *nic)
return -1;
}
time++;
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
}
/* Initializing proper values as Pause threshold into all
@@ -1182,8 +1179,7 @@ static int startNic(struct s2io_nic *nic
writeq(val64, &bar0->mc_rldram_mrs);
val64 = readq(&bar0->mc_rldram_mrs);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 10); /* Delay by around 100 ms. */
+ msleep(100); /* Delay by around 100 ms. */
/* Enabling ECC Protection. */
val64 = readq(&bar0->adapter_control);
@@ -1891,8 +1887,7 @@ int waitForCmdComplete(nic_t * sp)
ret = SUCCESS;
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
if (cnt++ > 10)
break;
}
@@ -1931,15 +1926,13 @@ void s2io_reset(nic_t * sp)
* As of now I'am just giving a 250ms delay and hoping that the
* PCI write to sw_reset register is done by this time.
*/
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 4);
+ msleep(250);
/* Restore the PCI state saved during initializarion. */
pci_restore_state(sp->pdev);
s2io_init_pci(sp);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 4);
+ msleep(250);
/* SXE-002: Configure link and activity LED to turn it off */
subid = sp->pdev->subsystem_device;
@@ -2169,8 +2162,7 @@ int s2io_close(struct net_device *dev)
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
cnt++;
if (cnt == 10) {
DBG_PRINT(ERR_DBG,
@@ -2846,11 +2838,10 @@ static int s2io_ethtool_idnic(struct net
sp->id_timer.data = (unsigned long) sp;
}
mod_timer(&sp->id_timer, jiffies);
- set_current_state(TASK_INTERRUPTIBLE);
if (data)
- schedule_timeout(data * HZ);
+ msleep_interruptible(data * 1000);
else
- schedule_timeout(MAX_SCHEDULE_TIMEOUT);
+ msleep_interruptible(jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
del_timer_sync(&sp->id_timer);
return 0;
@@ -2943,8 +2934,7 @@ static u32 readEeprom(nic_t * sp, int of
data = I2C_CONTROL_GET_DATA(val64);
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
exit_cnt++;
}
@@ -2983,8 +2973,7 @@ static int writeEeprom(nic_t * sp, int o
ret = 0;
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
exit_cnt++;
}
@@ -3256,8 +3245,7 @@ static int s2io_bistTest(nic_t * sp, uin
ret = 0;
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 10);
+ msleep(100);
cnt++;
}
@@ -3356,8 +3344,7 @@ static int s2io_rldramTest(nic_t * sp, u
val64 = readq(&bar0->mc_rldram_test_ctrl);
if (val64 & MC_RLDRAM_TEST_DONE)
break;
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 5);
+ msleep(200);
}
if (cnt == 5)
@@ -3373,8 +3360,7 @@ static int s2io_rldramTest(nic_t * sp, u
val64 = readq(&bar0->mc_rldram_test_ctrl);
if (val64 & MC_RLDRAM_TEST_DONE)
break;
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 2);
+ msleep(500);
}
if (cnt == 5)
@@ -3711,8 +3697,7 @@ static void s2io_set_link(unsigned long
/* Allow a small delay for the NICs self initiated
* cleanup to complete.
*/
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 10);
+ msleep(100);
val64 = readq(&bar0->adapter_status);
if (verify_xena_quiescence(val64, nic->device_enabled_once)) {
_
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 05/18] net/s2io: replace schedule_timeout() with msleep()
2004-10-30 22:42 [patch 05/18] net/s2io: replace schedule_timeout() with msleep() janitor
@ 2004-10-31 11:09 ` Jeff Garzik
2004-11-01 18:31 ` Nishanth Aravamudan
2004-11-01 20:20 ` [PATCH] net/s2io: replace schedule_timeout() with msleep()/ssleep_interruptible() Nishanth Aravamudan
2004-11-01 20:07 ` [PATCH] Add ssleep_interruptible() Nishanth Aravamudan
1 sibling, 2 replies; 12+ messages in thread
From: Jeff Garzik @ 2004-10-31 11:09 UTC (permalink / raw)
To: janitor; +Cc: netdev, nacc
janitor@sternwelten.at wrote:
> @@ -2846,11 +2838,10 @@ static int s2io_ethtool_idnic(struct net
> sp->id_timer.data = (unsigned long) sp;
> }
> mod_timer(&sp->id_timer, jiffies);
> - set_current_state(TASK_INTERRUPTIBLE);
> if (data)
> - schedule_timeout(data * HZ);
> + msleep_interruptible(data * 1000);
clearly wants ssleep() here
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 05/18] net/s2io: replace schedule_timeout() with msleep()
2004-11-01 18:31 ` Nishanth Aravamudan
@ 2004-11-01 18:03 ` Jeff Garzik
2004-11-01 19:14 ` Nishanth Aravamudan
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Garzik @ 2004-11-01 18:03 UTC (permalink / raw)
To: Nishanth Aravamudan; +Cc: janitor, netdev
Nishanth Aravamudan wrote:
> On Sun, Oct 31, 2004 at 06:09:34AM -0500, Jeff Garzik wrote:
>
>>janitor@sternwelten.at wrote:
>>
>>>@@ -2846,11 +2838,10 @@ static int s2io_ethtool_idnic(struct net
>>> sp->id_timer.data = (unsigned long) sp;
>>> }
>>> mod_timer(&sp->id_timer, jiffies);
>>>- set_current_state(TASK_INTERRUPTIBLE);
>>> if (data)
>>>- schedule_timeout(data * HZ);
>>>+ msleep_interruptible(data * 1000);
>>
>>
>>clearly wants ssleep() here
>
>
> Even though ssleep() is in TASK_UNINTERRUPTIBLE? In cases where the
> existing code used TASK_INTERRUPTIBLE, but was sleeping in terms of
> seconds, I left it as msleep_interruptible, since ssleep_interruptible()
> doesn't exist.
Right: create ssleep_interruptible() :)
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 05/18] net/s2io: replace schedule_timeout() with msleep()
2004-10-31 11:09 ` Jeff Garzik
@ 2004-11-01 18:31 ` Nishanth Aravamudan
2004-11-01 18:03 ` Jeff Garzik
2004-11-01 20:20 ` [PATCH] net/s2io: replace schedule_timeout() with msleep()/ssleep_interruptible() Nishanth Aravamudan
1 sibling, 1 reply; 12+ messages in thread
From: Nishanth Aravamudan @ 2004-11-01 18:31 UTC (permalink / raw)
To: Jeff Garzik; +Cc: janitor, netdev
On Sun, Oct 31, 2004 at 06:09:34AM -0500, Jeff Garzik wrote:
> janitor@sternwelten.at wrote:
> >@@ -2846,11 +2838,10 @@ static int s2io_ethtool_idnic(struct net
> > sp->id_timer.data = (unsigned long) sp;
> > }
> > mod_timer(&sp->id_timer, jiffies);
> >- set_current_state(TASK_INTERRUPTIBLE);
> > if (data)
> >- schedule_timeout(data * HZ);
> >+ msleep_interruptible(data * 1000);
>
>
> clearly wants ssleep() here
Even though ssleep() is in TASK_UNINTERRUPTIBLE? In cases where the
existing code used TASK_INTERRUPTIBLE, but was sleeping in terms of
seconds, I left it as msleep_interruptible, since ssleep_interruptible()
doesn't exist.
-Nish
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 05/18] net/s2io: replace schedule_timeout() with msleep()
2004-11-01 18:03 ` Jeff Garzik
@ 2004-11-01 19:14 ` Nishanth Aravamudan
0 siblings, 0 replies; 12+ messages in thread
From: Nishanth Aravamudan @ 2004-11-01 19:14 UTC (permalink / raw)
To: Jeff Garzik; +Cc: janitor, netdev
On Mon, Nov 01, 2004 at 01:03:10PM -0500, Jeff Garzik wrote:
> Nishanth Aravamudan wrote:
> >On Sun, Oct 31, 2004 at 06:09:34AM -0500, Jeff Garzik wrote:
> >
> >>janitor@sternwelten.at wrote:
> >>
> >>>@@ -2846,11 +2838,10 @@ static int s2io_ethtool_idnic(struct net
> >>> sp->id_timer.data = (unsigned long) sp;
> >>> }
> >>> mod_timer(&sp->id_timer, jiffies);
> >>>- set_current_state(TASK_INTERRUPTIBLE);
> >>> if (data)
> >>>- schedule_timeout(data * HZ);
> >>>+ msleep_interruptible(data * 1000);
> >>
> >>
> >>clearly wants ssleep() here
> >
> >
> >Even though ssleep() is in TASK_UNINTERRUPTIBLE? In cases where the
> >existing code used TASK_INTERRUPTIBLE, but was sleeping in terms of
> >seconds, I left it as msleep_interruptible, since ssleep_interruptible()
> >doesn't exist.
>
> Right: create ssleep_interruptible() :)
Ok, will do :) I wasn't sure by your e-mail whether you meant to use
ssleep() or create ssleep_interruptible(). Patch will follow shortly.
-Nish
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Add ssleep_interruptible()
2004-10-30 22:42 [patch 05/18] net/s2io: replace schedule_timeout() with msleep() janitor
2004-10-31 11:09 ` Jeff Garzik
@ 2004-11-01 20:07 ` Nishanth Aravamudan
2004-11-17 1:30 ` Nishanth Aravamudan
1 sibling, 1 reply; 12+ messages in thread
From: Nishanth Aravamudan @ 2004-11-01 20:07 UTC (permalink / raw)
To: janitor; +Cc: netdev, jgarzik, linux-kernel, kernel-janitors
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) */
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] net/s2io: replace schedule_timeout() with msleep()/ssleep_interruptible()
2004-10-31 11:09 ` Jeff Garzik
2004-11-01 18:31 ` Nishanth Aravamudan
@ 2004-11-01 20:20 ` Nishanth Aravamudan
1 sibling, 0 replies; 12+ messages in thread
From: Nishanth Aravamudan @ 2004-11-01 20:20 UTC (permalink / raw)
To: Jeff Garzik; +Cc: kernel-janitors, netdev
On Sun, Oct 31, 2004 at 06:09:34AM -0500, Jeff Garzik wrote:
> janitor@sternwelten.at wrote:
> >@@ -2846,11 +2838,10 @@ static int s2io_ethtool_idnic(struct net
> > sp->id_timer.data = (unsigned long) sp;
> > }
> > mod_timer(&sp->id_timer, jiffies);
> >- set_current_state(TASK_INTERRUPTIBLE);
> > if (data)
> >- schedule_timeout(data * HZ);
> >+ msleep_interruptible(data * 1000);
>
>
> clearly wants ssleep() here
Here is this patch. Depends, of course, on the patch sent just now to
lkml/kj which adds ssleep_interruptible to linux/delay.h.
Description: Uses msleep()/msleep_interruptible()/ssleep_interruptible()
[as appropriate] to guarantee the task delays as expected. I decided to
leave the MAX_SCHEDULE_TIMEOUT as is, since the overhead of converting
to seconds/msecs and then back again within the corresponding sleep()
function seemed unnecessary.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
--- 2.6.10-rc1-vanilla/drivers/net/s2io.c 2004-10-30 15:33:29.000000000 -0700
+++ 2.6.10-rc1/drivers/net/s2io.c 2004-11-01 12:19:22.000000000 -0800
@@ -555,8 +555,7 @@ static int initNic(struct s2io_nic *nic)
val64 = 0;
writeq(val64, &bar0->sw_reset);
val64 = readq(&bar0->sw_reset);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 2);
+ msleep(500);
/* Enable Receiving broadcasts */
val64 = readq(&bar0->mac_cfg);
@@ -803,8 +802,7 @@ static int initNic(struct s2io_nic *nic)
dev->name);
return -1;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
time++;
}
@@ -838,8 +836,7 @@ static int initNic(struct s2io_nic *nic)
return -1;
}
time++;
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
}
/* Initializing proper values as Pause threshold into all
@@ -1182,8 +1179,7 @@ static int startNic(struct s2io_nic *nic
writeq(val64, &bar0->mc_rldram_mrs);
val64 = readq(&bar0->mc_rldram_mrs);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 10); /* Delay by around 100 ms. */
+ msleep(100); /* Delay by around 100 ms. */
/* Enabling ECC Protection. */
val64 = readq(&bar0->adapter_control);
@@ -1891,8 +1887,7 @@ int waitForCmdComplete(nic_t * sp)
ret = SUCCESS;
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
if (cnt++ > 10)
break;
}
@@ -1931,15 +1926,13 @@ void s2io_reset(nic_t * sp)
* As of now I'am just giving a 250ms delay and hoping that the
* PCI write to sw_reset register is done by this time.
*/
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 4);
+ msleep(250);
/* Restore the PCI state saved during initializarion. */
pci_restore_state(sp->pdev);
s2io_init_pci(sp);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 4);
+ msleep(250);
/* SXE-002: Configure link and activity LED to turn it off */
subid = sp->pdev->subsystem_device;
@@ -2157,8 +2150,7 @@ int s2io_close(struct net_device *dev)
/* If the device tasklet is running, wait till its done before killing it */
while (atomic_read(&(sp->tasklet_status))) {
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 10);
+ msleep(100);
}
tasklet_kill(&sp->task);
@@ -2169,8 +2161,7 @@ int s2io_close(struct net_device *dev)
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
cnt++;
if (cnt == 10) {
DBG_PRINT(ERR_DBG,
@@ -2846,11 +2837,12 @@ static int s2io_ethtool_idnic(struct net
sp->id_timer.data = (unsigned long) sp;
}
mod_timer(&sp->id_timer, jiffies);
- set_current_state(TASK_INTERRUPTIBLE);
- if (data)
- schedule_timeout(data * HZ);
- else
+ if (data) {
+ ssleep_interruptible(data);
+ } else {
+ set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(MAX_SCHEDULE_TIMEOUT);
+ }
del_timer_sync(&sp->id_timer);
return 0;
@@ -2943,8 +2935,7 @@ static u32 readEeprom(nic_t * sp, int of
data = I2C_CONTROL_GET_DATA(val64);
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
exit_cnt++;
}
@@ -2983,8 +2974,7 @@ static int writeEeprom(nic_t * sp, int o
ret = 0;
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 20);
+ msleep(50);
exit_cnt++;
}
@@ -3256,8 +3246,7 @@ static int s2io_bistTest(nic_t * sp, uin
ret = 0;
break;
}
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 10);
+ msleep(100);
cnt++;
}
@@ -3356,8 +3345,7 @@ static int s2io_rldramTest(nic_t * sp, u
val64 = readq(&bar0->mc_rldram_test_ctrl);
if (val64 & MC_RLDRAM_TEST_DONE)
break;
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 5);
+ msleep(200);
}
if (cnt == 5)
@@ -3373,8 +3361,7 @@ static int s2io_rldramTest(nic_t * sp, u
val64 = readq(&bar0->mc_rldram_test_ctrl);
if (val64 & MC_RLDRAM_TEST_DONE)
break;
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 2);
+ msleep(500);
}
if (cnt == 5)
@@ -3711,8 +3698,7 @@ static void s2io_set_link(unsigned long
/* Allow a small delay for the NICs self initiated
* cleanup to complete.
*/
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ / 10);
+ msleep(100);
val64 = readq(&bar0->adapter_status);
if (verify_xena_quiescence(val64, nic->device_enabled_once)) {
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add ssleep_interruptible()
2004-11-01 20:07 ` [PATCH] Add ssleep_interruptible() Nishanth Aravamudan
@ 2004-11-17 1:30 ` Nishanth Aravamudan
2004-11-22 2:48 ` Horms
0 siblings, 1 reply; 12+ messages in thread
From: Nishanth Aravamudan @ 2004-11-17 1:30 UTC (permalink / raw)
To: janitor; +Cc: netdev, jgarzik, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add ssleep_interruptible()
2004-11-17 1:30 ` Nishanth Aravamudan
@ 2004-11-22 2:48 ` Horms
2004-11-22 17:19 ` Nishanth Aravamudan
0 siblings, 1 reply; 12+ messages in thread
From: Horms @ 2004-11-22 2:48 UTC (permalink / raw)
To: Nishanth Aravamudan
Cc: janitor, netdev, jgarzik, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add ssleep_interruptible()
2004-11-22 2:48 ` Horms
@ 2004-11-22 17:19 ` Nishanth Aravamudan
2004-11-24 2:01 ` Horms
0 siblings, 1 reply; 12+ messages in thread
From: Nishanth Aravamudan @ 2004-11-22 17:19 UTC (permalink / raw)
To: janitor, netdev, jgarzik, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add ssleep_interruptible()
2004-11-22 17:19 ` Nishanth Aravamudan
@ 2004-11-24 2:01 ` Horms
2004-11-24 18:16 ` Nishanth Aravamudan
0 siblings, 1 reply; 12+ messages in thread
From: Horms @ 2004-11-24 2:01 UTC (permalink / raw)
To: Nishanth Aravamudan
Cc: janitor, netdev, jgarzik, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add ssleep_interruptible()
2004-11-24 2:01 ` Horms
@ 2004-11-24 18:16 ` Nishanth Aravamudan
0 siblings, 0 replies; 12+ messages in thread
From: Nishanth Aravamudan @ 2004-11-24 18:16 UTC (permalink / raw)
To: janitor, netdev, jgarzik, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2004-11-24 18:16 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-10-30 22:42 [patch 05/18] net/s2io: replace schedule_timeout() with msleep() janitor
2004-10-31 11:09 ` Jeff Garzik
2004-11-01 18:31 ` Nishanth Aravamudan
2004-11-01 18:03 ` Jeff Garzik
2004-11-01 19:14 ` Nishanth Aravamudan
2004-11-01 20:20 ` [PATCH] net/s2io: replace schedule_timeout() with msleep()/ssleep_interruptible() Nishanth Aravamudan
2004-11-01 20:07 ` [PATCH] Add ssleep_interruptible() Nishanth Aravamudan
2004-11-17 1:30 ` Nishanth Aravamudan
2004-11-22 2:48 ` Horms
2004-11-22 17:19 ` Nishanth Aravamudan
2004-11-24 2:01 ` Horms
2004-11-24 18:16 ` Nishanth Aravamudan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).