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

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

[Gustavo Franco]

Hi,

I'm new here, and this is my first patch, following the issue described by
Matthew Wilcox: "Calling yield() Considered Harmful" on TODO list.

There is no doubt that isn't necessary replace all yield() calls, but i'm
planning to check each case on the tree.Comments?

Hope that helps,
Gustavo Franco
p.s: This e-mail on the "from" is only for ML traffic, my main e-mail is
stratus at acm.dot org.


--- drivers/cdrom/cdu31a.c.orig 2004-04-13 22:43:55.000000000 -0300
+++ drivers/cdrom/cdu31a.c      2004-04-13 22:44:17.000000000 -0300
@@ -386,7 +386,7 @@ static inline void sony_sleep(void)
        unsigned long flags;
                                                                                         

        if (cdu31a_irq <= 0) {
-               yield();
+               schedule_timeout(1);
        } else {                /* Interrupt driven */
                                                                                         

                save_flags(flags);

_______________________________________________
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: 1358 bytes --]

On Tue, 13 Apr 2004 22:59:10 -0300 Gustavo Franco wrote:

| Hi,
| 
| I'm new here, and this is my first patch, following the issue described by
| Matthew Wilcox: "Calling yield() Considered Harmful" on TODO list.
| 
| There is no doubt that isn't necessary replace all yield() calls, but i'm
| planning to check each case on the tree.Comments?
| 
| Hope that helps,
| Gustavo Franco
| p.s: This e-mail on the "from" is only for ML traffic, my main e-mail is
| stratus at acm.dot org.
| 
| 
| --- drivers/cdrom/cdu31a.c.orig 2004-04-13 22:43:55.000000000 -0300
| +++ drivers/cdrom/cdu31a.c      2004-04-13 22:44:17.000000000 -0300
| @@ -386,7 +386,7 @@ static inline void sony_sleep(void)
|         unsigned long flags;
|                                                                                          
| 
|         if (cdu31a_irq <= 0) {
| -               yield();
| +               schedule_timeout(1);
|         } else {                /* Interrupt driven */
| 
|                 save_flags(flags);

Makes sense, except that the comment above the function bothers me:

/*
 * Wait a little while (used for polling the drive).  If in initialization,
 * setting a timeout doesn't work, so just loop for a while.
 */

I expect that cdu31a_irq <= 0 means "in initializatgion," so is
using schedule_timeout() safe here?  anyone checked this?

--
~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

[Gustavo Franco]

Randy.Dunlap wrote:

>On Tue, 13 Apr 2004 22:59:10 -0300 Gustavo Franco wrote:
>
>| Hi,
>| 
>| I'm new here, and this is my first patch, following the issue described by
>| Matthew Wilcox: "Calling yield() Considered Harmful" on TODO list.
>| 
>| There is no doubt that isn't necessary replace all yield() calls, but i'm
>| planning to check each case on the tree.Comments?
>| 
>| Hope that helps,
>| Gustavo Franco
>| p.s: This e-mail on the "from" is only for ML traffic, my main e-mail is
>| stratus at acm.dot org.
>| 
>| 
>| --- drivers/cdrom/cdu31a.c.orig 2004-04-13 22:43:55.000000000 -0300
>| +++ drivers/cdrom/cdu31a.c      2004-04-13 22:44:17.000000000 -0300
>| @@ -386,7 +386,7 @@ static inline void sony_sleep(void)
>|         unsigned long flags;
>|                                                                                          
>| 
>|         if (cdu31a_irq <= 0) {
>| -               yield();
>| +               schedule_timeout(1);
>|         } else {                /* Interrupt driven */
>| 
>|                 save_flags(flags);
>
>Makes sense, except that the comment above the function bothers me:
>
>/*
> * Wait a little while (used for polling the drive).  If in initialization,
> * setting a timeout doesn't work, so just loop for a while.
> */
>
>I expect that cdu31a_irq <= 0 means "in initializatgion," so is
>using schedule_timeout() safe here?  anyone checked this?
>
>  
>

Good question. Read this bit from comments on the source.

[...]
 * This interface is (unfortunately) a polled interface.  This is
 * because most Sony interfaces are set up with DMA and interrupts
 * disables.  Some (like mine) do not even have the capability to
 * handle interrupts or DMA.  For this reason you will see a lot of
 * the following:
 *
 *   retry_count = jiffies+ SONY_JIFFIES_TIMEOUT;
 *   while (time_before(jiffies, retry_count) && (! <some condition to 
wait for))
 *   {
 *      while (handle_sony_cd_attention())
 *         ;
 *
 *      sony_sleep();
 *   }
 *   if (the condition not met)
 *   {
 *      return an error;
 *   }
 *
 * This ugly hack waits for something to happen, sleeping a little
 * between every try.  it also handles attentions, which are
 * asynchronous events from the drive informing the driver that a disk
 * has been inserted, removed, etc.
[...]

If it's related with initialization why it isn't being called from 
cdu31a_setup() ?
sony_sleep() is called from other functions as described above on the 
comments.


Hope that helps,
Gustavo Franco





_______________________________________________
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: 2829 bytes --]

On Thu, 15 Apr 2004 23:13:30 -0300 Gustavo Franco <stratus@legolas.alternex.com.br> wrote:

| Randy.Dunlap wrote:
| 
| >On Tue, 13 Apr 2004 22:59:10 -0300 Gustavo Franco wrote:
| >
| >| Hi,
| >| 
| >| I'm new here, and this is my first patch, following the issue described by
| >| Matthew Wilcox: "Calling yield() Considered Harmful" on TODO list.
| >| 
| >| There is no doubt that isn't necessary replace all yield() calls, but i'm
| >| planning to check each case on the tree.Comments?
| >| 
| >| Hope that helps,
| >| Gustavo Franco
| >| p.s: This e-mail on the "from" is only for ML traffic, my main e-mail is
| >| stratus at acm.dot org.
| >| 
| >| 
| >| --- drivers/cdrom/cdu31a.c.orig 2004-04-13 22:43:55.000000000 -0300
| >| +++ drivers/cdrom/cdu31a.c      2004-04-13 22:44:17.000000000 -0300
| >| @@ -386,7 +386,7 @@ static inline void sony_sleep(void)
| >|         unsigned long flags;
| >|                                                                                          
| >| 
| >|         if (cdu31a_irq <= 0) {
| >| -               yield();
| >| +               schedule_timeout(1);
| >|         } else {                /* Interrupt driven */
| >| 
| >|                 save_flags(flags);
| >
| >Makes sense, except that the comment above the function bothers me:
| >
| >/*
| > * Wait a little while (used for polling the drive).  If in initialization,
| > * setting a timeout doesn't work, so just loop for a while.
| > */
| >
| >I expect that cdu31a_irq <= 0 means "in initializatgion," so is
| >using schedule_timeout() safe here?  anyone checked this?
| >
| >  
| >
| 
| Good question. Read this bit from comments on the source.
| 
| [...]
|  * This interface is (unfortunately) a polled interface.  This is
|  * because most Sony interfaces are set up with DMA and interrupts
|  * disables.  Some (like mine) do not even have the capability to
|  * handle interrupts or DMA.  For this reason you will see a lot of
|  * the following:
|  *
|  *   retry_count = jiffies+ SONY_JIFFIES_TIMEOUT;
|  *   while (time_before(jiffies, retry_count) && (! <some condition to 
| wait for))
|  *   {
|  *      while (handle_sony_cd_attention())
|  *         ;
|  *
|  *      sony_sleep();
|  *   }
|  *   if (the condition not met)
|  *   {
|  *      return an error;
|  *   }
|  *
|  * This ugly hack waits for something to happen, sleeping a little
|  * between every try.  it also handles attentions, which are
|  * asynchronous events from the drive informing the driver that a disk
|  * has been inserted, removed, etc.
| [...]
| 
| If it's related with initialization why it isn't being called from 
| cdu31a_setup() ?
| sony_sleep() is called from other functions as described above on the 
| comments.

Yes, I looked at it again and I think that it's safe.
I'll put it into the next KJ patchset.

--
~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

[Domen Puncer]

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

...
> | >| --- drivers/cdrom/cdu31a.c.orig 2004-04-13 22:43:55.000000000 -0300
> | >| +++ drivers/cdrom/cdu31a.c      2004-04-13 22:44:17.000000000 -0300
> | >| @@ -386,7 +386,7 @@ static inline void sony_sleep(void)
> | >|         unsigned long flags;
> | >|                                                                                          
> | >| 
> | >|         if (cdu31a_irq <= 0) {
> | >| -               yield();
> | >| +               schedule_timeout(1);

set_current_state() missing.


	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

[Gustavo Franco]

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

Domen Puncer wrote:

>...
>  
>
>>| >| --- drivers/cdrom/cdu31a.c.orig 2004-04-13 22:43:55.000000000 -0300
>>| >| +++ drivers/cdrom/cdu31a.c      2004-04-13 22:44:17.000000000 -0300
>>| >| @@ -386,7 +386,7 @@ static inline void sony_sleep(void)
>>| >|         unsigned long flags;
>>| >|                                                                                          
>>| >| 
>>| >|         if (cdu31a_irq <= 0) {
>>| >| -               yield();
>>| >| +               schedule_timeout(1);
>>    
>>
>
>set_current_state() missing.
>
>
>	Domen
>
>  
>
Domen,

Are you talking about something like:
<http://www.iglu.org.il/lxr/ident?i=set_current_state>
set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(1);

Is is really necessary?

Thanks,
Gustavo Franco -- <stratus@acm.org>

[-- 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

[Domen Puncer]

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

On Sun, Apr 25, 2004 at 07:13:53PM -0300, Gustavo Franco wrote:
> Domen Puncer wrote:
> 
> >...
> > 
> >
> >>| >| --- drivers/cdrom/cdu31a.c.orig 2004-04-13 22:43:55.000000000 -0300
> >>| >| +++ drivers/cdrom/cdu31a.c      2004-04-13 22:44:17.000000000 -0300
> >>| >| @@ -386,7 +386,7 @@ static inline void sony_sleep(void)
> >>| >|         unsigned long flags;
> >>| >|                                                                      
> >>| >| 
> >>| >|         if (cdu31a_irq <= 0) {
> >>| >| -               yield();
> >>| >| +               schedule_timeout(1);
> >>   
> >>
> >
> >set_current_state() missing.
> >
> >
> Are you talking about something like:
> <http://www.iglu.org.il/lxr/ident?i=set_current_state>
> set_current_state(TASK_UNINTERRUPTIBLE);
> schedule_timeout(1);
> 
> Is is really necessary?

Yes, comment above schedule_timeout:
 * Make the current task sleep until @timeout jiffies have
 * elapsed. The routine will return immediately unless
 * the current task state has been set (see set_current_state()).

...the "immediately" part
> 
> Thanks,
> Gustavo Franco -- <stratus@acm.org>

[-- 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