Replaces yield() with schedule_timeout()

Hi list,

Following what i've said, i'm looking each yield() call to check if it needs to be
replaced.I'll send two more patches today and a status message, if everything
is ok, i'm planning to complete this task on the next week.

Thanks,
Gustavo Franco

@@ -1691,7 +1691,7 @@
        for (counter = 0; counter < 10000; counter++) {
                if (!mbox->m_in.busy)
                        return 0;
-               udelay(100); yield();
+               udelay(100); schedule_timeout(1);
        }
        return -1;              /* give up after 1 second */
 }

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

Hi list,

I guess that it's ok, please apply.

Thanks,
Gustavo Franco

@@ -271,7 +271,7 @@
        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)) {
@@ -295,7 +295,7 @@
        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)) {

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

Hi list,

I guess that it's ok, please apply.

Thanks,
Gustavo Franco

@@ -342,7 +342,7 @@
 sony_sleep(void)
 {
        if (sony535_irq_used <= 0) {    /* poll */
-               yield();
+               schedule_timeout(1);
        } else {        /* Interrupt driven */
                DEFINE_WAIT(wait);
 

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

Hi Domen,

My failure, i haven't checked if the set_current_state() was really needed.I'm working on the TODO item "Calling yield() Considered Harmful" written by Matthew Wilcox.Since set_current_state() isn't cited by him and wasn't by Randy, i'll wait until Randy ask for rediffied patches.Randy?

Thanks,
Gustavo Franco

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

On Sun, 25 Apr 2004 20:10:41 -0300 Gustavo Franco <[email protected]> wrote:

| Domen Puncer wrote:
| 
| >On Sat, Apr 24, 2004 at 03:34:44PM -0300, Gustavo Franco wrote:
| >  
| >
| >>Hi list,
| >>
| >>Following what i've said, i'm looking each yield() call to check if it 
| >>needs to be
| >>replaced.I'll send two more patches today and a status message, if 
| >>everything
| >>is ok, i'm planning to complete this task on the next week.
| >>
| >>Thanks,
| >>Gustavo Franco
| >>
| >>--- drivers/scsi/megaraid.c.orig     2004-04-24 13:54:26.000000000 -0300
| >>+++ drivers/scsi/megaraid.c  2004-04-24 13:54:36.000000000 -0300
| >>@@ -1691,7 +1691,7 @@
| >>       for (counter = 0; counter < 10000; counter++) {
| >>               if (!mbox->m_in.busy)
| >>                       return 0;
| >>-               udelay(100); yield();
| >>+               udelay(100); schedule_timeout(1);
| >>    
| >>
| >
| >This makes no sense to me.
| >schedule without set_current_state (also missing in other patches);
| >Why leave udelay?
| >
| >How about something like:
| >	for (counter = 0; counter < 1000; counter++) {
| >		if (!mbox->m_in.busy)
| >			return 0;
| >		set_current_state(TASK_INTERRUPTIBLE);
| >		schedule_timeout(1);
| >
| >counter < 1000 test is on purpose, to get >= 1 second, not 10.
| >
| >  
| >
| Hi Domen,
| 
| My failure, i haven't checked if the set_current_state() was really 
| needed.I'm working
| on the TODO item "Calling yield() Considered Harmful" written by Matthew 
| Wilcox.Since
| set_current_state() isn't cited by him and wasn't by Randy, i'll wait 
| until Randy ask for rediffied
| patches.Randy?

Yes, looks like Domen is correct. He's right about the source code comment. Then I looked at 25 source files that use schedule_timeout() and all of them set current state first.

I'll update the TODO list with this info.

Thanks, all.

--
~Randy

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

Randy,

So, do you need "rediffied" patches with set_current_state() calls, right?

Thanks,
Gustavo Franco

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

On Sun, 25 Apr 2004 21:47:33 -0300 Gustavo Franco <[email protected]> wrote:

| Randy.Dunlap wrote:
| 
| >| Hi Domen,
| >| 
| >| My failure, i haven't checked if the set_current_state() was really 
| >| needed.I'm working
| >| on the TODO item "Calling yield() Considered Harmful" written by Matthew 
| >| Wilcox.Since
| >| set_current_state() isn't cited by him and wasn't by Randy, i'll wait 
| >| until Randy ask for rediffied
| >| patches.Randy?
| >
| >Yes, looks like Domen is correct.  He's right about the source code comment.
| >Then I looked at 25 source files that use schedule_timeout() and all of
| >them set current state first.
| >
| >I'll update the TODO list with this info.
| >
| >  
| >
| Randy,
| 
| So, do you need "rediffied" patches with set_current_state() calls, right?

Oh, yes, sorry I wasn't clear about that.

Thanks,
--
~Randy

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

	Hi,

hello,

jeff garzik prefers schedule_timeout(1) in the case of the sis900, "that's IMO closer to the intention.."

regards maks

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

On Wed, 28 Apr 2004 23:03:56 +0200 maximilian attems wrote:

| hello,
| 
| On Mon, 26 Apr 2004, Felipe W Damasio wrote:
| 
| > Gustavo Franco wrote:
| > >--- drivers/cdrom/sonycd535.c.orig    2004-04-24 13:46:28.000000000 -0300
| > >+++ drivers/cdrom/sonycd535.c 2004-04-24 13:47:38.000000000 -0300
| > >@@ -342,7 +342,7 @@
| > >sony_sleep(void)
| > >{
| > >       if (sony535_irq_used <= 0) {    /* poll */
| > >-               yield();
| > >+               schedule_timeout(1);
| > 
| > 	I don't think it's that cut-and-dry...shouldn't you use something 
| > relevant to HZ on this? Please see the patch maximilian sent on sis900 
| > net driver.
| 
| jeff garzik prefers schedule_timeout(1) in the case of the sis900,
| "that's IMO closer to the intention.."

Yes, and I've already sent an updated sis900.c patch to Jeff.

--
~Randy

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

Sounds fair, since he's the one whom chooses which patches get in ;)

Anyway, we still need set_current_state before calling schedule_timeout.

Cheers,

Felipe

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