Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically

[Dongli Zhang]

Hi Joe,

On 9/26/23 17:29, Joe Jin wrote:
> On 9/26/23 4:06 PM, Dongli Zhang wrote:
>> This is to minimize the kvmclock drift during CPU hotplug (or when the
>> master clock and pvclock_vcpu_time_info are updated). The drift is
>> because kvmclock and raw monotonic (tsc) use different
>> equation/mult/shift to calculate that how many nanoseconds (given the tsc
>> as input) has passed.
>>
>> The calculation of the kvmclock is based on the pvclock_vcpu_time_info
>> provided by the host side.
>>
>> struct pvclock_vcpu_time_info {
>> 	u32   version;
>> 	u32   pad0;
>> 	u64   tsc_timestamp;     --> by host raw monotonic
>> 	u64   system_time;       --> by host raw monotonic
>> 	u32   tsc_to_system_mul; --> by host KVM
>> 	s8    tsc_shift;         --> by host KVM
>> 	u8    flags;
>> 	u8    pad[2];
>> } __attribute__((__packed__));
>>
>> To calculate the current guest kvmclock:
>>
>> 1. Obtain the tsc = rdtsc() of guest.
>>
>> 2. If shift < 0:
>>     tmp = tsc >> tsc_shift
>>    if shift > 0:
>>     tmp = tsc << tsc_shift
>>
>> 3. The kvmclock value will be: (tmp * tsc_to_system_mul) >> 32
>>
>> Therefore, the current kvmclock will be either:
>>
>> (rdtsc() >> tsc_shift) * tsc_to_system_mul >> 32
>>
>> ... or ...
>>
>> (rdtsc() << tsc_shift) * tsc_to_system_mul >> 32
>>
>> The 'tsc_to_system_mul' and 'tsc_shift' are calculated by the host KVM.
>>
>> When the master clock is actively used, the 'tsc_timestamp' and
>> 'system_time' are derived from the host raw monotonic time, which is
>> calculated based on the 'mult' and 'shift' of clocksource_tsc:
>>
>> elapsed_time = (tsc * mult) >> shift
>>
>> Since kvmclock and raw monotonic (clocksource_tsc) use different
>> equation/mult/shift to convert the tsc to nanosecond, there may be clock
>> drift issue during CPU hotplug (when the master clock is updated).
>>
>> 1. The guest boots and all vcpus have the same 'pvclock_vcpu_time_info'
>> (suppose the master clock is used).
>>
>> 2. Since the master clock is never updated, the periodic kvmclock_sync_work
>> does not update the values in 'pvclock_vcpu_time_info'.
>>
>> 3. Suppose a very long period has passed (e.g., 30-day).
>>
>> 4. The user adds another vcpu. Both master clock and
>> 'pvclock_vcpu_time_info' are updated, based on the raw monotonic.
>>
>> (Ideally, we expect the update is based on 'tsc_to_system_mul' and
>> 'tsc_shift' from kvmclock).
>>
>>
>> Because kvmclock and raw monotonic (clocksource_tsc) use different
>> equation/mult/shift to convert the tsc to nanosecond, there will be drift
>> between:
>>
>> (1) kvmclock based on current rdtsc and old 'pvclock_vcpu_time_info'
>> (2) kvmclock based on current rdtsc and new 'pvclock_vcpu_time_info'
>>
>> According to the test, there is a drift of 4502145ns between (1) and (2)
>> after about 40 hours. The longer the time, the large the drift.
>>
>> This is to add a module param to allow the user to configure for how often
>> to refresh the master clock, in order to reduce the kvmclock drift based on
>> user requirement (e.g., every 5-min to every day). The more often that the
>> master clock is refreshed, the smaller the kvmclock drift during the vcpu
>> hotplug.
>>
>> Cc: Joe Jin <joe.jin@xxxxxxxxxx>
>> Signed-off-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
>> ---
>> Other options are to update the masterclock in:
>> - kvmclock_sync_work, or
>> - pvclock_gtod_notify()
>>
>>  arch/x86/include/asm/kvm_host.h |  1 +
>>  arch/x86/kvm/x86.c              | 34 +++++++++++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 17715cb8731d..57409dce5d73 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1331,6 +1331,7 @@ struct kvm_arch {
>>  	u64 master_cycle_now;
>>  	struct delayed_work kvmclock_update_work;
>>  	struct delayed_work kvmclock_sync_work;
>> +	struct delayed_work masterclock_sync_work;
>>  
>>  	struct kvm_xen_hvm_config xen_hvm_config;
>>  
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 9f18b06bbda6..0b71dc3785eb 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -157,6 +157,9 @@ module_param(min_timer_period_us, uint, S_IRUGO | S_IWUSR);
>>  static bool __read_mostly kvmclock_periodic_sync = true;
>>  module_param(kvmclock_periodic_sync, bool, S_IRUGO);
>>  
>> +unsigned int __read_mostly masterclock_sync_period;
>> +module_param(masterclock_sync_period, uint, 0444);
> 
> Can the mode be 0644 and allow it be changed at runtime?

It can be RW.

So far I just copy from kvmclock_periodic_sync as most code are from the
mechanism of kvmclock_periodic_sync.


static bool __read_mostly kvmclock_periodic_sync = true;
module_param(kvmclock_periodic_sync, bool, S_IRUGO);


Thank you very much!

Dongli Zhang

> 
> Thanks,
> Joe
>> +
>>  /* tsc tolerance in parts per million - default to 1/2 of the NTP threshold */
>>  static u32 __read_mostly tsc_tolerance_ppm = 250;
>>  module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
>> @@ -3298,6 +3301,31 @@ static void kvmclock_sync_fn(struct work_struct *work)
>>  					KVMCLOCK_SYNC_PERIOD);
>>  }
>>  
>> +static void masterclock_sync_fn(struct work_struct *work)
>> +{
>> +	unsigned long i;
>> +	struct delayed_work *dwork = to_delayed_work(work);
>> +	struct kvm_arch *ka = container_of(dwork, struct kvm_arch,
>> +					   masterclock_sync_work);
>> +	struct kvm *kvm = container_of(ka, struct kvm, arch);
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	if (!masterclock_sync_period)
>> +		return;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		/*
>> +		 * It is not required to kick the vcpu because it is not
>> +		 * expected to update the master clock immediately.
>> +		 */
>> +		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>> +	}
>> +
>> +	schedule_delayed_work(&ka->masterclock_sync_work,
>> +			      masterclock_sync_period * HZ);
>> +}
>> +
>> +
>>  /* These helpers are safe iff @msr is known to be an MCx bank MSR. */
>>  static bool is_mci_control_msr(u32 msr)
>>  {
>> @@ -11970,6 +11998,10 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>  	if (kvmclock_periodic_sync && vcpu->vcpu_idx == 0)
>>  		schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
>>  						KVMCLOCK_SYNC_PERIOD);
>> +
>> +	if (masterclock_sync_period && vcpu->vcpu_idx == 0)
>> +		schedule_delayed_work(&kvm->arch.masterclock_sync_work,
>> +				      masterclock_sync_period * HZ);
>>  }
>>  
>>  void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>> @@ -12344,6 +12376,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>  
>>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
>>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
>> +	INIT_DELAYED_WORK(&kvm->arch.masterclock_sync_work, masterclock_sync_fn);
>>  
>>  	kvm_apicv_init(kvm);
>>  	kvm_hv_init_vm(kvm);
>> @@ -12383,6 +12416,7 @@ static void kvm_unload_vcpu_mmus(struct kvm *kvm)
>>  
>>  void kvm_arch_sync_events(struct kvm *kvm)
>>  {
>> +	cancel_delayed_work_sync(&kvm->arch.masterclock_sync_work);
>>  	cancel_delayed_work_sync(&kvm->arch.kvmclock_sync_work);
>>  	cancel_delayed_work_sync(&kvm->arch.kvmclock_update_work);
>>  	kvm_free_pit(kvm);
>