Re: [PATCH 1/4] hrtimers: provide a hrtimers_late_resume() call

[David Vrabel <david.vrabel@citrix.com>]

On 21/06/13 15:32, Thomas Gleixner wrote:
> On Fri, 21 Jun 2013, David Vrabel wrote:
>> On 21/06/13 08:53, Thomas Gleixner wrote:
>>> This is the completely wrong approach. If an architecture does not
>>> shut down the non boot cpus on suspend, then this wants to be handled
>>> in the core code and not in some random arch specific driver.
>>
>> Agreed.  Does the following meet your requirements?
> 
> Indeed. That's looks way more reasonable. Though...
> 
>> hrtimers_resume() cannot call on_each_cpu(retrigger_next_event,...)
>> as the other CPUs will be stopped with IRQs disabled.  Instead, defer
>> the call to the next softirq.
> 
> that's just working by chance and not by design as there is no
> guarantee that the next interrupt, which invokes the softirq, will
> arrive in time. So you want to make sure that an interrupt arrives.

That is a good point.

> Invoking retrigger_next_event(NULL) from hrtimer_resume() should do
> the trick.

But I'm not sure that would be sufficient, although I may not be
understanding how the hrtimers work or are used.

There may be timers on other CPUs that are supposed to fire earlier than
those on the current CPU.  There may even be no timers scheduled on the
current CPU.

I think there needs to be something like:

hrtimers_resume()
{
    retrigger_next_event(NULL);

    /* Timers on other CPUs might expire earlier.
       Program an earlier event and use this to kick the softirq to
       correctly reprogram the events on the other CPUs. */
    expires_next = cpu_base->expires_next;
    for_each_online_cpu(cpu) {
        expires = hrtimers_next_event_on_cpu(cpu)
        if (expires < expires_next)
            expires_next = expires;
   }
   tick_program_event(expires_next, 1);
   cpu_base->clock_was_set = 1;
   __raise_softirq_irqoff(HRTIMER_SOFTIRQ);
}

However, since hrtimers require the use of a one-shot ticker and when
one-shot timers are resumed they are armed to fire immediately (see
tick_resume_oneshot()) this interrupt is sufficient to kick the require
softirq.

So, as proposed before:

hrtimers_resume()
{
   /* This CPU's tick is armed to fire immediately by
      tick_oneshot_resume(). Just need raise a softirq to program
      the timers on all CPUs. */
   cpu_base->clock_was_set = 1;
   __raise_softirq_irqoff(HRTIMER_SOFTIRQ);
}

Do you agree or disagree?

David