Skip to contentSkip to conversation

Get rid of yield()

PATCH In review Masoud Sharbiani 3 participants 7 messages Oct 13, 2005 – Oct 14, 2005

Subsystems (9)
Network drivers SCSI USB Kernel janitors +5 more subsystems
lore
Share & tools
lore

Masoud Sharbiani

author
#0 lore raw

PATCH 1/1 Get rid of yield()

Hello,
the following patch removes yield() from all drivers. They have been replaced by schedule_timeout() or cond_resched(). Please verify and comment.

Signed-off-by Masoud A Sharbiani <[email protected]>
drivers/cdrom/cdu31a.c +1 -1
@@ -371,7 +371,7 @@ static inline void disable_interrupts(vo
 static inline void sony_sleep(void)
 {
 	if (cdu31a_irq <= 0) {
-		yield();
+		schedule_timeout(1);
 	} else {		/* Interrupt driven */
 		DEFINE_WAIT(w);
 		int first = 1;
drivers/cdrom/sonycd535.c +1 -1
@@ -342,7 +342,7 @@ static inline void
 sony_sleep(void)
 {
 	if (sony535_irq_used <= 0) {	/* poll */
-		yield();
+		schedule_timeout(1);
 	} else {	/* Interrupt driven */
 		DEFINE_WAIT(wait);
drivers/macintosh/adb.c +1 -1
@@ -556,7 +556,7 @@ adb_unregister(int index)
 	if (adb_handler[index].handler) {
 		while(adb_handler[index].busy) {
 			write_unlock_irq(&adb_handler_lock);
-			yield();
+			schedule_timeout(1);
 			write_lock_irq(&adb_handler_lock);
 		}
 		ret = 0;
drivers/mmc/mmc.c +1 -1
@@ -396,7 +396,7 @@ static void mmc_deselect_cards(struct mm
 static inline void mmc_delay(unsigned int ms)
 {
 	if (ms < HZ / 1000) {
-		yield();
+		cond_resched();
 		mdelay(ms);
 	} else {
 		msleep_interruptible (ms);
drivers/net/depca.c +1 -3
@@ -762,9 +762,7 @@ static int __init depca_hw_init (struct 
 		/* Trigger an initialization just for the interrupt. */
 		outw(INEA | INIT, DEPCA_DATA);
 
-		delay = jiffies + HZ/50;
-		while (time_before(jiffies, delay))
-			yield();
+		schedule_timeout(HZ/50);
 
 		irqnum = probe_irq_off(irq_mask);
drivers/net/sb1000.c +2 -2
@@ -264,7 +264,7 @@ card_wait_for_busy_clear(const int ioadd
 	timeout = jiffies + TimeOutJiffies;
 	while (a & 0x80 || a & 0x40) {
 		/* a little sleep */
-		yield();
+		schedule_timeout(1);
 
 		a = inb(ioaddr[0] + 7);
 		if (time_after_eq(jiffies, timeout)) {

@@ -288,7 +288,7 @@ card_wait_for_ready(const int ioaddr[], 
 	timeout = jiffies + TimeOutJiffies;
 	while (a & 0x80 || !(a & 0x40)) {
 		/* a little sleep */
-		yield();
+		schedule_timeout(1);
 
 		a = inb(ioaddr[1] + 6);
 		if (time_after_eq(jiffies, timeout)) {
drivers/net/sis900.c +1 -1
@@ -654,7 +654,7 @@ static int __init sis900_mii_probe(struc
 
 	if(status & MII_STAT_LINK){
 		while (poll_bit) {
-			yield();
+			schedule_timeout(1);
 
 			poll_bit ^= (mdio_read(net_dev, sis_priv->cur_phy, MII_STATUS) & poll_bit);
 			if (time_after_eq(jiffies, timeout)) {
drivers/net/smc91x.c +1 -1
@@ -1073,7 +1073,7 @@ static void smc_phy_powerdown(struct net
 	   above). linkwatch_event() also wants the netlink semaphore.
 	*/
 	while(lp->work_pending)
-		yield();
+		schedule_timeout(1);
 
 	bmcr = smc_phy_read(dev, phy, MII_BMCR);
 	smc_phy_write(dev, phy, MII_BMCR, bmcr | BMCR_PDOWN);
drivers/net/sungem.c +2 -2
@@ -2397,7 +2397,7 @@ static int gem_suspend(struct pci_dev *p
 
 	/* Wait for a pending reset task to complete */
 	while (gp->reset_task_pending)
-		yield();
+		sched_timeout(1);
 	flush_scheduled_work();
 
 	/* Shut the PHY down eventually and setup WOL */

@@ -2922,7 +2922,7 @@ static void __devexit gem_remove_one(str
 
 		/* Wait for a pending reset task to complete */
 		while (gp->reset_task_pending)
-			yield();
+			schedule_timeout(1);
 		flush_scheduled_work();
 
 		/* Shut the PHY down */
drivers/net/wireless/hostap/hostap_hw.c +2 -2
@@ -995,7 +995,7 @@ static u16 hfa384x_allocate_fid(struct n
 	delay = jiffies + HFA384X_ALLOC_COMPL_TIMEOUT;
 	while (!(HFA384X_INW(HFA384X_EVSTAT_OFF) & HFA384X_EV_ALLOC) &&
 	       time_before(jiffies, delay))
-		yield();
+		schedule_timeout(1);
 	if (!(HFA384X_INW(HFA384X_EVSTAT_OFF) & HFA384X_EV_ALLOC)) {
 		printk("%s: fid allocate, len=%d - timeout\n", dev->name, len);
 		return 0xffff;

@@ -1332,7 +1332,7 @@ static int prism2_hw_init(struct net_dev
 	delay = jiffies + HFA384X_INIT_TIMEOUT;
 	while (!(HFA384X_INW(HFA384X_EVSTAT_OFF) & HFA384X_EV_CMD) &&
 	       time_before(jiffies, delay))
-		yield();
+		schedule_timeout(1);
 	if (!(HFA384X_INW(HFA384X_EVSTAT_OFF) & HFA384X_EV_CMD)) {
 		printk(KERN_DEBUG "%s: assuming no Primary image in "
 		       "flash - card initialization not completed\n",
drivers/scsi/NCR5380.c +1 -1
@@ -346,7 +346,7 @@ static int NCR5380_poll_politely(struct 
 		if((r & bit) == val)
 			return 0;
 		if(!in_interrupt())
-			yield();
+			schedule_timeout(1);
 		else
 			cpu_relax();
 	}
drivers/scsi/ibmmca.c +2 -2
@@ -2196,7 +2196,7 @@ static int __ibmmca_abort(Scsi_Cmnd * cm
 #endif
 	spin_unlock_irq(shpnt->host_lock);
 	while (!cmd->SCp.Status)
-		yield();
+		schedule_timeout(1);
 	spin_lock_irq(shpnt->host_lock);
 	cmd->scsi_done = saved_done;
 #ifdef IM_DEBUG_PROBE

@@ -2275,7 +2275,7 @@ static int __ibmmca_host_reset(Scsi_Cmnd
 		if (!(inb(IM_STAT_REG(host_index)) & IM_BUSY))
 			break;
 		spin_unlock_irq(shpnt->host_lock);
-		yield();
+		schedule_timeout(1);
 		spin_lock_irq(shpnt->host_lock);
 	}
 	/*write registers and enable system interrupts */
drivers/scsi/megaraid.c +1 -1
@@ -1750,7 +1750,7 @@ __mega_busywait_mbox (adapter_t *adapter
 	for (counter = 0; counter < 10000; counter++) {
 		if (!mbox->m_in.busy)
 			return 0;
-		udelay(100); yield();
+		udelay(100); cond_resched();
 	}
 	return -1;		/* give up after 1 second */
 }
drivers/usb/core/message.c +1 -1
@@ -477,7 +477,7 @@ void usb_sg_wait (struct usb_sg_request 
 			io->urbs[i]->dev = NULL;
 			retval = 0;
 			i--;
-			yield ();
+			schedule_timeout(1);
 			break;
 
 			/* no error? continue immediately.

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

#1 lore raw
Quoted 3 lines · Masoud Sharbiani

Hello,
the following patch removes yield() from all drivers. They have been replaced by schedule_timeout() or cond_resched(). Please verify and comment.

Which entry in the TODO is this in reference to?

schedule_timeout() should either be schedule_interruptible_timeout() or schedule_uninterruptible_timeout() now. Calling just schedule_timeout() without setting the state is a noop and will cause loops to busy wait.

Have you boot-tested any of the affected drivers to make sure the behavior is the same?

Thanks,
Nish

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

#2 lore raw
In reply to Masoud Sharbiani
Quoted 2 lines · Masoud Sharbiani

Hello,
the following patch removes yield() from all drivers. They have been replaced by schedule_timeout() or cond_resched(). Please verify and comment.

You didn't even compile-test these, did you?

Quoted 4 lines

drivers/net/depca.c static int __init depca_hw_init (struct · line 768

Quoted 10 lines
drivers/net/depca.c +1 -3
@@ -762,9 +762,7 @@ static int __init depca_hw_init (struct 
 		/* Trigger an initialization just for the interrupt. */
 		outw(INEA | INIT, DEPCA_DATA);
 
-		delay = jiffies + HZ/50;
-		while (time_before(jiffies, delay))
-			yield();
+		schedule_timeout(HZ/50);
 
 		irqnum = probe_irq_off(irq_mask);

This one adds a warning -- the delay variable is no longer used.

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

#3 lore raw
In reply to Masoud Sharbiani
Quoted 18 lines · Matthew Wilcox
Quoted 2 lines · Masoud Sharbiani

Hello,
the following patch removes yield() from all drivers. They have been replaced by schedule_timeout() or cond_resched(). Please verify and comment.

You didn't even compile-test these, did you?

Quoted 12 lines
drivers/net/depca.c +1 -3
@@ -762,9 +762,7 @@ static int __init depca_hw_init (struct
              /* Trigger an initialization just for the interrupt. */
              outw(INEA | INIT, DEPCA_DATA);
 
-             delay = jiffies + HZ/50;
-             while (time_before(jiffies, delay))
-                     yield();
+             schedule_timeout(HZ/50);
Quoted 4 lines

drivers/net/depca.c static int __init depca_hw_init (struct · line 768

Quoted 2 lines
drivers/net/depca.c +0 -0
              irqnum = probe_irq_off(irq_mask);

This one adds a warning -- the delay variable is no longer used.

And from a functional perspective, this is clearly a case where there is only one state that will match how the old code worked -- TASK_UNINTERRUPTIBLE. If one were to use TASK_INTERRUPTIBLE, there is no guarantee that a single schedule_timeout(HZ/50) call would last the full request time, so you'd need to loop there as well.

Thanks,
Nish

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

#4 lore raw
In reply to Masoud Sharbiani
Quoted 5 lines · Nish Aravamudan

And from a functional perspective, this is clearly a case where there is only one state that will match how the old code worked -- TASK_UNINTERRUPTIBLE. If one were to use TASK_INTERRUPTIBLE, there is no guarantee that a single schedule_timeout(HZ/50) call would last the full request time, so you'd need to loop there as well.

Do you think it's worth putting a WARN_ON(current->state == TASK_RUNNING) into schedule_timeout(), or are there legitimate reasons for occasionally calling schedule_timeout() with the task in TASK_RUNNING?

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

author
#5 lore raw
In reply to Masoud Sharbiani
Quoted 11 lines · Nish Aravamudan
Quoted 3 lines · Masoud Sharbiani

Hello,
the following patch removes yield() from all drivers. They have been replaced by schedule_timeout() or cond_resched(). Please verify and comment.

Which entry in the TODO is this in reference to?

schedule_timeout() should either be schedule_interruptible_timeout() or schedule_uninterruptible_timeout() now. Calling just schedule_timeout() without setting the state is a noop and will cause loops to busy wait.

D'oh! Will correct and resend.
cheers,
Masoud

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

#6 lore raw
In reply to Masoud Sharbiani
Quoted 10 lines · Matthew Wilcox
Quoted 5 lines · Nish Aravamudan

And from a functional perspective, this is clearly a case where there is only one state that will match how the old code worked -- TASK_UNINTERRUPTIBLE. If one were to use TASK_INTERRUPTIBLE, there is no guarantee that a single schedule_timeout(HZ/50) call would last the full request time, so you'd need to loop there as well.

Do you think it's worth putting a WARN_ON(current->state = TASK_RUNNING) into schedule_timeout(), or are there legitimate reasons for occasionally calling schedule_timeout() with the task in TASK_RUNNING?

I don't think there are any callers currently that are so broken -- I fixed the ones I found, at least, with the patchsets to use msleep() et al. I try to keep my eye on it, but haven't had much time for KJ patches lately.

And now that we have schedule_timeout_interruptible() and schedule_timeout_uninterruptible(), the number of direct callers of schedule_timeout() should be pretty small (and get smaller with time).

There is *no* reason, though, for calling schedule_timeout() with TASK_RUNNING, that I can think of. I guess one could design code that might or might not sleep depending on some variable and set the state to TASK_{UN,}INTERRUPTIBLE or TASK_RUNNING, respectively, and then call schedule_timeout(). That would be nasty code, though, and probably would never have been accepted by the community :)

Thanks,
Nish

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