Re: [PATCH v2] KVM: x86: Rate-limit global clock updates on vCPU load

[Thorsten Leemhuis]

On 4/9/26 21:21, Sean Christopherson wrote:
> On Thu, Apr 09, 2026, Lei Chen wrote:
>> commit 446fcce2a52b ("Revert "x86: kvm: rate-limit global clock updates"")
>> dropped the rate limiting for KVM_REQ_GLOBAL_CLOCK_UPDATE.
>>
>> As a result, kvm_arch_vcpu_load() can queue global clock update requests
>> every time a vCPU is scheduled when the master clock is disabled or when
>> the vCPU is loaded for the first time.
>>
>> Restore the throttling with a per-VM ratelimit state and gate
>> KVM_REQ_GLOBAL_CLOCK_UPDATE through __ratelimit(), so frequent vCPU
>> scheduling does not generate a steady stream of redundant clock update
>> requests.
>>
>> Fixes: 446fcce2a52b ("Revert "x86: kvm: rate-limit global clock updates"")
>> Signed-off-by: Lei Chen <lei.chen@xxxxxxxxxx>
>> Reported-by: Jaroslav Pulchart <jaroslav.pulchart@xxxxxxxxxxxx>
>> Closes: https://lore.kernel.org/all/CAK8fFZ5gY8_Mw2A=iZVFNVKQNrXQzVsn-HTd+Me9K6ZfmdgA+Q@xxxxxxxxxxxxxx/

Was this performance regression ever addressed? Looks like this fall
through the cracks, but it's easy to miss something.

Ciao, Thorsten

>> ---
>> CHANGELOG:
>> v2:
>> - remove comment of kvmclock_update_rs
>> - make sure kvm_arch_vcpu_load make KVM_REQ_CLOCK_UPDATE for this vcpu
>> - add RATELIMIT_MSG_ON_RELEASE to kvmclock_update_rs
>>
>> v1:
>> - initial version(https://lore.kernel.org/all/20260407070046.2336-1-lei.chen@xxxxxxxxxx/)
>> ---
>>  arch/x86/include/asm/kvm_host.h |  1 +
>>  arch/x86/kvm/x86.c              | 11 +++++++++--
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 5a3bfa293e8b..5e750c49d21e 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1453,6 +1453,7 @@ struct kvm_arch {
>>  	bool use_master_clock;
>>  	u64 master_kernel_ns;
>>  	u64 master_cycle_now;
>> +	struct ratelimit_state kvmclock_update_rs;
>>  
>>  #ifdef CONFIG_KVM_HYPERV
>>  	struct kvm_hv hyperv;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 63afdb6bb078..a534e8391611 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5210,8 +5210,13 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  		 * On a host with synchronized TSC, there is no need to update
>>  		 * kvmclock on vcpu->cpu migration
>>  		 */
>> -		if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1)
>> -			kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
>> +		if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1) {
>> +			if (__ratelimit(&vcpu->kvm->arch.kvmclock_update_rs))
>> +				kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
>> +			else
>> +				kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> 
> What I was trying to call out in my review of v1, is that prior to commit
> 446fcce2a52b, the effectively ratelimiting applied to *all* instances of
> KVM_REQ_GLOBAL_CLOCK_UPDATE.  Which meant that KVM's existing behavior is that
> kvm_write_system_time() would be subject to the ratelimiting as well.
> 
> That said, I don't see any obvious problems with immediately honoring writes to
> MSR_KVM_SYSTEM_TIME{,_NEW}, and it's probably a much better experience for the
> guest.  So I'm a-ok with this approach, but we should call out that skipping the
> synthetic MSR case is deliberate.  No need for a v3, I'll add a blurb when
> applying.
> 
>> +		}
>> +
>>  		if (vcpu->cpu != cpu)
>>  			kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu);
>>  		vcpu->cpu = cpu;
>> @@ -13189,6 +13194,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>  	raw_spin_lock_init(&kvm->arch.tsc_write_lock);
>>  	mutex_init(&kvm->arch.apic_map_lock);
>>  	seqcount_raw_spinlock_init(&kvm->arch.pvclock_sc, &kvm->arch.tsc_write_lock);
>> +	ratelimit_state_init(&kvm->arch.kvmclock_update_rs, HZ, 10);
>> +	ratelimit_set_flags(&kvm->arch.kvmclock_update_rs, RATELIMIT_MSG_ON_RELEASE);
> 
> IIUC, so long was KVM doesn't explicitly invoke ratelimit_state_exit(), setting
> RATELIMIT_MSG_ON_RELEASE means we won't get dmesg spam?  To be clear, I'm 100%
> in favor of suppressing dmesg output.
> 
>>  	kvm->arch.kvmclock_offset = -get_kvmclock_base_ns();
>>  
>>  	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
>> -- 
>> 2.51.0
>>
>