Re: [PATCH v1 1/6] timer: make __next_timer_interrupt explicit about no future event

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

On Thu, 3 May 2012, Gilad Ben-Yossef wrote:
> What is happening is that when __next_timer_interrupt() wishes
> to return a value that signifies "there is no future timer
> event", it returns (base->timer_jiffies + NEXT_TIMER_MAX_DELTA).
> However, the code in tick_nohz_stop_sched_tick(), which called
> __next_timer_interrupt() via get_next_timer_interrupt(),
> compares the return value to (last_jiffies + NEXT_TIMER_MAX_DELTA)
> to see if the timer needs to be re-armed.

Yeah, that's nonsense.
> I've noticed a similar but slightly different fix to the
> same problem in the Tilera kernel tree from Chris M. (I've
> wrote this before seeing that one), so some variation of this
> fix is in use on real hardware for some time now.

Sigh, why can't people post their fixes instead of burying them in
their private trees?

> -static unsigned long __next_timer_interrupt(struct tvec_base *base)
> +static bool __next_timer_interrupt(struct tvec_base *base,
> +					unsigned long *next_timer)


> +out:
> +	if (found)
> +		*next_timer = expires;
> +	return found;

I'd really like to avoid that churn. That function is ugly enough
already. No need to make it even more so.

> @@ -1317,9 +1322,15 @@ unsigned long get_next_timer_interrupt(unsigned long now)
>  	if (cpu_is_offline(smp_processor_id()))
>  		return now + NEXT_TIMER_MAX_DELTA;
>  	spin_lock(&base->lock);
> -	if (time_before_eq(base->next_timer, base->timer_jiffies))
> -		base->next_timer = __next_timer_interrupt(base);
> -	expires = base->next_timer;
> +	if (time_before_eq(base->next_timer, base->timer_jiffies)) {
> +
> +		if (__next_timer_interrupt(base, &expires))
> +			base->next_timer = expires;
> +		else
> +			expires = now + NEXT_TIMER_MAX_DELTA;

Here you don't update base->next_timer which makes sure, that we run
through the scan function on every call. Not good.

> +	} else
> +		expires = base->next_timer;
> +

If the thing is empty or just contains deferrable timers then we
really want to avoid running through the whole cascade horror for

Timer add and remove are protected by base->lock. So we simply should
count the non deferrable enqueued timers and avoid the whole
__next_timer_interrupt() completely in case there is nothing what
should wake us up.

I had a deeper look at that and will send out a repair set soon.



To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: .
Fight unfair telecom internet charges in Canada: sign
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>

[Site Home]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Tools]     [DDR & Rambus]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

Add to Google Google PageRank Checking tool