Re: [PATCH 2/3] hrtimer: Kick lowres dynticks targets on hrtimer_start*() calls

On Wed, Jun 18, 2014 at 06:47:57PM +0530, Viresh Kumar wrote:

On 18 June 2014 18:23, Frederic Weisbecker fweisbec@gmail.com wrote:

...

Reusing the changelogs of periodic timers kind of works here. But it's a bit awkward to put things that way.

The changelog seem to assume that it's obvious that we must kick the target of a lowres hrtimer whereas we actually never did it.

Correct !!

I have updated logs and pushed them back in my repo I shared with you earlier. I have updated 1/3 as well a bit with what you suggested..

Though I have kept some part from earlier log about how can we get idle/full dynticks CPU from get_nohz_timer_target(). Might be worth mentioning ?

New logs:

hrtimer: Kick lowres dynticks targets on hrtimer_start*() calls

In lowres mode, hrtimers are serviced by tick instead of a clock event.  Now
being serviced by the tick imply that we must deal with dynticks

mode, pretty much like timer list timers do.

So if you want to mention get_nohz_timer_target(), better put it right here to outline that get_nohz_timer_target() can return either a full dynticks CPU or an idle dynticks CPU dur to the race window.

There are two requirements to correctly handle dynticks:

1) On target's tick stop time, we must not delay the next tick

further the next hrtimer 2) On hrtimer queue time. If the tick of the target is stopped, we must wake up that CPU such that it sees the new hrtimer and recalculate the next tick.

The point 1 is well handled currently through get_nohz_timer_interrupt() and
cmp_next_hrtimer_event().

But the point 2 isn't handled at all.

Fixing this is easy though as we have the necessary API ready for

that. All we need is to call wake_up_nohz_cpu() on a target when a newly enqueued timer requires tick rescheduling, like timer list timer do."

get_nohz_timer_target() doesn't return a remote idle target but

there is a small race window here. The elected target has all the time to turn dynticks idle between the call to get_nohz_timer_target() and the locking of its base. Hence a risk that we enqueue a hrtimer on a dynticks idle destination without kicking it. As a result, the hrtimer might be serviced too late in the future.

Also a target elected by get_nohz_timer_target() can be in full

dynticks mode and thus require to be kicked as well. And unlike idle dynticks, this concern both local and remote targets.

To fix this, lets centralize dynticks kick to __hrtimer_start_range_ns(), so
that it is well handled for all sort of hrtimer enqueue APIs. It will be
performed by an IPI kick on the target, exactly the way it is performed for
periodic timers currently.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Any more improvements required?

Nope, looks good, thanks!