* Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()
@ 2004-09-02 6:50 Margit Schubert-While
2004-09-02 8:24 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep() maximilian attems
0 siblings, 1 reply; 7+ messages in thread
From: Margit Schubert-While @ 2004-09-02 6:50 UTC (permalink / raw)
To: netdev; +Cc: janitor
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace
2004-09-02 6:50 [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep() Margit Schubert-While
@ 2004-09-02 8:24 ` maximilian attems
0 siblings, 0 replies; 7+ messages in thread
From: maximilian attems @ 2004-09-02 8:24 UTC (permalink / raw)
To: Margit Schubert-While; +Cc: netdev, kj
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()
@ 2004-09-02 8:24 ` maximilian attems
0 siblings, 0 replies; 7+ messages in thread
From: maximilian attems @ 2004-09-02 8:24 UTC (permalink / raw)
To: Margit Schubert-While; +Cc: netdev, kj
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace
2004-09-02 8:24 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep() maximilian attems
@ 2004-09-02 16:09 ` Nishanth Aravamudan
-1 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2004-09-02 16:09 UTC (permalink / raw)
To: Margit Schubert-While, netdev, kj
[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]
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
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()
@ 2004-09-02 16:09 ` Nishanth Aravamudan
0 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2004-09-02 16:09 UTC (permalink / raw)
To: Margit Schubert-While, netdev, kj
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()
@ 2004-09-02 9:35 Margit Schubert-While
2004-09-02 10:03 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace maximilian attems
0 siblings, 1 reply; 7+ messages in thread
From: Margit Schubert-While @ 2004-09-02 9:35 UTC (permalink / raw)
To: netdev; +Cc: janitor
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace
2004-09-02 9:35 Margit Schubert-While
@ 2004-09-02 10:03 ` maximilian attems
2004-09-02 16:23 ` Nishanth Aravamudan
0 siblings, 1 reply; 7+ messages in thread
From: maximilian attems @ 2004-09-02 10:03 UTC (permalink / raw)
To: Margit Schubert-While; +Cc: netdev, kj
[-- Attachment #1: Type: text/plain, Size: 460 bytes --]
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/
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace
2004-09-02 10:03 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace maximilian attems
@ 2004-09-02 16:23 ` Nishanth Aravamudan
0 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2004-09-02 16:23 UTC (permalink / raw)
To: Margit Schubert-While, netdev, kj
[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]
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
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2004-09-02 16:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-09-02 6:50 [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep() Margit Schubert-While
2004-09-02 8:24 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace maximilian attems
2004-09-02 8:24 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep() maximilian attems
2004-09-02 16:09 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace Nishanth Aravamudan
2004-09-02 16:09 ` [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep() Nishanth Aravamudan
-- strict thread matches above, loose matches on Subject: below --
2004-09-02 9:35 Margit Schubert-While
2004-09-02 10:03 ` [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace maximilian attems
2004-09-02 16:23 ` Nishanth Aravamudan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.