Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Tue, 10 Oct 2023 17:20:52 -0700
In-Reply-To: <34057852-f6c0-d6d5-261f-bbb5fa056425@oracle.com>
Mime-Version: 1.0
References: <a461bf3f-c17e-9c3f-56aa-726225e8391d@oracle.com>
 <884aa233ef46d5209b2d1c92ce992f50a76bd656.camel@infradead.org>
 <ZRrxtagy7vJO5tgU@google.com> <52a3cea2084482fc67e35a0bf37453f84dcd6297.camel@infradead.org>
 <ZRtl94_rIif3GRpu@google.com> <9975969725a64c2ba2b398244dba3437bff5154e.camel@infradead.org>
 <ZRysGAgk6W1bpXdl@google.com> <d6dc1242ff731cf0f2826760816081674ade9ff9.camel@infradead.org>
 <ZR2pwdZtO3WLCwjj@google.com> <34057852-f6c0-d6d5-261f-bbb5fa056425@oracle.com>
Message-ID: <ZSXqZOgLYkwLRWLO@google.com>
Subject: Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically
From:   Sean Christopherson <seanjc@google.com>
To:     Dongli Zhang <dongli.zhang@oracle.com>
Cc:     David Woodhouse <dwmw2@infradead.org>,
        Joe Jin <joe.jin@oracle.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com,
        bp@alien8.de, dave.hansen@linux.intel.com
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Wed, Oct 04, 2023, Dongli Zhang wrote:
> > And because that's not enough, on pCPU migration or if the TSC is unstable,
> > kvm_arch_vcpu_load() requests KVM_REQ_GLOBAL_CLOCK_UPDATE, which schedules
> > kvmclock_update_fn() with a delay of 100ms.  The large delay is to play nice with
> > unstable TSCs.  But if KVM is periodically doing clock updates on all vCPU,
> > scheduling another update with a *longer* delay is silly.
> 
> We may need to add above message to the places, where
> KVM_REQ_GLOBAL_CLOCK_UPDATE is replaced with KVM_REQ_CLOCK_UPDATE in the patch?

Yeah, comments are most definitely needed, this was just intended to be a quick
sketch to get the ball rolling.

> > -static void kvm_gen_kvmclock_update(struct kvm_vcpu *v)
> > -{
> > -	struct kvm *kvm = v->kvm;
> > -
> > -	kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
> > -	schedule_delayed_work(&kvm->arch.kvmclock_update_work,
> > -					KVMCLOCK_UPDATE_DELAY);
> > -}
> > -
> >  #define KVMCLOCK_SYNC_PERIOD (300 * HZ)
> 
> While David mentioned "maximum delta", how about to turn above into a module
> param with the default 300HZ.
> 
> BTW, 300HZ should be enough for vCPU hotplug case, unless people prefer 1-hour
> or 1-day.

Hmm, I think I agree with David that it would be better if KVM can take care of
the gory details and promise a certain level of accuracy.  I'm usually a fan of
punting complexity to userspace, but requiring every userspace to figure out the
ideal sync frequency on every platform is more than a bit unfriendly.  And it
might not even be realistic unless userspace makes assumptions about how the kernel
computes CLOCK_MONOTONIC_RAW from TSC cycles.

 : so rather than having a user-configured period for the update, KVM could
 : calculate the frequency for the updates based on the rate at which the clocks
 : would otherwise drift, and a maximum delta? Not my favourite option, but
 : perhaps better than nothing?