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

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

On 31/03/22 5:58 am, Sean Christopherson wrote:
> This whole series needs to be Cc'd to the arm64 and s390 folks.  The easiest way
> to that is to use scripts/get_maintainers.pl, which will grab the appropriate
> people.  There are a variety of options you can use to tailor it to your style.
> E.g. for KVM I do
>
>    --nogit --nogit-fallback --norolestats --nofixes --pattern-depth=1
>
> for To:, and then add
>
>    --nom
>
> for Cc:.  The --pattern-depth=1 tells it to not recurse up so that it doesn't
> include the x86 maintainers for arch/x86/kvm patches.
>
> I'd Cc them manually, but I think it'll be easier to just post v4.

Thanks. I'm waiting for some reviews on the selftests (the third patch 
of this series). As
soon as I receive some, I'll send v4.

>
> On Sun, Mar 06, 2022, Shivam Kumar wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index eb4029660bd9..0b35b8cc0274 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10257,6 +10257,10 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
>>   	vcpu->arch.l1tf_flush_l1d = true;
>>   
>>   	for (;;) {
>> +		r = kvm_vcpu_check_dirty_quota(vcpu);
>> +		if (!r)
>> +			break;
>> +
>>   		if (kvm_vcpu_running(vcpu)) {
>>   			r = vcpu_enter_guest(vcpu);
>>   		} else {
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index f11039944c08..b1c599c78c42 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -530,6 +530,21 @@ static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
>>   	return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
>>   }
>>   
>> +static inline int kvm_vcpu_check_dirty_quota(struct kvm_vcpu *vcpu)
>> +{
>> +	u64 dirty_quota = READ_ONCE(vcpu->run->dirty_quota);
>> +	u64 pages_dirtied = vcpu->stat.generic.pages_dirtied;
>> +	struct kvm_run *run = vcpu->run;
> Might as well use "run" when reading the dirty quota.
Sure. Thanks.
>
>> +
>> +	if (!dirty_quota || (pages_dirtied < dirty_quota))
>> +		return 1;
> I don't love returning 0/1 from a function that suggests it returns a bool, but
> I do agree it's better than actually returning a bool.  I also don't have a better
> name, so I'm just whining in the hope that Paolo or someone else has an idea :-)
I've seen plenty of check functions returning 0/1 but please do let me 
know if there's
a convention to use a bool in such scenarios. I'm also looking for a 
better name but
this one also looks good enough to me.
>> +	run->exit_reason = KVM_EXIT_DIRTY_QUOTA_EXHAUSTED;
>> +	run->dirty_quota_exit.count = pages_dirtied;
>> +	run->dirty_quota_exit.quota = dirty_quota;
>> +	return 0;
>> +}