2004-10-22 22:41:14

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] pmac_cpufreq msleep cleanup/fixes

On Sat, 2004-10-23 at 04:11, Linux Kernel Mailing List wrote:
> ChangeSet 1.2015, 2004/10/22 11:11:36-07:00, [email protected]
>
> [PATCH] pmac_cpufreq msleep cleanup/fixes
>
> Uses msleep() instead of schedule_timeout() to guarantee the task delays
> as expected. Two of the changes are reworks of previous msleep() calls
> which unnecessarily added a jiffy to the parameter.
>
> Signed-off-by: Nishanth Aravamudan <[email protected]>
> Signed-off-by: Linus Torvalds <[email protected]>

Please revert that change until we have made absolutely sure that msleep(1)
on a HZ=1000 machine will actually sleep at least 1ms, this is really not
clear since it will end up doing schedule_timeout(1) which, afaik, will
only guarantee to sleep up to the next jiffie, which can be a lot shorter
than the actual duration of a jiffie.

Ben.



2004-10-22 22:54:47

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] pmac_cpufreq msleep cleanup/fixes



On Sat, 23 Oct 2004, Benjamin Herrenschmidt wrote:
>
> Please revert that change until we have made absolutely sure that msleep(1)
> on a HZ=1000 machine will actually sleep at least 1ms, this is really not
> clear since it will end up doing schedule_timeout(1) which, afaik, will
> only guarantee to sleep up to the next jiffie, which can be a lot shorter
> than the actual duration of a jiffie.

In that case I'd much prefer to revert the whole previous "cleanup" as
well, since it obviously isn't really. Having

msleep(1 + jiffy_to_ms(1));

is just not a cleanup to me.

Linus

2004-10-22 23:26:18

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] pmac_cpufreq msleep cleanup/fixes

On Sat, 2004-10-23 at 08:53, Linus Torvalds wrote:
> On Sat, 23 Oct 2004, Benjamin Herrenschmidt wrote:
> >
> > Please revert that change until we have made absolutely sure that msleep(1)
> > on a HZ=1000 machine will actually sleep at least 1ms, this is really not
> > clear since it will end up doing schedule_timeout(1) which, afaik, will
> > only guarantee to sleep up to the next jiffie, which can be a lot shorter
> > than the actual duration of a jiffie.
>
> In that case I'd much prefer to revert the whole previous "cleanup" as
> well, since it obviously isn't really. Having
>
> msleep(1 + jiffy_to_ms(1));
>
> is just not a cleanup to me.

This wasn't a cleanup but a bug fix actually ... Oh well, I think we need
to fix msleep() instead, what do you think ? If we keep Nishanth's latest
cleanup and fix msleep to add +1 to the delay, that would work and potentially
fix other users as well ... provided my theory is right in the first place
and that schedule_timeout(1) will indeed only sleep until the next jiffy and
not for at least one jiffy...

What about something like this ?

---

Makes sure msleep() sleeps at least the amount provided, since
schedule_timeout() doesn't guarantee a full jiffy.

Signed-off-by: Benjamin Herrenschmidt <[email protected]>

===== kernel/timer.c 1.100 vs edited =====
--- 1.100/kernel/timer.c 2004-10-19 19:40:28 +10:00
+++ edited/kernel/timer.c 2004-10-23 09:16:10 +10:00
@@ -1605,7 +1605,7 @@
*/
void msleep(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs);
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1;

while (timeout) {
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -1621,7 +1621,7 @@
*/
unsigned long msleep_interruptible(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs);
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1;

while (timeout && !signal_pending(current)) {
set_current_state(TASK_INTERRUPTIBLE);


2004-10-22 23:46:50

by Nishanth Aravamudan

[permalink] [raw]
Subject: Re: [PATCH] pmac_cpufreq msleep cleanup/fixes

On Sat, Oct 23, 2004 at 09:17:33AM +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2004-10-23 at 08:53, Linus Torvalds wrote:
> > On Sat, 23 Oct 2004, Benjamin Herrenschmidt wrote:
> > >
> > > Please revert that change until we have made absolutely sure that msleep(1)
> > > on a HZ=1000 machine will actually sleep at least 1ms, this is really not
> > > clear since it will end up doing schedule_timeout(1) which, afaik, will
> > > only guarantee to sleep up to the next jiffie, which can be a lot shorter
> > > than the actual duration of a jiffie.
> >
> > In that case I'd much prefer to revert the whole previous "cleanup" as
> > well, since it obviously isn't really. Having
> >
> > msleep(1 + jiffy_to_ms(1));
> >
> > is just not a cleanup to me.
>
> This wasn't a cleanup but a bug fix actually ... Oh well, I think we need
> to fix msleep() instead, what do you think ? If we keep Nishanth's latest
> cleanup and fix msleep to add +1 to the delay, that would work and potentially
> fix other users as well ... provided my theory is right in the first place
> and that schedule_timeout(1) will indeed only sleep until the next jiffy and
> not for at least one jiffy...
>
> What about something like this ?
>

Looks good to me... And makes quite a bit of sense, after I thought
about it. Does feel a little hacky, but I don't see a slicker way around
the problem...


Makes sure msleep() sleeps at least the amount provided, since
schedule_timeout() doesn't guarantee a full jiffy.

Signed-off-by: Benjamin Herrenschmidt <[email protected]>
Acked-by: Nishanth Aravamudan <[email protected]>

===== kernel/timer.c 1.100 vs edited =====
--- 1.100/kernel/timer.c 2004-10-19 19:40:28 +10:00
+++ edited/kernel/timer.c 2004-10-23 09:16:10 +10:00
@@ -1605,7 +1605,7 @@
*/
void msleep(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs);
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1;

while (timeout) {
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -1621,7 +1621,7 @@
*/
unsigned long msleep_interruptible(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs);
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1;

while (timeout && !signal_pending(current)) {
set_current_state(TASK_INTERRUPTIBLE);

2004-10-22 23:55:07

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] pmac_cpufreq msleep cleanup/fixes



On Sat, 23 Oct 2004, Benjamin Herrenschmidt wrote:
>
> This wasn't a cleanup but a bug fix actually ... Oh well, I think we need
> to fix msleep() instead, what do you think ?

I agree.

Linus

2004-10-23 01:24:25

by Paul Mackerras

[permalink] [raw]
Subject: Re: [PATCH] pmac_cpufreq msleep cleanup/fixes

Nishanth Aravamudan writes:

> Looks good to me... And makes quite a bit of sense, after I thought
> about it. Does feel a little hacky, but I don't see a slicker way around
> the problem...

We would need a platform-specific function to tell us how long until
the next jiffy to do any better, I think, and even then it would only
make much of a difference if HZ was significantly smaller than 1000.

We could do the how-long-until-next-jiffy function quite easily and
quickly on ppc and ppc64, just by reading the decrementer register and
scaling it. I don't know about other architectures. But if we are
mostly using HZ=1000 there doesn't seem to be much point.

Paul.

2004-10-25 00:21:57

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: [PATCH][RESEND] Fix msleep to sleep _at_least_ the requested amount

Hi Linus !

You "agreed" but didn't apply the patch on the last message ... so here
it is again.


Makes sure msleep() sleeps at least the amount provided, since
schedule_timeout() doesn't guarantee a full jiffy.

Signed-off-by: Benjamin Herrenschmidt <[email protected]>

===== kernel/timer.c 1.100 vs edited =====
--- 1.100/kernel/timer.c 2004-10-19 19:40:28 +10:00
+++ edited/kernel/timer.c 2004-10-23 09:16:10 +10:00
@@ -1605,7 +1605,7 @@
*/
void msleep(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs);
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1;

while (timeout) {
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -1621,7 +1621,7 @@
*/
unsigned long msleep_interruptible(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs);
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1;

while (timeout && !signal_pending(current)) {
set_current_state(TASK_INTERRUPTIBLE);