Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Margit Schubert-While]

I agree with Jean and add the following :
You are assuming HZ = 1000.
In 2.4, HZ = 100 (And in 2.6, HZ is not necessarily = 1000).
The prism54 code base is identical between 2.4/2.6 and is
maintained as such in the project.
Therefore, I look forward to your implementation of msleep()
in 2.4  ;-)

Margit

[Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[maximilian attems]

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

On Thu, 02 Sep 2004, Margit Schubert-While wrote:

> I agree with Jean and add the following :
> You are assuming HZ = 1000.
> In 2.4, HZ = 100 (And in 2.6, HZ is not necessarily = 1000).

shure, 
but this is an argument for msleep as it uses msecs as unit,
which don't depend on your arch. 
(well physics says different for arch/relativistic :)

> The prism54 code base is identical between 2.4/2.6 and is
> maintained as such in the project.
> Therefore, I look forward to your implementation of msleep()
> in 2.4  ;-)

2.4 is closed for such stuff, you'll know that better than me,
it shouldn't hinder 2.6 in it's progression.
one day you will need to branch.

--
maks
kernel janitor  	http://janitor.kernelnewbies.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: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On Thu, Sep 02, 2004 at 10:24:33AM +0200, maximilian attems wrote:
> On Thu, 02 Sep 2004, Margit Schubert-While wrote:
> 
> > I agree with Jean and add the following :
> > You are assuming HZ = 1000.
> > In 2.4, HZ = 100 (And in 2.6, HZ is not necessarily = 1000).

As the original author of the patches, I feel I should interject . . .
Why/Where do you see an assumption about the value of HZ? The conversion
of the parameter to schedule_timeout() from jiffies to msecs is the only
place I can see where that might appear to be the case. But, upon closer
examination, there is no such assumption:

1000 = the number of milliseconds in a second.
HZ = the number of jiffies in a second (regardless of architecture)

In the original code for prism54/islpci_dev.c:

set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(50*HZ/1000);

Thus, to convert (50*HZ/1000) from jiffies to msecs, multiply by 1000
and divide by HZ, or:

50*HZ/1000 jiffies * 1000/HZ msecs/jiffie = 50 msecs.

And thus, in the patched code, the above becomes:

msleep(50);

Does that clear things up?

-Nish

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Margit Schubert-While]

On Thu, 02 Sep 2004, Maximilian scribeth:
> it shouldn't hinder 2.6 in it's progression.
I consider this a regression.
As schedule_timeout is used elesewhere in the prism54 code,
we are using a consistent and documented method.

Margit

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[maximilian attems]

On Thu, 02 Sep 2004, Margit Schubert-While wrote:

> On Thu, 02 Sep 2004, Maximilian scribeth:
> > it shouldn't hinder 2.6 in it's progression.
> I consider this a regression.
> As schedule_timeout is used elesewhere in the prism54 code,
> we are using a consistent and documented method.

you didn't answer to the unit argument in favour of msleep.

shure msleep is also consistent and documented.

--
maks
kernel janitor  	http://janitor.kernelnewbies.org/

Re: [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On Thu, Sep 02, 2004 at 12:03:22PM +0200, maximilian attems wrote:
> On Thu, 02 Sep 2004, Margit Schubert-While wrote:
> 
> > On Thu, 02 Sep 2004, Maximilian scribeth:
> > > it shouldn't hinder 2.6 in it's progression.
> > I consider this a regression.
> > As schedule_timeout is used elesewhere in the prism54 code,
> > we are using a consistent and documented method.

A grep of drivers/net/wireless/prism54 for schedule_timeout showed three
occurrences (in 2.6.9-rc1-bk7):

islpci_dev.c:   schedule_timeout(50*HZ/1000);
islpci_dev.c:           remaining = schedule_timeout(HZ);
islpci_mgt.c:           timeleft = schedule_timeout(wait_cycle_jiffies);

The first is removed by my patch.

The second & third are potentially bugs as there is no
set_current_state() preceding the call to schedule_timeout(). As per the
source:

/**
 * schedule_timeout - sleep until timeout
 * @timeout: timeout value in jiffies
 *
 * 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()).

Therefore, in the current code, the schedule_timeout() call does not
have the desired effect (the same information is available in
kernel-hacking.ps).

Both of these calls should probably be fixed, but I'm not sure if you
wish to sleep in TASK_INTERUPTIBLE or TASK_UNINTERRUPTIBLE. Keep in mind
that msleep_interruptible() is also (hopefully) being pushed to the
kernel soon.

As to consistency or documentation . . . I have no evidence to suggest
that msleep() is inconsistent. And I don't think there is any need for
more documentation than the source in this case:

/**
 * msleep - sleep safely even with waitqueue interruptions
 * @msecs: Time in milliseconds to sleep for
 */

Hope this helps clear things up.

-Nish