Skip to contentSkip to conversation

net/s2io: replace schedule_timeout() with msleep()

PATCH In review [email protected] 4 participants 12 messages Oct 30, 2004 – Nov 24, 2004

Subsystems (4)
Networking Network drivers Kernel janitors High-resolution timers, timer wheel, clockevents
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 5/18 net/s2io: replace schedule_timeout() with msleep()

Any comments would be appreciated.

Description: Use msleep() instead of schedule_timeout() to guarantee the task delays as expected.

Signed-off-by Nishanth Aravamudan <[email protected]>
Signed-off-by Maximilian Attems <[email protected]>

---

 linux-2.6.10-rc1-max/drivers/net/s2io.c |   47 ++++++++++----------------------
 1 files changed, 16 insertions(+), 31 deletions(-)
drivers/net/s2io.c renamed from drivers/net/s2io.c~msleep-drivers_net_s2io +16 -31
diff -puN drivers/net/s2io.c~msleep-drivers_net_s2io drivers/net/s2io.c
@@ -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)) {

_

#1 lore raw

drivers/net/s2io.c static int s2io_ethtool_idnic(struct net · line 2842

Quoted 8 lines · [email protected] wrote:
drivers/net/s2io.c renamed from drivers/net/s2io.c~msleep-drivers_net_s2io +1 -2
@@ -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

#2 lore raw
Quoted 12 lines · Jeff Garzik

drivers/net/s2io.c static int s2io_ethtool_idnic(struct net · line 2842

Quoted 8 lines · [email protected] wrote:
drivers/net/s2io.c renamed from drivers/net/s2io.c~msleep-drivers_net_s2io +1 -2
@@ -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

#3 lore raw
Quoted 17 lines · Nishanth Aravamudan
Quoted 13 lines · Jeff Garzik

drivers/net/s2io.c static int s2io_ethtool_idnic(struct net · line 2842

Quoted 8 lines · [email protected] wrote:
drivers/net/s2io.c renamed from drivers/net/s2io.c~msleep-drivers_net_s2io +1 -2
@@ -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

Quoted 4 lines

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

#4 lore raw
Quoted 18 lines · Jeff Garzik
Quoted 17 lines · Nishanth Aravamudan
Quoted 13 lines · Jeff Garzik

drivers/net/s2io.c static int s2io_ethtool_idnic(struct net · line 2842

Quoted 8 lines · [email protected] wrote:
drivers/net/s2io.c renamed from drivers/net/s2io.c~msleep-drivers_net_s2io +1 -2
@@ -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

Quoted 6 lines
Quoted 4 lines

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

#5 lore raw

PATCH 1/1 net/s2io: replace schedule_timeout() with msleep()/ssleep_interruptible()

In reply to Jeff Garzik
Quoted 12 lines · Jeff Garzik

Review static int s2io_ethtool_idnic(struct net · line 2842

Quoted 8 lines · [email protected] wrote:
(quoted diff) +1 -2
@@ -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 <[email protected]>
drivers/net/s2io.c +20 -34
@@ -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)) {

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#6 lore raw

PATCH 1/1 Add ssleep_interruptible()

In reply to [email protected]

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 <[email protected]>
include/linux/delay.h +0 -0
@@ -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) */

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#7 lore raw

Re: [PATCH] Add ssleep_interruptible()

Quoted 13 lines · 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 <[email protected]>

--- 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

Review static inline void ssleep(unsigned int s · line 54

Quoted 9 lines
(quoted diff) +5 -0
@@ -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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#8 lore raw

Re: [PATCH] Add ssleep_interruptible()

Quoted 30 lines · Nishanth Aravamudan
Quoted 13 lines · 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 <[email protected]>

--- 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

Review static inline void ssleep(unsigned int s · line 54

Quoted 9 lines
(quoted diff) +5 -0
@@ -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).

Quoted 6 lines

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 =====

include/linux/delay.h +6 -0
@@ -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) */
Signature · 1 line
Horms

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#9 lore raw

Re: [PATCH] Add ssleep_interruptible()

Quoted 49 lines · Horms
Quoted 30 lines · Nishanth Aravamudan
Quoted 13 lines · 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 <[email protected]>

--- 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

include/linux/delay.h static inline void ssleep(unsigned int s · line 54

Quoted 9 lines
include/linux/delay.h +5 -0
@@ -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).

Quoted 6 lines

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 =====

include/linux/delay.h static inline void ssleep(unsigned int s · line 52

Quoted 7 lines
include/linux/delay.h +4 -0
@@ -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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#10 lore raw

Re: [PATCH] Add ssleep_interruptible()

Quoted 64 lines · Nishanth Aravamudan
Quoted 49 lines · Horms
Quoted 30 lines · Nishanth Aravamudan
Quoted 13 lines · 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 <[email protected]>

--- 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

include/linux/delay.h static inline void ssleep(unsigned int s · line 54

Quoted 9 lines
include/linux/delay.h +5 -0
@@ -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).

Quoted 6 lines

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 =====

include/linux/delay.h static inline void ssleep(unsigned int s · line 52

Quoted 7 lines
include/linux/delay.h +4 -0
@@ -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.

Quoted 7 lines

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.

Signature · 1 line
Horms

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#11 lore raw

Re: [PATCH] Add ssleep_interruptible()

Quoted 70 lines · Horms
Quoted 69 lines · Nishanth Aravamudan
Quoted 49 lines · Horms
Quoted 30 lines · Nishanth Aravamudan
Quoted 13 lines · 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 <[email protected]>

--- 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

include/linux/delay.h static inline void ssleep(unsigned int s · line 54

Quoted 9 lines
include/linux/delay.h +5 -0
@@ -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).

Quoted 6 lines

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 =====

include/linux/delay.h static inline void ssleep(unsigned int s · line 52

Quoted 7 lines
include/linux/delay.h +4 -0
@@ -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.

Quoted 6 lines
Quoted 2 lines

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.

Quoted 4 lines

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors