Re: [PATCH] x86/kvm: fix condition to update kvm master clocks

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-05-27 21:46+0300, Roman Kagan:
> On Fri, May 27, 2016 at 08:11:40PM +0200, Radim Krčmář wrote:
> > 2016-05-27 20:28+0300, Roman Kagan:
>> >> Interaction between kvm_gen_update_masterclock(), pvclock_gtod_work(),
>> >> and NTP could be a problem:  kvm_gen_update_masterclock() only has to
>> >> run once per VM, but pvclock_gtod_work() calls it on every VCPU, so
>> >> frequent NTP updates on bigger guests could kill performance.
>> > 
>> > Unfortunately, things are worse than that: this stuff is updated on
>> > every *tick* on the timekeeping CPU, so, as long as you keep at least
>> > one of your CPUs busy, the update rate can reach HZ.  The frequency of
>> > NTP updates is unimportant; it happens without NTP updates at all.
>> > 
>> > So I tend to agree that we're perhaps better off not fixing this bug and
>> > leaving the kvm_clocks to drift until we figure out how to do it with
>> > acceptable overhead.
>> 
>> Yuck ... the hunk below could help a bit.
>> I haven't checked if the timekeeping code updates gtod and therefore
>> sets 'was_set' even when the resulting time hasn't changed, so we might
>> need to do more to avoid useless situations.
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index a8c7ca34ee5d..37ed0a342bf1 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5802,12 +5802,15 @@ static DECLARE_WORK(pvclock_gtod_work, pvclock_gtod_update_fn);
>>  /*
>>   * Notification about pvclock gtod data update.
>>   */
>> -static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
>> +static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long was_set,
>>  			       void *priv)
>>  {
>>  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
>>  	struct timekeeper *tk = priv;
>>  
>> +	if (!was_set)
>> +		return 0;
>> +
>>  	update_pvclock_gtod(tk);
>>  
> 
> Nope, this parameter is only set when there's a step-like change in the
> time.  The timekeeper itself is always updated.  I guess we could
> mitigate the costs somewhat if we skipped updating the gtod copy until
> the accumulated error reaches certain limit; not sure if that's gonna
> help though.

I see, timekeeping_adjust() isn't covered, but it should not adjust
every tick, so we could propagate information about adjustments to
pvclock_gtod_notify (rename unused to has_changed), because pvclock only
cares about change of time.
Adding another threshold is a reasonable improvement if adjustments
happen too often, but we need to fix pvclock_gtod_update_fn() in any
case.

Am I missing anyting else?

Thanks.