Mailing List Archive: Linux: Kernel

[PATCH] UBI: Replace yield() with cond_resched()

Index | Next | Previous | View Threaded

richard at nod

May 8, 2012, 1:26 AM

Post #1 of 6 (85 views)
Permalink

[PATCH] UBI: Replace yield() with cond_resched()

The comment above yield()'s definition says:
* If you want to use yield() to wait for something, use wait_event().
* If you want to use yield() to be 'nice' for others, use cond_resched().
* If you still want to use yield(), do not!

The yield() usage of UBI really looks like the "be nice for others" case,
so use cond_resched().

Signed-off-by: Richard Weinberger <richard [at] nod>
CC: peterz [at] infradead
---
drivers/mtd/ubi/io.c | 6 +++---
drivers/mtd/ubi/wl.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 43f1a00..a125ef6 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -192,7 +192,7 @@ retry:
dbg_io("error %d%s while reading %d bytes from PEB "
"%d:%d, read only %zd bytes, retry",
err, errstr, len, pnum, offset, read);
- yield();
+ cond_resched();
goto retry;
}

@@ -366,7 +366,7 @@ retry:
if (retries++ < UBI_IO_RETRIES) {
dbg_io("error %d while erasing PEB %d, retry",
err, pnum);
- yield();
+ cond_resched();
goto retry;
}
ubi_err("cannot erase PEB %d, error %d", pnum, err);
@@ -384,7 +384,7 @@ retry:
if (ei.state == MTD_ERASE_FAILED) {
if (retries++ < UBI_IO_RETRIES) {
dbg_io("error while erasing PEB %d, retry", pnum);
- yield();
+ cond_resched();
goto retry;
}
ubi_err("cannot erase PEB %d", pnum);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 7c1a9bf..f2f9056 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1243,7 +1243,7 @@ retry:
*/
spin_unlock(&ubi->wl_lock);
dbg_wl("the PEB %d is not in proper tree, retry", pnum);
- yield();
+ cond_resched();
goto retry;
}

--
1.7.6.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

peterz at infradead

May 8, 2012, 1:31 AM

Post #2 of 6 (81 views)
Permalink

Re: [PATCH] UBI: Replace yield() with cond_resched() [In reply to]

dedekind1 at gmail

May 8, 2012, 2:02 AM

Post #3 of 6 (81 views)
Permalink

Re: [PATCH] UBI: Replace yield() with cond_resched() [In reply to]

On Tue, 2012-05-08 at 10:26 +0200, Richard Weinberger wrote:
> The comment above yield()'s definition says:
> * If you want to use yield() to wait for something, use wait_event().
> * If you want to use yield() to be 'nice' for others, use cond_resched().
> * If you still want to use yield(), do not!
>
> The yield() usage of UBI really looks like the "be nice for others" case,
> so use cond_resched().
>
> Signed-off-by: Richard Weinberger <richard [at] nod>
> CC: peterz [at] infradead

I guess we would better sleep for a couple of hundreds of milliseconds
instead.

The purpose was different. If we have I/O error, we hope it is a
transient failure. E.g., the HW is temporary unavailable because of an
internal issue. And with yield() we hoped to schedule away for longer
time than usual and let other processes which may affect that HW go
forward and do something.

But whole this thing does not have any solid real-case basis, just
something which looked sane to do to me.

Someone on the list also questioned the entire re-tries, but he did not
care enough to send a tested patch. But this is a separate and
independent issue.

--
Best Regards,
Artem Bityutskiy

Attachments:

signature.asc (0.82 KB)

richard at nod

May 8, 2012, 3:59 AM

Post #4 of 6 (78 views)
Permalink

Re: [PATCH] UBI: Replace yield() with cond_resched() [In reply to]

Am 08.05.2012 11:02, schrieb Artem Bityutskiy:
> On Tue, 2012-05-08 at 10:26 +0200, Richard Weinberger wrote:
>> The comment above yield()'s definition says:
>> * If you want to use yield() to wait for something, use wait_event().
>> * If you want to use yield() to be 'nice' for others, use cond_resched().
>> * If you still want to use yield(), do not!
>>
>> The yield() usage of UBI really looks like the "be nice for others" case,
>> so use cond_resched().
>>
>> Signed-off-by: Richard Weinberger <richard [at] nod>
>> CC: peterz [at] infradead
>
> I guess we would better sleep for a couple of hundreds of milliseconds
> instead.

So, you'd prefer a msleep_interruptible(500) instead of yield() in this case?

> The purpose was different. If we have I/O error, we hope it is a
> transient failure. E.g., the HW is temporary unavailable because of an
> internal issue. And with yield() we hoped to schedule away for longer
> time than usual and let other processes which may affect that HW go
> forward and do something.

Okay, yield() is clearly the wrong choice here.
cond_resched() is better.

Thanks,
//richard

Attachments:

signature.asc (0.48 KB)

peterz at infradead

May 8, 2012, 6:22 AM

Post #5 of 6 (79 views)
Permalink

Re: [PATCH] UBI: Replace yield() with cond_resched() [In reply to]

On Tue, 2012-05-08 at 16:23 +0300, Artem Bityutskiy wrote:
> On Tue, 2012-05-08 at 12:59 +0200, Richard Weinberger wrote:
> > > The purpose was different. If we have I/O error, we hope it is a
> > > transient failure. E.g., the HW is temporary unavailable because of an
> > > internal issue. And with yield() we hoped to schedule away for longer
> > > time than usual and let other processes which may affect that HW go
> > > forward and do something.
> >
> > Okay, yield() is clearly the wrong choice here.
> > cond_resched() is better.
>
> I do not think it is better. Yes, for I/O I'd suggest
> msleep_interruptible(200) instead.

At least document the intent of that random sleep. Otherwise we'll be
having this same discussion again in a few years or so ;-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

dedekind1 at gmail

May 8, 2012, 6:23 AM

Post #6 of 6 (78 views)
Permalink

Re: [PATCH] UBI: Replace yield() with cond_resched() [In reply to]

On Tue, 2012-05-08 at 12:59 +0200, Richard Weinberger wrote:
> > The purpose was different. If we have I/O error, we hope it is a
> > transient failure. E.g., the HW is temporary unavailable because of an
> > internal issue. And with yield() we hoped to schedule away for longer
> > time than usual and let other processes which may affect that HW go
> > forward and do something.
>
> Okay, yield() is clearly the wrong choice here.
> cond_resched() is better.

I do not think it is better. Yes, for I/O I'd suggest
msleep_interruptible(200) instead.

--
Best Regards,
Artem Bityutskiy

Attachments:

signature.asc (0.82 KB)

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads