Re: [PATCH V2 2/2] tick/cpuidle: Initialize hrtimer mode ofbroadcast

From: Thomas Gleixner
Date: Tue Feb 04 2014 - 05:18:30 EST


> +++ b/kernel/time/tick-broadcast-hrtimer.c
> +/*
> + * This is called from the guts of the broadcast code when the cpu
> + * which is about to enter idle has the earliest broadcast timer event.
> + */
> +static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
> +{
> + ktime_t now, interval;
> + /*
> + * We try to cancel the timer first. If the callback is on
> + * flight on some other cpu then we let it handle it. If we
> + * were able to cancel the timer nothing can rearm it as we
> + * own broadcast_lock.
> + *
> + * However if we are called from the hrtimer interrupt handler
> + * itself, reprogram it.
> + */
> + if (hrtimer_try_to_cancel(&bctimer) >= 0) {
> + hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
> + /* Bind the "device" to the cpu */
> + bc->bound_on = smp_processor_id();
> + } else if (bc->bound_on == smp_processor_id()) {

This part really wants a proper comment. It took me a while to figure
out why this is correct and what the call chain is.


> + now = ktime_get();
> + interval = ktime_sub(expires, now);
> + hrtimer_forward_now(&bctimer, interval);

We are in the event handler called from bc_handler() and expires is
absolute time. So what's wrong with calling
hrtimer_set_expires(&bctimer, expires)?

> +static enum hrtimer_restart bc_handler(struct hrtimer *t)
> +{
> + ce_broadcast_hrtimer.event_handler(&ce_broadcast_hrtimer);
> + return HRTIMER_RESTART;

We probably want to check whether the timer needs to be restarted at
all.

if (ce_broadcast_timer.next_event.tv64 == KTIME_MAX)
return HRTIMER_NORESTART;

return HRTIMER_RESTART;

Hmm?

Thanks,

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