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 <[email protected]>
---
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);
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 <[email protected]>
> ---
> 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...
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?
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
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.
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
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.
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 <[email protected]>
>>---
>>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
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
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