video/bttv-driver: replace
Quoted 11 lines · Nishanth Aravamudan
I would appreciate any comments from the janitors list. This is one (of many) cases where I made a decision about replacing
set_current_state(TASK_INTERRUPTIBLE);schedule_timeout(some_time);with
msleep(jiffies_to_msecs(some_time));
msleep()is not exactly the same as the previous code,
How does it differ? And what exactly is the point in replacing it?
Gerd
Signature · 1 line
return -ENOSIG;
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 9 lines · Gerd Knorr
Quoted 6 lines · Nishanth Aravamudan
set_current_state(TASK_INTERRUPTIBLE);schedule_timeout(some_time);msleep(jiffies_to_msecs(some_time));
msleep()is not exactly the same as the previous code,How does it differ? And what exactly is the point in replacing it?
Especially as msleep() does busy waits (unless I've missed something) whereas schedule_timeout() doesn't? I'd like to veto this one, and the other ones touching video4linux drivers as well.
Gerd
Signature · 1 line
return -ENOSIG;
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 7 lines · Gerd Knorr
Quoted 6 lines · Nishanth Aravamudan
I would appreciate any comments from the janitors list. This is one (of many) cases where I made a decision about replacing
set_current_state(TASK_INTERRUPTIBLE);schedule_timeout(some_time);
Quoted 7 lines
Quoted 5 lines
with
msleep(jiffies_to_msecs(some_time));
msleep()is not exactly the same as the previous code,How does it differ? And what exactly is the point in replacing it?
From a previous post to kernel-janitors:
Well, there are two things to consider here:
As a reference, the way the code was written before:
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(HZ / 10); /* wait 100ms */
1) Since the state is set to TASK_INTERRUPTIBLE, although a 100ms delay is requested, if a signal is received, then the delay ends prematurely and the task will resume after the schedule_timeout() call. Now, if this was the desired effect, then the the patch should not be applied.
2) However, if a true delay was desired, then TASK_INTERRUPTIBLE should not have been the set state, but TASK_UNINTERRUPTIBLE should have been. This would have made the code:
set_current_state(TASK_UNINTERRUPTIBLE); schedule_timeout(HZ / 10);
msleep() achieves this exact purpose, but also:
a) allows the parameter time to be in msecs, which is far clearer IMO; and
b) guarantees that the task will not continue execution until after the timeout has expired.
Confusingly (at least to me), there are certain conditions -- the details of which I will have to defer to Greg Kroah-Hartman -- in which TASK_UNINTERRUPTIBLE'd schedule_timeout()s may still return before the timeout. The desired delay is achieved by wrapping the schedule_timeout(timeout) with a while(timeout) loop. Thus, as long as timeout is non-zero, the task is deferred. (see msleep() definition for more details).
Hope that clears up the differences.
-Nish
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 8 lines · Gerd Knorr
On Tue, Jul 27, 2004 at 08:26:50AM +0200, Gerd Knorr wrote:
return-ENOSIG;Quoted 6 lines · Nishanth Aravamudan
Quoted 5 lines
set_current_state(TASK_INTERRUPTIBLE);schedule_timeout(some_time);msleep(jiffies_to_msecs(some_time));
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
msleep()is not exactly the same as the previous code,How does it differ? And what exactly is the point in replacing it?
Especially as
msleep()does busy waits (unless I've missed something) whereasschedule_timeout()doesn't? I'd like to veto this one, and the other ones touching video4linux drivers as well.
msleep() does not busy wait. It in fact uses schedule_timeout() but in a more reliable way than most code. Also, it allows the drivers to consider their longer delays in terms of msecs instead of some sort of conversion on HZ.
kernel/timer.c::msleep():
/**
* msleep - sleep safely even with waitqueue interruptions
* @msecs: Time in milliseconds to sleep for
*/
void msleep(unsigned int msecs)
{
unsigned long timeout = msecs_to_jiffies(msecs);
while (timeout) {
set_current_state(TASK_UNINTERRUPTIBLE);
timeout = schedule_timeout(timeout);
}
}-Nish
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors