char/ipmi_si_intf: replace schedule_timeout()

Hi,

Sorry, I was off by one in my total.

Please consider applying.

Description: Use msleep() instead of schedule_timeout(). I have chosen to use msleep(10) here, even though the former calls were schedule_timeout(1), as many drivers were not updated to consider the new HZ values. Under HZ==100, the existing calls were 10ms delays, which I have returned to with this patch. If 1 ms was actually desired (or 1 scheduler unit) then the patch can be changed appropriately.

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

drivers/char/ipmi/ipmi_si_intf.c +5 -10

@@ -1880,8 +1880,7 @@ static int try_get_dev_id(struct smi_inf
 	for (;;)
 	{
 		if (smi_result == SI_SM_CALL_WITH_DELAY) {
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			schedule_timeout(1);
+			msleep(10);
 			smi_result = smi_info->handlers->event(
 				smi_info->si_sm, 100);
 		}

@@ -2153,10 +2152,8 @@ static int init_one_smi(int intf_num, st
 
 	/* Wait for the timer to stop.  This avoids problems with race
 	   conditions removing the timer here. */
-	while (!new_smi->timer_stopped) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(1);
-	}
+	while (!new_smi->timer_stopped)
+		msleep(10);
 
 out_err:
 	if (new_smi->intf)

@@ -2280,17 +2277,14 @@ static void __exit cleanup_one_si(struct
 
 	/* Wait for the timer to stop.  This avoids problems with race
 	   conditions removing the timer here. */
-	while (!to_clean->timer_stopped) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(1);
-	}
+	while (!to_clean->timer_stopped)
+		msleep(10);
 

Hi,

Sorry, I was off by one in my total.

Please consider applying.

Description: Use msleep() instead of schedule_timeout(). I have chosen to use msleep(10) here, even though the former calls were schedule_timeout(1), as many drivers were not updated to consider the new HZ values. Under HZ=100, the existing calls were 10ms delays, which I have returned to with this patch. If 1 ms was actually desired (or 1 scheduler unit) then the patch can be changed appropriately.

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

drivers/char/ipmi/ipmi_si_intf.c +5 -10

@@ -1880,8 +1880,7 @@ static int try_get_dev_id(struct smi_inf
 	for (;;)
 	{
 		if (smi_result = SI_SM_CALL_WITH_DELAY) {
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			schedule_timeout(1);
+			msleep(10);
 			smi_result = smi_info->handlers->event(
 				smi_info->si_sm, 100);
 		}

@@ -2153,10 +2152,8 @@ static int init_one_smi(int intf_num, st
 
 	/* Wait for the timer to stop.  This avoids problems with race
 	   conditions removing the timer here. */
-	while (!new_smi->timer_stopped) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(1);
-	}
+	while (!new_smi->timer_stopped)
+		msleep(10);
 
 out_err:
 	if (new_smi->intf)

@@ -2280,17 +2277,14 @@ static void __exit cleanup_one_si(struct
 
 	/* Wait for the timer to stop.  This avoids problems with race
 	   conditions removing the timer here. */
-	while (!to_clean->timer_stopped) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(1);
-	}
+	while (!to_clean->timer_stopped)
+		msleep(10);
 

On my previous question, to be more clear, why was this change necessary? In this particular instance, it doesn't matter how long it sleeps, it just can't spin waiting for something to happen. msleep() spins, right? That would be very bad in this code. Even if not, the particular timing is not important.

Quoted 7 lines · Corey Minyard

On my previous question, to be more clear, why was this change necessary? In this particular instance, it doesn't matter how long it sleeps, it just can't spin waiting for something to happen. msleep() spins, right? That would be very bad in this code. Even if not, the particular timing is not important.

msleep() does not spin. The name itself is intended to indicate this. Delays (mdelay(), udelay()) spin by looping tightly. Sleeps (msleep(), ssleep(), msleep_interruptible()) use schedule_timeout() to give up the CPU for a certain amount of time.

Given this last comment, there is a clear benefit (IMO) to use actual time units for sleeps (msleep() and co. use milliseconds or seconds) as opposed to jiffy-relative units (as in schedule_timeout() where you are requesting a delay in jiffies, which varies from arch to arch).

This becomes very important with dynamic HZ, for instance. Clearly if HZ changes, then the delay caused by schedule_timeout(1) will change, which is not necessarily desired and may be confusing the user. A time specified in real time units will be adjusted with the change in HZ.

If it is not important how long the driver sleeps, then I believe msleep() should be preferred in effectively all cases (depending on whether wait-queue events or signals may be important early triggers, of course). Milliseconds indicate a clear delay, independent of HZ's value. Jiffy delays have a clear reliance on the value of HZ.

I am open to other suggestions, but I think these are good reasons. Another basic one is that it will lead to consistency across the board. HZ is not a measure of time, it should not be used as a measure of time. Instead, milliseconds should be used and, when/if the facility has been added, microseconds and nanoseconds.

Thanks,
Nish

Ok, I was unaware of the difference between msleep and mdelay (I was actually reading "mdelay").

Actually, in the code I want to wait as small a time as possible without spinning. The operations being waited for generally happen in ~500us, which is far too long to spin, but 10ms would be non-optimal if a faster increment was available. So I think it is still best as it is.