[PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative

[PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative

[Amol Lad]

In 2.6, the semantics of calling yield() changed from "sleep for a
bit" to "I really don't want to run for a while".  This matches POSIX
better, but there's a lot of drivers still using yield() when they mean
cond_resched(), schedule() or even schedule_timeout().

For this driver cond_resched() seems to be a better
alternative

Tested compile only

Signed-off-by: Amol Lad <amol@verismonetworks.com>
---
diff -uprN -X linux-2.6.19-rc1-orig/Documentation/dontdiff linux-2.6.19-rc1-orig/drivers/mmc/mmc.c linux-2.6.19-rc1/drivers/mmc/mmc.c
--- linux-2.6.19-rc1-orig/drivers/mmc/mmc.c	2006-10-05 14:00:46.000000000 +0530
+++ linux-2.6.19-rc1/drivers/mmc/mmc.c	2006-10-11 17:57:02.000000000 +0530
@@ -454,7 +454,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);

Re: [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative

[Arjan van de Ven]

On Wed, 2006-10-11 at 18:15 +0530, Amol Lad wrote:
> In 2.6, the semantics of calling yield() changed from "sleep for a
> bit" to "I really don't want to run for a while".  This matches POSIX
> better, but there's a lot of drivers still using yield() when they mean
> cond_resched(), schedule() or even schedule_timeout().
> 
> For this driver cond_resched() seems to be a better
> alternative
> 

are you sure?

> Tested compile only
> 
> Signed-off-by: Amol Lad <amol@verismonetworks.com>
> ---
> diff -uprN -X linux-2.6.19-rc1-orig/Documentation/dontdiff linux-2.6.19-rc1-orig/drivers/mmc/mmc.c linux-2.6.19-rc1/drivers/mmc/mmc.c
> --- linux-2.6.19-rc1-orig/drivers/mmc/mmc.c	2006-10-05 14:00:46.000000000 +0530
> +++ linux-2.6.19-rc1/drivers/mmc/mmc.c	2006-10-11 17:57:02.000000000 +0530
> @@ -454,7 +454,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);


this probably wants msleep(), especially with hrtimers comming up; there
the sleeps are always exact...

most users of msleep_interruptible are broken

[Matthew Wilcox]

On Wed, Oct 11, 2006 at 02:58:11PM +0200, Arjan van de Ven wrote:
> > +++ linux-2.6.19-rc1/drivers/mmc/mmc.c	2006-10-11 17:57:02.000000000 +0530
> > @@ -454,7 +454,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);
> 
> 
> this probably wants msleep(), especially with hrtimers comming up; there
> the sleeps are always exact...

They clearly don't care about exactness; they msleep_interruptible and
throw away the return value, so they don't know how long they slept
before they got a signal.

__must_check treatment for msleep_interruptible, anyone?  On the one hand,
that's 136 new warnings.  On the other hand, that's 136 places wheree
we may as well *delete the call* to msleep_interruptible.  Since it can
return immediately, the code must be prepared to deal with that ... right?

Re: most users of msleep_interruptible are broken

[Pierre Ossman]

Matthew Wilcox wrote:
> 
> They clearly don't care about exactness; they msleep_interruptible and
> throw away the return value, so they don't know how long they slept
> before they got a signal.
> 

It's broken then. That delay function should delay at least the amount
given.

Rgds
Pierre

Re: most users of msleep_interruptible are broken

[Arjan van de Ven]

On Wed, 2006-10-11 at 16:20 +0200, Pierre Ossman wrote:
> Matthew Wilcox wrote:
> > 
> > They clearly don't care about exactness; they msleep_interruptible and
> > throw away the return value, so they don't know how long they slept
> > before they got a signal.
> > 
> 
> It's broken then. That delay function should delay at least the amount
> given.


if you want that don't use the _interruptible variant. It's that simple.

Re: most users of msleep_interruptible are broken

[Pierre Ossman]

Arjan van de Ven wrote:
>
> if you want that don't use the _interruptible variant. It's that simple.
>
>   

Patches welcome ;)

I believe noone has fixed the if-condition yet as well...

Rgds
Pierre

Re: most users of msleep_interruptible are broken

[Pavel Machek]

Hi!

> > > +++ linux-2.6.19-rc1/drivers/mmc/mmc.c	2006-10-11 17:57:02.000000000 +0530
> > > @@ -454,7 +454,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);
> > 
> > 
> > this probably wants msleep(), especially with hrtimers comming up; there
> > the sleeps are always exact...
> 
> They clearly don't care about exactness; they msleep_interruptible and
> throw away the return value, so they don't know how long they slept
> before they got a signal.
> 
> __must_check treatment for msleep_interruptible, anyone?  On the one hand,
> that's 136 new warnings.  On the other hand, that's 136 places wheree
> we may as well *delete the call* to msleep_interruptible.  Since it can
> return immediately, the code must be prepared to deal with that ... right?

Well, it must work, but it may busyloop instead of sleeping. This does
not look like must_check to me.
						Pavel

-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative

[Nick Piggin]

Arjan van de Ven wrote:
> On Wed, 2006-10-11 at 18:15 +0530, Amol Lad wrote:
> 
>>In 2.6, the semantics of calling yield() changed from "sleep for a
>>bit" to "I really don't want to run for a while".  This matches POSIX
>>better, but there's a lot of drivers still using yield() when they mean
>>cond_resched(), schedule() or even schedule_timeout().
>>
>>For this driver cond_resched() seems to be a better
>>alternative
>>
> 
> 
> are you sure?
> 
> 
>>Tested compile only
>>
>>Signed-off-by: Amol Lad <amol@verismonetworks.com>
>>---
>>diff -uprN -X linux-2.6.19-rc1-orig/Documentation/dontdiff linux-2.6.19-rc1-orig/drivers/mmc/mmc.c linux-2.6.19-rc1/drivers/mmc/mmc.c
>>--- linux-2.6.19-rc1-orig/drivers/mmc/mmc.c	2006-10-05 14:00:46.000000000 +0530
>>+++ linux-2.6.19-rc1/drivers/mmc/mmc.c	2006-10-11 17:57:02.000000000 +0530
>>@@ -454,7 +454,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);
> 
> 
> 
> this probably wants msleep(), especially with hrtimers comming up; there
> the sleeps are always exact...

The condition looks broken too. It should be
if (ms < 1000 / HZ) {...}

Shouldn't it?
-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative

[Pierre Ossman]

Nick Piggin wrote:
> 
> The condition looks broken too. It should be
> if (ms < 1000 / HZ) {...}
> 
> Shouldn't it?

Yup. I poked Russell about it ages ago, but he must have forgotten (and
admittedly, so have I). Since MMC is now on my table, I guess I should
put it on my todo list.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org
  OLPC, developer                     http://www.laptop.org

Re: [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative

[Pierre Ossman]

Amol Lad wrote:
> In 2.6, the semantics of calling yield() changed from "sleep for a
> bit" to "I really don't want to run for a while".  This matches POSIX
> better, but there's a lot of drivers still using yield() when they mean
> cond_resched(), schedule() or even schedule_timeout().
> 
> For this driver cond_resched() seems to be a better
> alternative
> 

A version of this patch has been pushed towards Andrew. Thanks for
pointing it out.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org