| Message ID | 1452197433-26086-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
Hi Nicholas, > - msleep(jiffies_to_msecs(initial_delay)); > + schedule_timeout_uninterruptible(initial_delay); The code to msleep looks like this: /** * 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) + 1; while (timeout) timeout = schedule_timeout_uninterruptible(timeout); } So msleep will wait for the requested amount of time to pass even if woken early but your change may not. To make it equivalent you would need to borrow code from msleep and remove the "if (initial_delay > 0)" and just have: while (initial_delay) initial_delay = schedule_timeout_uninterruptible(initial_delay); We can change initial_delay since it's not used elsewhere in the function and if initial_delay is 0 we won't ever call schedule_timeout_uninterruptible in the while loop so there's no need to test if it's > 0 or not. Thanks Shane -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 08, 2016 at 12:02:18AM +0000, Seymour, Shane M wrote: > Hi Nicholas, > > > - msleep(jiffies_to_msecs(initial_delay)); > > + schedule_timeout_uninterruptible(initial_delay); > > The code to msleep looks like this: > > /** > * 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) + 1; > > while (timeout) > timeout = schedule_timeout_uninterruptible(timeout); > } > > So msleep will wait for the requested amount of time to pass even if woken early but your change may not. To make it equivalent you would need to borrow code from msleep and remove the "if (initial_delay > 0)" and just have: If I understand the documentation to schedule_timeout() correctly then this is not the case: <snip> /** * 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()). * * You can set the task state as follows - * * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to * pass before the routine returns. The routine will return 0 ... <snip> and schedule_timeout_uninterruptiple does that signed long __sched schedule_timeout_uninterruptible(signed long timeout) { __set_current_state(TASK_UNINTERRUPTIBLE); return schedule_timeout(timeout); } so my understanding is that this will not return early and should thus be equivalent to the behavior of msleep. did I overlook something here ? thx! hofrat -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> did I overlook something here ? You probably didn't overlook anything my comments were based on how msleep was coded. --- Reviewed-by: Shane Seymour <shane.seymour@hpe.com> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 11, 2016 at 12:54:48AM +0000, Seymour, Shane M wrote: > > > did I overlook something here ? > > You probably didn't overlook anything my comments were based on how msleep > was coded. > ...I did overlookck something - comments in the code may not always be correct... Unfortunatly the comment regarding schedule_timeout() in timer.c turns out to be wrong as clarified by Thomas Gleixnr - see below: <snip> On Tue, 12 Jan 2016, Nicholas Mc Guire wrote: > On Mon, Jan 11, 2016 at 09:15:25AM +0100, Thomas Gleixner wrote: > > On Sat, 9 Jan 2016, Nicholas Mc Guire wrote: > > > > > The while loop in msleep does not seem necessary as > > > timeout is unsigned long and no larger than MAX_JIFFY_OFFSET (which is > > > LONG_MAX/2 - 1) so the while-loop condition is always true at the beginning > > > (msecs_to_jiffies will return >=0 always and with the +1 timeout is >= 1 so > > > the while condition is always true at the start) and > > > schedule_timeout_uninterruptible always returns 0, so the while loop always > > > terminates after the first loop. > > > > Err, no. schedule_timeout_uninterruptible() can return > 0 when there was a > > non timer wakeup. Thinks spurious wakeups. So we need that loop. > > ok - thanks - was following the comment in schedule_timeout which states: > /** > * 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()). > * > * You can set the task state as follows - > * > * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to > * pass before the routine returns. The routine will return 0 > <snip> > > So I had assumed that would not actualy be possible given this comment. > Is the while(timeout) loop im msleep() just defensive programming or is > there a spurious timer wakeup path that defeats TASK_UNINTERRUPTIBLE ? > Probably a stupid question but I was unable to figure out how such an > wakeup would occure. That comment is wrong. Just a simple example: wait_for_completion_io_timeout(x, timeout) wait_for_common_io(x, timeout, TASK_UNINTERRUPTIBLE) __wait_for_common(x, io_schedule_timeout, timeout, state) do_wait_for_common(x, action, timeout, state) ... io_schedule_timeout(timeout) schedule_timeout(timeout); So the function can return either because the timer fired or the completion completed. In the latter case the @timeout jiffies certainly have not passed. > > > Q: what is the purpose of the + 1 offset to the jiffies here ? > > > > > > msleep was introduced in 2.6.7 but without the + 1, so with: > > > unsigned long timeout = msecs_to_jiffies(msecs); > > > in 2.6.10-rc2 the msecs_to_jiffies(msecs) + 1; is introduced. > > > Nishanth Aravamudan <nacc@us.ibm.com> (https://lkml.org/lkml/2004/11/19/294) > > > seems to be the origin while converting msleep to a macro, but no reason > > > for the + 1 is given there. > > > > Not really. The +1 was introduced with the following commit: > > > > https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/kernel/timer.c?id=c259ef842622a5e64418d9dab3b62ee051867edf > > > > thanks - was ignorant of history.git being available. > will go try and understand where that "lost jiffie" could come from. Assume HZ=1000 and msleep(1). If the msleep() is issued right at the edge of the next tick interrupt, then it would return almost immediately. Certainly not what you would expect, right? Thanks, tglx <snip> So the proposed patch is simply wrong and the replacement of the msleep() by schedule_timeout_uninterruptible() is not equivalent. thx! hofrat -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c index 5033223..d8e5aa9 100644 --- a/drivers/scsi/osst.c +++ b/drivers/scsi/osst.c @@ -760,7 +760,7 @@ static int osst_wait_ready(struct osst_tape * STp, struct osst_request ** aSRpnt #endif if (initial_delay > 0) - msleep(jiffies_to_msecs(initial_delay)); + schedule_timeout_uninterruptible(initial_delay); memset(cmd, 0, MAX_COMMAND_SIZE); cmd[0] = TEST_UNIT_READY;
If the time is actually in jiffies already it seems cleaner to call schedule_timeout_interruptible() than to call msleep and covert the timeout. In this case it will actually optimize the code a bit as it does not need to call jiffies_to_msecs here. Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> --- problem was found by coccinelle spatch patch was compile tested with: x86_64_defconfig + CONFIG_CHR_DEV_OSST=m patch is against linux-next (localversion-next is -next-20160107) drivers/scsi/osst.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)