Re: [UPDATE PATCH] Add schedule_timeout_{intr,unintr}{,_msecs}() interfaces

[Roman Zippel <zippel@linux-m68k.org>]

Hi,

On Mon, 1 Aug 2005, Nishanth Aravamudan wrote:

> +unsigned int __sched schedule_timeout_msecs(unsigned int timeout_msecs)
> +{
> +	unsigned long expire_jifs;
> +
> +	if (timeout_msecs == MAX_SCHEDULE_TIMEOUT_MSECS) {
> +		expire_jifs = MAX_SCHEDULE_TIMEOUT;
> +	} else {
> +		/*
> +		 * msecs_to_jiffies() is a unit conversion, which truncates
> +		 * (rounds down), so we need to add 1.
> +		 */
> +		expire_jifs = msecs_to_jiffies(timeout_msecs) + 1;
> +	}
> +
> +	expire_jifs = schedule_timeout(expire_jifs);
> +
> +	/*
> +	 * don't need to add 1 here, even though there is truncation,
> +	 * because we will add 1 if/when the value is sent back in
> +	 */
> +	return jiffies_to_msecs(expire_jifs);
> +}

As I already mentioned for msleep_interruptible this is a really terrible 
interface.
The "jiffies_to_msecs(msecs_to_jiffies(timeout_msecs) + 1)" case (when the 
process is immediately woken up again) makes the caller suspectible to 
timeout manipulations and requires constant reauditing, that no caller 
gets it wrong, so it's better to avoid this error source completely.
Constant conversion between different time units is a really bad idea. If 
the user needs the remaining time, he is really better off to do it 
himself by checking jiffies and only does an initial conversion from 
relative to absolute (kernel) time.
This wrapper function really should be an inline function and should look 
more like this:

static inline int schedule_timeout_msecs(unsigned int timeout_msecs)
{
	return schedule_timeout(msecs_to_jiffies(timeout_msecs) + 1) != 0;
}

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/