Re: [PATCH 2/2] x86: kvm: introduce periodic global clock updates

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Feb 26, 2014 at 07:15:12PM +0100, Andrew Jones wrote:
> commit 0061d53daf26f introduced a mechanism to execute a global clock
> update for a vm. We can apply this periodically in order to propagate
> host NTP corrections. Also, if all vcpus of a vm are pinned, then
> without an additional trigger, no guest NTP corrections can propagate
> either, as the current trigger is only vcpu cpu migration.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/x86.c              | 65 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 9aa09d330a4b5..77c69aa4756f9 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -599,6 +599,7 @@ struct kvm_arch {
>  	u64 master_kernel_ns;
>  	cycle_t master_cycle_now;
>  	struct delayed_work kvmclock_update_work;
> +	bool clocks_synced;
>  
>  	struct kvm_xen_hvm_config xen_hvm_config;
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a2d30de597b7d..5cba20b446aac 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1620,6 +1620,60 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>  	return 0;
>  }
>  
> +static void kvm_schedule_kvmclock_update(struct kvm *kvm, bool now);
> +static void clock_sync_fn(struct work_struct *work);
> +static DECLARE_DELAYED_WORK(clock_sync_work, clock_sync_fn);
> +
> +#define CLOCK_SYNC_PERIOD_SECS	300
> +#define CLOCK_SYNC_BUMP_SECS	30
> +#define CLOCK_SYNC_STEP_MSECS	100
> +
> +#define __steps(s) (((s) * MSEC_PER_SEC) / CLOCK_SYNC_STEP_MSECS)
> +
> +static void clock_sync_fn(struct work_struct *work)
> +{
> +	static unsigned reset_step = __steps(CLOCK_SYNC_PERIOD_SECS);
> +	static unsigned step = 0;
> +	struct kvm *kvm;
> +	bool sync = false;
> +
> +	spin_lock(&kvm_lock);

Better have it per vm to avoid kvm_lock?

> +	if (step == 0)
> +		list_for_each_entry(kvm, &vm_list, vm_list)
> +			kvm->arch.clocks_synced = false;
> +
> +	list_for_each_entry(kvm, &vm_list, vm_list) {
> +		if (!kvm->arch.clocks_synced) {
> +			kvm_get_kvm(kvm);
> +			sync = true;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock(&kvm_lock);
> +
> +	if (sync) {
> +		kvm_schedule_kvmclock_update(kvm, true);
> +		kvm_put_kvm(kvm);
> +
> +		if (++step == reset_step) {
> +			reset_step += __steps(CLOCK_SYNC_BUMP_SECS);
> +			pr_warn("kvmclock: reducing VM clock sync frequency "
> +				"to every %ld seconds.\n", (reset_step
> +					* CLOCK_SYNC_STEP_MSECS)/MSEC_PER_SEC);
> +		}

Can you explain the variable sync frequency? To be based on NTP
frequency correction?