Re: kvm guest: hrtimer: interrupt too slow

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sun, Oct 04, 2009 at 04:01:02PM +0400, Michael Tokarev wrote:
> Marcelo Tosatti wrote:
>> Michael,
>>
>> Can you please give the patch below a try please? (without acpi_pm 
>> timer or priority adjustments for the guest).
>
> Sure.  I'll try it out in a hour or two, while I can experiment freely because
> it's weekend.
>
> But I wonder...
> []
>>> hrtimer: interrupt too slow, forcing clock min delta to 93629025 ns
>>
>> It seems the way hrtimer_interrupt_hanging calculates min_delta is
>> wrong (especially to virtual machines). The guest vcpu can be scheduled
>> out during the execution of the hrtimer callbacks (and the callbacks
>> themselves can do operations that translate to blocking operations in
>> the hypervisor).
>>
>> So high min_delta values can be calculated if, for example, a single
>> hrtimer_interrupt run takes two host time slices to execute, while some
>> other higher priority task runs for N slices in between.
>>
>> Using the hrtimer_interrupt execution time (which can be the worse
>> case at any given time), as the min_delta is problematic.
>>
>> So simply increase min_delta_ns by 50% once every detected failure,
>> which will eventually lead to an acceptable threshold (the algorithm
>> should scale back to down lower min_delta, to adjust back to wealthier
>> times, too).
>
> ..I wonder what should I check for.  I mean, the end result of this patch
> is not entirely clear to me, what should it change?  I see that instead
> of the now-calculated-after-error (probably very large) min_delta, it's
> increased to a smaller value.
>
> So I should be getting more such messages (forcing min_delta to $foo), but
> the "responsiveness" of the guest should stay in more or less acceptable
> range (unless it will continue erroring, in which case the "responsiveness"
> will be slowly reduced).

Right.

> Yes indeed, it's better than current situation, when the guest works fine
> initially but out of the sudden it switches to a wild very-slow-to-reply
> mode.  But it does not look like a right solution either, even if the
> back adjustment (mentioned in the last statement above) will be implemented.
> Unless I completely missed the point...
>
> Neverless, the question stands: what I'm looking for now, when the patch is
> applied?  I can't measure the "responsiveness", especially since the min_delta
> gets set to different (large) values each time (I mean current situation
> without the patch).

You should see min_delta_ns increase to a much smaller value, hopefully in
the 2000-10000 range.

min_delta_ns is the minimum delay a high resolution timer can have. You
had it set in the hundreds of milliseconds range.

>
> Thanks!
>
> /mjt
>
>> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
>> index 49da79a..8997978 100644
>> --- a/kernel/hrtimer.c
>> +++ b/kernel/hrtimer.c
>> @@ -1234,28 +1234,20 @@ static void __run_hrtimer(struct hrtimer *timer)
>>   #ifdef CONFIG_HIGH_RES_TIMERS
>>  -static int force_clock_reprogram;
>> -
>>  /*
>>   * After 5 iteration's attempts, we consider that hrtimer_interrupt()
>>   * is hanging, which could happen with something that slows the interrupt
>> - * such as the tracing. Then we force the clock reprogramming for each future
>> - * hrtimer interrupts to avoid infinite loops and use the min_delta_ns
>> - * threshold that we will overwrite.
>> - * The next tick event will be scheduled to 3 times we currently spend on
>> - * hrtimer_interrupt(). This gives a good compromise, the cpus will spend
>> - * 1/4 of their time to process the hrtimer interrupts. This is enough to
>> - * let it running without serious starvation.
>> + * such as the tracing, so we increase min_delta_ns.
>>   */
>>   static inline void
>> -hrtimer_interrupt_hanging(struct clock_event_device *dev,
>> -			ktime_t try_time)
>> +hrtimer_interrupt_hanging(struct clock_event_device *dev)
>>  {
>> -	force_clock_reprogram = 1;
>> -	dev->min_delta_ns = (unsigned long)try_time.tv64 * 3;
>> -	printk(KERN_WARNING "hrtimer: interrupt too slow, "
>> -		"forcing clock min delta to %lu ns\n", dev->min_delta_ns);
>> +	dev->min_delta_ns += dev->min_delta_ns >> 1;
>> +	if (printk_ratelimit())
>> +		printk(KERN_WARNING "hrtimer: interrupt too slow, "
>> +			"forcing clock min delta to %lu ns\n",
>> +			dev->min_delta_ns);
>>  }
>>  /*
>>   * High resolution timer interrupt
>> @@ -1276,7 +1268,7 @@ void hrtimer_interrupt(struct clock_event_device *dev)
>>   retry:
>>  	/* 5 retries is enough to notice a hang */
>>  	if (!(++nr_retries % 5))
>> -		hrtimer_interrupt_hanging(dev, ktime_sub(ktime_get(), now));
>> +		hrtimer_interrupt_hanging(dev);
>>   	now = ktime_get();
>>  @@ -1342,7 +1334,7 @@ void hrtimer_interrupt(struct clock_event_device 
>> *dev)
>>   	/* Reprogramming necessary ? */
>>  	if (expires_next.tv64 != KTIME_MAX) {
>> -		if (tick_program_event(expires_next, force_clock_reprogram))
>> +		if (tick_program_event(expires_next, 0))
>>  			goto retry;
>>  	}
>>  }