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

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Thu, Sep 15, 2022, Shivam Kumar wrote:
> @@ -542,6 +545,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)
> +{
> +	struct kvm_run *run = vcpu->run;
> +	u64 dirty_quota = READ_ONCE(run->dirty_quota);
> +	u64 pages_dirtied = vcpu->stat.generic.pages_dirtied;
> +
> +	if (!dirty_quota || (pages_dirtied < dirty_quota))
> +		return 1;
> +
> +	run->exit_reason = KVM_EXIT_DIRTY_QUOTA_EXHAUSTED;
> +	run->dirty_quota_exit.count = pages_dirtied;
> +	run->dirty_quota_exit.quota = dirty_quota;
> +	return 0;

Dead code.

> +}

...

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 584a5bab3af3..f315af50037d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3298,18 +3298,36 @@ int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len)
>  }
>  EXPORT_SYMBOL_GPL(kvm_clear_guest);
>  
> +static void kvm_vcpu_is_dirty_quota_exhausted(struct kvm_vcpu *vcpu)

Ouch, sorry.  I suspect you got this name from me[*].  That was a goof on my end,
I'm 99% certain I copy-pasted stale code, i.e. didn't intended to suggest a
rename.

Let's keep kvm_vcpu_check_dirty_quota(), IMO that's still the least awful name.

[*] https://lore.kernel.org/all/Yo+82LjHSOdyxKzT@xxxxxxxxxx

> +{
> +	u64 dirty_quota = READ_ONCE(vcpu->run->dirty_quota);
> +
> +	if (!dirty_quota || (vcpu->stat.generic.pages_dirtied < dirty_quota))
> +		return;
> +
> +	/*
> +	 * Snapshot the quota to report it to userspace.  The dirty count will be
> +	 * captured when the request is processed.
> +	 */
> +	vcpu->dirty_quota = dirty_quota;
> +	kvm_make_request(KVM_REQ_DIRTY_QUOTA_EXIT, vcpu);

Making the request needs to be guarded with an arch opt-in.  Pending requests
prevent KVM from entering the guest, and so making a request that an arch isn't
aware of will effectively hang the vCPU.  Obviously userspace would be shooting
itself in the foot by setting run->dirty_quota in this case, but KVM shouldn't
hand userspace a loaded gun and help them aim.

My suggestion from v1[*] about not forcing architectures to opt-in was in the
context of a request-less implementation where dirty_quota was a nop until the
arch took action.

And regardless of arch opt-in, I think this needs a capability so that userspace
can detect KVM support.

I don't see any reason to wrap the request or vcpu_run field, e.g. something like
this should suffice:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ea5847d22aff..93362441215b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3298,8 +3298,9 @@ int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len)
 }
 EXPORT_SYMBOL_GPL(kvm_clear_guest);
 
-static void kvm_vcpu_is_dirty_quota_exhausted(struct kvm_vcpu *vcpu)
+static void kvm_vcpu_check_dirty_quota(struct kvm_vcpu *vcpu)
 {
+#ifdef CONFIG_HAVE_KVM_DIRTY_QUOTA
        u64 dirty_quota = READ_ONCE(vcpu->run->dirty_quota);
 
        if (!dirty_quota || (vcpu->stat.generic.pages_dirtied < dirty_quota))
@@ -3311,6 +3312,7 @@ static void kvm_vcpu_is_dirty_quota_exhausted(struct kvm_vcpu *vcpu)
         */
        vcpu->dirty_quota = dirty_quota;
        kvm_make_request(KVM_REQ_DIRTY_QUOTA_EXIT, vcpu);
+#endif
 }
 
 void mark_page_dirty_in_slot(struct kvm *kvm,
@@ -4507,6 +4509,8 @@ static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
        case KVM_CAP_BINARY_STATS_FD:
        case KVM_CAP_SYSTEM_EVENT_DATA:
                return 1;
+       case KVM_CAP_DIRTY_QUOTA:
+               return !!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_QUOTA);
        default:
                break;
        }


[*] https://lore.kernel.org/all/YZaUENi0ZyQi%2F9M0@xxxxxxxxxx