[KJ] Re: [PATCH 18/21] ppc64/rtasd: replace schedule_timeout() with msleep()

[Nish Aravamudan]

On Sat, 22 Jan 2005 16:25:21 +1100, Paul Mackerras <paulus at samba.org> wrote:
> Nishanth Aravamudan writes:
> 
> > Description: Replace schedule_timeout() with msleep()/ssleep(). In both cases,
> > the current code sleeps in TASK_INTERRUPTIBLE but does not account for early
> > wakeups due to signals being caught; therefore I have used TASK_UNINTERRUPTIBLE
> > sleeps in both cases. The second sleep is slightly more difficult to convert as
> > rtas_event_scan_rate is variable. I have left it as a msleep() call, although
> > ssleep() may be more appropriate.
> 
> You have a good point about signals, but I don't like the way that
> this will elevate the load average by 1 the whole time.  We need to
> fix this properly instead.

Ideally, we should fix the load average calculation :) It just seems
counterintuitive to me that people would use a less correct
sleep-state just to prevent the load average from going up. But I
understand your motivation, so it's ok. Just an FYI/FWIW, it seems
most other driver authors/maintainers have been somewhat ok with use
TASK_UNINTERRUPTIBLE via msleep()/ssleep(), just because of the time
units difference (which is just so much easier to understand).
Admittedly, it's going to be a long time before HZ is completely out
of the kernel (at least the way it is used today to calculate
delays/timeouts -- there are a total of ~4000 lines of HZ throughout
the kernel), but these patches *are* the first step.

How exactly do you mean fix it properly? Do you want to deal with
signals? It doesn't seem like the code should fail if a signal hits,
but you could save the signal state, block all signals, sleep
interruptibly (to prevent load average) and then restore all signals
on wake-up. I would also add a comment to the effect that
TASK_UNINTERRUPTIBLE would be acceptable, if the loadavg calculation
changes; just so another Janitor (once the calc. does change) could go
through and change it to msleep() / ssleep() then.

Thanks,
Nish