[RFC] mod_timer() helper functions?

[RFC] mod_timer() helper functions?

[Chris Peterson]

Reviewing the kernel's nearly one-thousand calls to mod_timer(), there
are three basic patterns:

 * multi-second timeouts
 * millisecond timeouts
 * +1 jiffie ASAP events

Few mod_timer() calls actually use the function's 'expires' deadline
time without manually calculating 'jiffies + delay'. The following
helper functions could provide a simpler, more descriptive interface
than the low-level mod_timer() function. Also, when scheduling longer
timers, these helper functions could use round_jiffies() to (secretly)
batch timers on whole seconds to reduce power usage.

Any suggestions? Is there enough value to warrant adding helper
function like these as an alternative to mod_timer()?


chris

---
static inline int mod_timer_seconds(struct timer_list *timer, time_t seconds)
{
	return mod_timer(timer, round_jiffies(jiffies + seconds * HZ));
}

static inline int mod_timer_msecs(struct timer_list *timer, unsigned int msecs)
{
	/* TODO? Round jiffies if within some epsilon of a whole second? */
	return mod_timer(timer, jiffies + msecs_to_jiffies(msecs));
}

static inline int mod_timer_yield(struct timer_list *timer)
{
	/* After these messages, we'll be right back. */
	return mod_timer(timer, jiffies + 1);
}

Re: [RFC] mod_timer() helper functions?

[Andrew Morton]

On Sat, 16 May 2009 00:36:15 -0700 Chris Peterson <cpeterso@cpeterso.com> wrote:

> Reviewing the kernel's nearly one-thousand calls to mod_timer(), there
> are three basic patterns:
> 
>  * multi-second timeouts
>  * millisecond timeouts
>  * +1 jiffie ASAP events
> 
> Few mod_timer() calls actually use the function's 'expires' deadline
> time without manually calculating 'jiffies + delay'. The following
> helper functions could provide a simpler, more descriptive interface
> than the low-level mod_timer() function. Also, when scheduling longer
> timers, these helper functions could use round_jiffies() to (secretly)
> batch timers on whole seconds to reduce power usage.
> 
> Any suggestions? Is there enough value to warrant adding helper
> function like these as an alternative to mod_timer()?
> 
> 
> chris
> 
> ---
> static inline int mod_timer_seconds(struct timer_list *timer, time_t seconds)
> {
> 	return mod_timer(timer, round_jiffies(jiffies + seconds * HZ));
> }
> 
> static inline int mod_timer_msecs(struct timer_list *timer, unsigned int msecs)
> {
> 	/* TODO? Round jiffies if within some epsilon of a whole second? */
> 	return mod_timer(timer, jiffies + msecs_to_jiffies(msecs));
> }
> 
> static inline int mod_timer_yield(struct timer_list *timer)
> {
> 	/* After these messages, we'll be right back. */
> 	return mod_timer(timer, jiffies + 1);
> }

I think it makes sense, yes.

I do think that the name should be changed to communicate the fact that
the change is relative.  advance_timer_foo(), perhaps.

Or, if you want to put lipstick on our pig, timer_advance_foo().  All
these API functions should have started with "timer_" but we screwed
that up ages ago.

I expect that most/all of these functions will be too large to inline,
btw.  Check the generated code and I expect you'll be surprised how
porky they are.

Re: [RFC] mod_timer() helper functions?

[Chris Peterson]

>> Reviewing the kernel's nearly one-thousand calls to mod_timer(), there
>> are three basic patterns:
>>
>>  * multi-second timeouts
>>  * millisecond timeouts
>>  * +1 jiffie ASAP events
>>

Is there a functional difference between the following "expire this
timer ASAP" statements?

mod_timer(timer, jiffies + 1); /* 48 uses */
mod_timer(timer, jiffies); /* 44 uses */
mod_timer(timer, jiffies - 1); /* 6 uses */


chris

Re: [RFC] mod_timer() helper functions?

[Andrew Morton]

On Sun, 17 May 2009 00:50:55 -0700 Chris Peterson <cpeterso@cpeterso.com> wrote:

> >> Reviewing the kernel's nearly one-thousand calls to mod_timer(), there
> >> are three basic patterns:
> >>
> >> __* multi-second timeouts
> >> __* millisecond timeouts
> >> __* +1 jiffie ASAP events
> >>
> 
> Is there a functional difference between the following "expire this
> timer ASAP" statements?
> 
> mod_timer(timer, jiffies + 1); /* 48 uses */
> mod_timer(timer, jiffies); /* 44 uses */
> mod_timer(timer, jiffies - 1); /* 6 uses */

That's something which has always worried me.  Lots of code does:

	mod_timer(timer, jiffies + 1);

(for varying values of "1").  What happens if this thread of control
then gets stalled for a couple of jiffies.  Say it gets preempted or
there's a long interrupt or whatever.  So there's a several-jiffy
interval between the caller evaluating jiffies+1 and the entry to
mod_timer().


>From my reading, we'll hit

		int i;
		/* If the timeout is larger than 0xffffffff on 64-bit
		 * architectures then we use the maximum timeout:
		 */
		if (idx > 0xffffffffUL) {
			idx = 0xffffffffUL;
			expires = idx + base->timer_jiffies;
		}
		i = (expires >> (TVR_BITS + 3 * TVN_BITS)) & TVN_MASK;
		vec = base->tv5.vec + i;

and the timer gets scheduled at some time in the far-distant future!

But this is such a glaring and huge problem that surely it cannot
exist.  But I don't know why not.

If the bug _does_ exist then mod_timer(timer, jiffies - 1) will set the
timer to go off in the far future.  mod_timer(timer, jiffies) will
usually make it go off real soon now, but it's scary.  mod_timer(timer,
jiffies + 1) is safer, but still vulnerable.

Re: [RFC] mod_timer() helper functions?

[Thomas Gleixner]

On Sun, 17 May 2009, Andrew Morton wrote:

> On Sun, 17 May 2009 00:50:55 -0700 Chris Peterson <cpeterso@cpeterso.com> wrote:
> 
> > >> Reviewing the kernel's nearly one-thousand calls to mod_timer(), there
> > >> are three basic patterns:
> > >>
> > >> __* multi-second timeouts
> > >> __* millisecond timeouts
> > >> __* +1 jiffie ASAP events
> > >>
> > 
> > Is there a functional difference between the following "expire this
> > timer ASAP" statements?
> > 
> > mod_timer(timer, jiffies + 1); /* 48 uses */
> > mod_timer(timer, jiffies); /* 44 uses */
> > mod_timer(timer, jiffies - 1); /* 6 uses */
> 
> That's something which has always worried me.  Lots of code does:
> 
> 	mod_timer(timer, jiffies + 1);
> 
> (for varying values of "1").  What happens if this thread of control
> then gets stalled for a couple of jiffies.  Say it gets preempted or
> there's a long interrupt or whatever.  So there's a several-jiffy
> interval between the caller evaluating jiffies+1 and the entry to
> mod_timer().
> 
> 
> >From my reading, we'll hit
> 
> 		int i;
> 		/* If the timeout is larger than 0xffffffff on 64-bit
> 		 * architectures then we use the maximum timeout:
> 		 */
> 		if (idx > 0xffffffffUL) {
> 			idx = 0xffffffffUL;
> 			expires = idx + base->timer_jiffies;
> 		}
> 		i = (expires >> (TVR_BITS + 3 * TVN_BITS)) & TVN_MASK;
> 		vec = base->tv5.vec + i;
> 
> and the timer gets scheduled at some time in the far-distant future!
> 
> But this is such a glaring and huge problem that surely it cannot
> exist.  But I don't know why not.
> 
> If the bug _does_ exist then mod_timer(timer, jiffies - 1) will set the
> timer to go off in the far future.  mod_timer(timer, jiffies) will
> usually make it go off real soon now, but it's scary.  mod_timer(timer,
> jiffies + 1) is safer, but still vulnerable.

You missed a little detail:

	} else if ((signed long) idx < 0) {
		/*
		 * Can happen if you add a timer with expires == jiffies,
		 * or you set a timer to go off in the past
		 */
		vec = base->tv1.vec + (base->timer_jiffies & TVR_MASK);
	} else {
		int i;
		/* If the timeout is larger than 0xffffffff on 64-bit
		 * architectures then we use the maximum timeout:
		 */

So if the timer is set to go off in the past or right away then it is
enqueued into the timer list bucket which is processed in the next
timer softirq. That's why we use base->timer_jiffies and not jiffies
directly. base->timer_jiffies is incremented in the timer
softirq. softirq and enqueueing are serialized via base->lock.

Thanks,

	tglx

Re: [RFC] mod_timer() helper functions?

[Andi Kleen]

Chris Peterson <cpeterso@cpeterso.com> writes:
>
> Any suggestions? Is there enough value to warrant adding helper
> function like these as an alternative to mod_timer()?

I like the basic idea. The opencoding has always annoyed me.
>
>
> chris
>
> ---
> static inline int mod_timer_seconds(struct timer_list *timer, time_t seconds)

I would prefer a name like mod_timer_in_secs() to make it clear it's relative

> {
> 	return mod_timer(timer, round_jiffies(jiffies + seconds * HZ));
> }

>
> static inline int mod_timer_msecs(struct timer_list *timer, unsigned int msecs)
> {
> 	/* TODO? Round jiffies if within some epsilon of a whole second? */
> 	return mod_timer(timer, jiffies + msecs_to_jiffies(msecs));

This is a bit misleading because depending on HZ the accuracy is far from
a millisecond (worst case 10ms error with HZ==100). Naming should make
that clearer. Or maybe these users should all switch to hrtimers,
but then those could be not available either.

I suspect you also want a helper for longer term delays that uses
deferred timers or more aggressive rounding.


> }
>
> static inline int mod_timer_yield(struct timer_list *timer)
> {
> 	/* After these messages, we'll be right back. */
> 	return mod_timer(timer, jiffies + 1);

Can't say I like the name here, although I don't have a better one.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFC] mod_timer() helper functions?

[Chris Peterson]

How about something like these helper functions?

The function names "timer_settime_msecs()" and "timer_settime_secs()"
are not fantastic, but they are inspired by the precedent set by the
POSIX Realtime function timer_settime().


/**
 * timer_settime_msecs - modify a timer's timeout
 * @timer: the timer to be modified
 * @msecs: minimum time in milliseconds to sleep for
 */
int timer_settime_msecs(struct timer_list *timer, int msecs)
{
	unsigned long expires = jiffies + msecs_to_jiffies(msecs);

	/* TODO? round_jiffies() if msecs parameter is large, say >= ~5000? */
        /* If people don't mind if timer_settime_msecs() rounds to
whole seconds, then timer_settime_secs() might be unnecessary. */

	return mod_timer(timer, expires);
}


/**
 * timer_settime_secs - modify a timer's timeout
 * @timer: the timer to be modified
 * @secs: approximate time in seconds to sleep for
 */
int timer_settime_secs(struct timer_list *timer, time_t secs)
{
	unsigned long expires = jiffies + (secs * HZ);

	/* If the caller doesn't mind waiting multiple seconds, then
	 * feel free to round up to whole seconds (to reduce wakeups).
	 */
	if (secs >= 2)
		expires = round_jiffies_up(expires);

	return mod_timer(timer, expires);
}

Re: [RFC] mod_timer() helper functions?

[Andi Kleen]

> /**
>  * timer_settime_msecs - modify a timer's timeout
>  * @timer: the timer to be modified
>  * @msecs: minimum time in milliseconds to sleep for
>  */
> int timer_settime_msecs(struct timer_list *timer, int msecs)

At least this function needs something that indicates it is very approximate
(upto 900% error for 1ms with HZ=100)
The old open coded pattern showed this. 

Or perhaps better drop the msecs version and just keep the seconds version.

People who want msecs accuracy likely should use hrtimers anyways.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFC] mod_timer() helper functions?

[Chris Peterson]

>> int timer_settime_msecs(struct timer_list *timer, int msecs)
>
> At least this function needs something that indicates it is very approximate
> (upto 900% error for 1ms with HZ=100)
> The old open coded pattern showed this.

I see your point. But doesn't the open coded pattern with jiffies
imply an even greater timer precision than milliseconds? Or does
mod_timer()'s use of an expiration time instead of a time delay avoid
promising more precision than it can provide?

Quite of a few mod_timer() calls already try to schedule precise
timeouts in terms of HZ/10 or HZ/20.


chris