Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1303867 > unrolled thread

[PATCH] [SCSI] osst: remove double conversion of timeout

Back to article view | Back to linux.kernel

Contents

  [PATCH] [SCSI] osst: remove double conversion of timeout Nicholas Mc Guire <hofrat@osadl.org> - 2016-01-07 21:20 +0100
    RE: [PATCH] [SCSI] osst: remove double conversion of timeout "Seymour, Shane M" <shane.seymour@hpe.com> - 2016-01-08 01:10 +0100
      Re: [PATCH] [SCSI] osst: remove double conversion of timeout Nicholas Mc Guire <der.herr@hofr.at> - 2016-01-08 08:40 +0100
        RE: [PATCH] [SCSI] osst: remove double conversion of timeout "Seymour, Shane M" <shane.seymour@hpe.com> - 2016-01-11 02:00 +0100
          Re: [PATCH] [SCSI] osst: remove double conversion of timeout Nicholas Mc Guire <der.herr@hofr.at> - 2016-01-12 16:20 +0100

#1303867 — [PATCH] [SCSI] osst: remove double conversion of timeout

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(-)

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;
-- 
2.1.4

[toc] | [next] | [standalone]

#1304049

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

[toc] | [prev] | [next] | [standalone]

#1304236

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

[toc] | [prev] | [next] | [standalone]

#1305757

> 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>

[toc] | [prev] | [next] | [standalone]

#1307530

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web