Re: [PATCH v2 1/1] KVM: Implement dirty quota-based throttling of vcpus

[Shivam Kumar <shivam.kumar1@nutanix.com>]

On 10/01/22 11:38 pm, Sean Christopherson wrote:
> On Mon, Dec 20, 2021, Shivam Kumar wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 9a2972fdae82..723f24909314 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10042,6 +10042,11 @@ static inline bool kvm_vcpu_running(struct kvm_vcpu *vcpu)
>>   		!vcpu->arch.apf.halted);
>>   }
>>   
>> +static inline bool is_dirty_quota_full(struct kvm_vcpu *vcpu)
>> +{
>> +	return (vcpu->stat.generic.dirty_count >= vcpu->run->dirty_quota);
>> +}
>> +
>>   static int vcpu_run(struct kvm_vcpu *vcpu)
>>   {
>>   	int r;
>> @@ -10079,6 +10084,18 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
>>   				return r;
>>   			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
>>   		}
>> +
>> +		/*
>> +		 * Exit to userspace when dirty quota is full (if dirty quota
>> +		 * throttling is enabled, i.e. dirty quota is non-zero).
>> +		 */
>> +		if (vcpu->run->dirty_quota > 0 && is_dirty_quota_full(vcpu)) {
> Kernel style is to omit the "> 0" when checking for non-zero.  It matters here
> because the "> 0" suggests dirty_quota can be negative, which it can't.
>
> To allow userspace to modify dirty_quota on the fly, run->dirty_quota should be
> READ_ONCE() with the result used for both the !0 and >= checks.  And then also
> capture the effective dirty_quota in the exit union struct (free from a memory
> perspective because the exit union is padded to 256 bytes).   That way if userspace
> wants to modify the dirty_quota while the vCPU running it will get coherent data
> even though the behavior is somewhat non-deterministic.
>
> And then to simplify the code and also make this logic reusable for other
> architectures, move it all into the helper and put the helper in kvm_host.h.
>
> For other architectures, unless the arch maintainers explicitly don't want to
> support this, I would prefer we enable at least arm64 right away to prevent this
> from becoming a de facto x86-only feature.  s390 also appears to be easy to support.
> I almost suggested moving the check to generic code, but then I looked at MIPS
> and PPC and lost all hope :-/
>
>> +			vcpu->run->exit_reason = KVM_EXIT_DIRTY_QUOTA_FULL;
>>
>>
>> --
>>
I am not able to test this on arm64 and s390 as I don't have access to 
arm64 and s390 hardware. Looking forward to your suggestions. Thank you!