* Re: [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative
@ 2006-10-11 12:58 ` Arjan van de Ven
0 siblings, 0 replies; 20+ messages in thread
From: Arjan van de Ven @ 2006-10-11 12:58 UTC (permalink / raw)
To: Amol Lad; +Cc: linux kernel, kernel Janitors
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...
^ permalink raw reply [flat|nested] 20+ messages in thread* [KJ] most users of msleep_interruptible are broken
2006-10-11 12:58 ` [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative Arjan van de Ven
@ 2006-10-11 14:16 ` Matthew Wilcox
-1 siblings, 0 replies; 20+ messages in thread
From: Matthew Wilcox @ 2006-10-11 14:16 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Amol Lad, kernel Janitors, linux kernel
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?
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 20+ messages in thread* most users of msleep_interruptible are broken
@ 2006-10-11 14:16 ` Matthew Wilcox
0 siblings, 0 replies; 20+ messages in thread
From: Matthew Wilcox @ 2006-10-11 14:16 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Amol Lad, kernel Janitors, linux kernel
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?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [KJ] most users of msleep_interruptible are broken
2006-10-11 14:16 ` Matthew Wilcox
@ 2006-10-11 14:20 ` Pierre Ossman
-1 siblings, 0 replies; 20+ messages in thread
From: Pierre Ossman @ 2006-10-11 14:20 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Arjan van de Ven, Amol Lad, kernel Janitors, linux kernel
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
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: most users of msleep_interruptible are broken
@ 2006-10-11 14:20 ` Pierre Ossman
0 siblings, 0 replies; 20+ messages in thread
From: Pierre Ossman @ 2006-10-11 14:20 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Arjan van de Ven, Amol Lad, kernel Janitors, linux kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [KJ] most users of msleep_interruptible are broken
2006-10-11 14:20 ` Pierre Ossman
@ 2006-10-11 14:53 ` Arjan van de Ven
-1 siblings, 0 replies; 20+ messages in thread
From: Arjan van de Ven @ 2006-10-11 14:53 UTC (permalink / raw)
To: Pierre Ossman; +Cc: Matthew Wilcox, Amol Lad, kernel Janitors, linux kernel
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.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: most users of msleep_interruptible are broken
@ 2006-10-11 14:53 ` Arjan van de Ven
0 siblings, 0 replies; 20+ messages in thread
From: Arjan van de Ven @ 2006-10-11 14:53 UTC (permalink / raw)
To: Pierre Ossman; +Cc: Matthew Wilcox, Amol Lad, kernel Janitors, linux kernel
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [KJ] most users of msleep_interruptible are broken
2006-10-11 14:53 ` Arjan van de Ven
@ 2006-10-11 15:07 ` Pierre Ossman
-1 siblings, 0 replies; 20+ messages in thread
From: Pierre Ossman @ 2006-10-11 15:07 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Matthew Wilcox, Amol Lad, kernel Janitors, linux kernel
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
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [KJ] most users of msleep_interruptible are broken
2006-10-11 14:16 ` Matthew Wilcox
@ 2006-10-11 20:30 ` Pavel Machek
-1 siblings, 0 replies; 20+ messages in thread
From: Pavel Machek @ 2006-10-11 20:30 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Arjan van de Ven, Amol Lad, kernel Janitors, linux kernel
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.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: most users of msleep_interruptible are broken
@ 2006-10-11 20:30 ` Pavel Machek
0 siblings, 0 replies; 20+ messages in thread
From: Pavel Machek @ 2006-10-11 20:30 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Arjan van de Ven, Amol Lad, kernel Janitors, linux kernel
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [KJ] [PATCH] drivers/mmc/mmc.c: Replacing yield() with a
2006-10-11 12:58 ` [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative Arjan van de Ven
@ 2006-10-12 5:54 ` Nick Piggin
-1 siblings, 0 replies; 20+ messages in thread
From: Nick Piggin @ 2006-10-12 5:54 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Amol Lad, linux kernel, kernel Janitors
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
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative
@ 2006-10-12 5:54 ` Nick Piggin
0 siblings, 0 replies; 20+ messages in thread
From: Nick Piggin @ 2006-10-12 5:54 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Amol Lad, linux kernel, kernel Janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [KJ] [PATCH] drivers/mmc/mmc.c: Replacing yield() with a
2006-10-12 5:54 ` [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative Nick Piggin
@ 2006-10-16 6:49 ` Pierre Ossman
-1 siblings, 0 replies; 20+ messages in thread
From: Pierre Ossman @ 2006-10-16 6:49 UTC (permalink / raw)
To: Nick Piggin; +Cc: Arjan van de Ven, Amol Lad, linux kernel, kernel Janitors
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
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] drivers/mmc/mmc.c: Replacing yield() with a better alternative
@ 2006-10-16 6:49 ` Pierre Ossman
0 siblings, 0 replies; 20+ messages in thread
From: Pierre Ossman @ 2006-10-16 6:49 UTC (permalink / raw)
To: Nick Piggin; +Cc: Arjan van de Ven, Amol Lad, linux kernel, kernel Janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread