[Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Gustavo Franco]

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);
        }
        return -1;              /* give up after 1 second */
 }

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Gustavo Franco]

Hi list,

I guess that it's ok, please apply.

Thanks,
Gustavo Franco

--- drivers/net/sb1000.c.orig       2004-04-24 13:46:26.000000000 -0300
+++ drivers/net/sb1000.c    2004-04-24 13:47:01.000000000 -0300
@@ -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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Gustavo Franco]

Hi list,

I guess that it's ok, please apply.

Thanks,
Gustavo Franco


--- 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);
        } else {        /* Interrupt driven */
                DEFINE_WAIT(wait);

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with

[Domen Puncer]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

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.

Comments?


	Domen

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Gustavo Franco]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

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?

Thanks,
Gustavo Franco


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with

[Randy.Dunlap]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On Sun, 25 Apr 2004 20:10:41 -0300 Gustavo Franco <stratus@acm.org> 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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Gustavo Franco]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

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?

Thanks,
Gustavo Franco

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with

[Randy.Dunlap]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

On Sun, 25 Apr 2004 21:47:33 -0300 Gustavo Franco <stratus@acm.org> 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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Felipe W Damasio]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

	Hi,

Gustavo Franco wrote:
> I guess that it's ok, please apply.
> 
> Thanks,
> Gustavo Franco
> 
> 
> --- 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.

	Also, don't forget to use TASK_INTERRUPTITBLE (with a set_current_state 
that uses a memory barrier) before using schedule_timeout, please.

	Cheers,

Felipe

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with

[maximilian attems]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

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.."

regards maks


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with

[Randy.Dunlap]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] Replaces yield() with schedule_timeout()

[Felipe W Damasio]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

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

	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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors