Re: [PATCH v7 21/36] KVM: x86: Avoid redundant masterclock updates from multiple vCPUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving O(N) VM-wide vCPU iterations inside a raw spinlock critical section and before write_seqcount_end() causes severe hardirq latency and prolongs reader spinning.
--

--- Patch [21]: [PATCH v7 21/36] KVM: x86: Avoid redundant masterclock updates from multiple vCPUs ---
commit 47fb88fca997575b4513fe25e0a0202e7a2e0b1b
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Avoid redundant masterclock updates from multiple vCPUs

This patch optimizes masterclock updates by preventing multiple vCPUs from
redundantly executing the update sequence. It introduces a check in
__kvm_start_pvclock_update() to bail out if another vCPU has already
processed the request.

> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -1710,10 +1710,39 @@ static void kvm_make_mclock_inprogress_request(struct kvm *kvm)
>  	kvm_make_all_cpus_request(kvm, KVM_REQ_MCLOCK_INPROGRESS);
>  }
>  
> -static void __kvm_start_pvclock_update(struct kvm *kvm)
> +static void kvm_clear_mclock_inprogress_request(struct kvm *kvm)
>  {
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
> +}
> +
> +static bool __kvm_start_pvclock_update(struct kvm *kvm, struct kvm_vcpu *requesting_vcpu)
> +{
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
>  	raw_spin_lock_irq(&kvm->arch.tsc_write_lock);
> +
> +	/*
> +	 * If another vCPU already did the update while we were waiting
> +	 * for the lock, our request will have been cleared. Bail out.
> +	 */
> +	if (requesting_vcpu &&
> +	    !kvm_test_request(KVM_REQ_MASTERCLOCK_UPDATE, requesting_vcpu)) {
> +		kvm_clear_mclock_inprogress_request(kvm);
> +		raw_spin_unlock_irq(&kvm->arch.tsc_write_lock);
> +		return false;
> +	}
> +
> +	/* The update is VM-wide; prevent other vCPUs from redoing it. */
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		kvm_clear_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

[Severity: High]
Will this O(N) loop cause hardirq latency issues on large VMs?

By moving the kvm_for_each_vcpu() loop inside the raw_spin_lock_irq()
critical section, a VM with hundreds or thousands of vCPUs will execute
many atomic bit operations with interrupts disabled.

> +
>  	write_seqcount_begin(&kvm->arch.pvclock_sc);
> +	return true;
>  }
[ ... ]
> @@ -1730,22 +1759,25 @@ static void kvm_end_pvclock_update(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	unsigned long i;
>  
> -	write_seqcount_end(&ka->pvclock_sc);
> -	raw_spin_unlock_irq(&ka->tsc_write_lock);
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>  
>  	/* guest entries allowed */
> -	kvm_for_each_vcpu(i, vcpu, kvm)
> -		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
> +	kvm_clear_mclock_inprogress_request(kvm);
> +
> +	write_seqcount_end(&ka->pvclock_sc);
> +	raw_spin_unlock_irq(&ka->tsc_write_lock);
>  }

[Severity: High]
Does moving the seqcount and lock release to the end of the function
prolong the critical section too much?

The O(N) vCPU iteration loops are now happening before
write_seqcount_end() and before releasing the tsc_write_lock. This
substantially increases the IRQ-disabled critical section length.

Furthermore, won't holding the seqcount write lock across these loops force
reader vCPUs in kvm_guest_time_update() to spin indefinitely, wasting CPU
cycles and exacerbating latency?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728144954.355376-1-dwmw2@infradead.org?part=21