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?
