From: Shivam Kumar <shivam.kumar1@nutanix.com>
To: Sean Christopherson <seanjc@google.com>
Cc: pbonzini@redhat.com, maz@kernel.org, james.morse@arm.com,
borntraeger@linux.ibm.com, david@redhat.com, kvm@vger.kernel.org,
Shaju Abraham <shaju.abraham@nutanix.com>,
Manish Mishra <manish.mishra@nutanix.com>,
Anurag Madnawat <anurag.madnawat@nutanix.com>
Subject: Re: [PATCH v6 2/5] KVM: x86: Dirty quota-based throttling of vcpus
Date: Tue, 18 Oct 2022 23:13:54 +0530 [thread overview]
Message-ID: <a835d5c3-8742-e8f7-e810-a69a139c4349@nutanix.com> (raw)
In-Reply-To: <c6700c5d-d942-065b-411c-7f4723836054@nutanix.com>
>>> @@ -10379,6 +10379,15 @@ static int vcpu_enter_guest(struct kvm_vcpu
>>> *vcpu)
>>> r = 0;
>>> goto out;
>>> }
>>> + if (kvm_check_request(KVM_REQ_DIRTY_QUOTA_EXIT, vcpu)) {
>>> + struct kvm_run *run = vcpu->run;
>>> +
>>
>>> + run->exit_reason = KVM_EXIT_DIRTY_QUOTA_EXHAUSTED;
>>> + run->dirty_quota_exit.count =
>>> vcpu->stat.generic.pages_dirtied;
>>> + run->dirty_quota_exit.quota = vcpu->dirty_quota;
>>
>> As mentioned in patch 1, the code code snippet I suggested is bad.
>> With a request,
>> there's no need to snapshot the quota prior to making the request. If
>> userspace
>> changes the quota, then using the new quota is perfectly ok since KVM
>> is still
>> providing sane data. If userspace lowers the quota, an exit will
>> still ocurr as
>> expected. If userspace disables or increases the quota to the point
>> where no exit
>> is necessary, then userspace can't expect and exit and won't even be
>> aware that KVM
>> temporarily detected an exhausted quota.
>>
>> And all of this belongs in a common KVM helper. Name isn't pefect,
>> but it aligns
>> with kvm_check_request().
>>
>> #ifdef CONFIG_HAVE_KVM_DIRTY_QUOTA
>> static inline bool kvm_check_dirty_quota_request(struct kvm_vcpu *vcpu)
>> {
>> struct kvm_run *run = vcpu->run;
>>
>> run->exit_reason = KVM_EXIT_DIRTY_QUOTA_EXHAUSTED;
>> run->dirty_quota_exit.count = vcpu->stat.generic.pages_dirtied;
>> run->dirty_quota_exit.quota = READ_ONCE(run->dirty_quota);
>>
>> /*
>> * Re-check the quota and exit if and only if the vCPU still
>> exceeds its
>> * quota. If userspace increases (or disables entirely) the
>> quota, then
>> * no exit is required as KVM is still honoring its ABI, e.g.
>> userspace
>> * won't even be aware that KVM temporarily detected an exhausted
>> quota.
>> */
>> return run->dirty_quota_exit.count >= run->dirty_quota_exit.quota;
>> }
>> #endif
>>
>> And then arch usage is simply something like:
>>
>> if (kvm_check_dirty_quota_request(vcpu)) {
>> r = 0;
>> goto out;
>> }
> If we are not even checking for request KVM_REQ_DIRTY_QUOTA_EXIT, what's
> the use of kvm_make_request in patch 1?
Ok, so we don't need to explicitely check for request here because we
are checking the quota directly but we do need to make the request when
the quota is exhausted so as to guarantee that we enter the if block "if
(kvm_request_pending(vcpu))".
Please let me know if my understanding is correct or if I am missing
something.
Also, should I add ifdef here:
#ifdef CONFIG_HAVE_KVM_DIRTY_QUOTA
if (kvm_check_dirty_quota_request(vcpu)) {
r = 0;
goto out;
}
#endif
Or should I bring the ifdef inside kvm_check_dirty_quota_request and
return false if not defined.
static inline bool kvm_check_dirty_quota_request(struct kvm_vcpu *vcpu)
{
#ifdef CONFIG_HAVE_KVM_DIRTY_QUOTA
struct kvm_run *run = vcpu->run;
run->exit_reason = KVM_EXIT_DIRTY_QUOTA_EXHAUSTED;
run->dirty_quota_exit.count = vcpu->stat.generic.pages_dirtied;
run->dirty_quota_exit.quota = READ_ONCE(run->dirty_quota);
/*
* Re-check the quota and exit if and only if the vCPU still exceeds its
* quota. If userspace increases (or disables entirely) the quota, then
* no exit is required as KVM is still honoring its ABI, e.g. userspace
* won't even be aware that KVM temporarily detected an exhausted quota.
*/
return run->dirty_quota_exit.count >= run->dirty_quota_exit.quota;
#else
return false;
#endif
}
Thanks a lot for the reviews so far. Looking forward to your reply.
next prev parent reply other threads:[~2022-10-18 17:44 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-15 10:10 [PATCH v6 0/5] KVM: Dirty quota-based throttling Shivam Kumar
2022-09-15 10:10 ` [PATCH v6 1/5] KVM: Implement dirty quota-based throttling of vcpus Shivam Kumar
2022-09-15 13:21 ` Christian Borntraeger
2022-09-15 14:34 ` Christian Borntraeger
2022-10-07 18:18 ` Sean Christopherson
2022-10-09 18:36 ` Shivam Kumar
2022-10-10 6:12 ` Christian Borntraeger
2022-10-07 19:08 ` Sean Christopherson
2022-10-07 19:20 ` Sean Christopherson
2022-10-09 18:49 ` Shivam Kumar
2022-10-10 16:09 ` Sean Christopherson
2022-10-09 19:30 ` Shivam Kumar
2022-10-10 5:41 ` Shivam Kumar
2022-10-17 5:28 ` Shivam Kumar
2022-10-19 16:01 ` Sean Christopherson
2022-09-15 10:10 ` [PATCH v6 2/5] KVM: x86: Dirty " Shivam Kumar
2022-10-07 19:30 ` Sean Christopherson
2022-10-09 19:05 ` Shivam Kumar
2022-10-18 17:43 ` Shivam Kumar [this message]
2022-10-19 15:42 ` Sean Christopherson
2022-10-09 19:17 ` Shivam Kumar
2022-09-15 10:10 ` [PATCH v6 3/5] KVM: arm64: " Shivam Kumar
2022-09-15 10:10 ` [PATCH v6 4/5] KVM: s390x: " Shivam Kumar
2022-09-15 13:24 ` Christian Borntraeger
2022-09-15 10:10 ` [PATCH v6 5/5] KVM: selftests: Add selftests for dirty quota throttling Shivam Kumar
2022-10-07 18:29 ` Sean Christopherson
2022-10-09 19:26 ` Shivam Kumar
2022-10-10 15:47 ` Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a835d5c3-8742-e8f7-e810-a69a139c4349@nutanix.com \
--to=shivam.kumar1@nutanix.com \
--cc=anurag.madnawat@nutanix.com \
--cc=borntraeger@linux.ibm.com \
--cc=david@redhat.com \
--cc=james.morse@arm.com \
--cc=kvm@vger.kernel.org \
--cc=manish.mishra@nutanix.com \
--cc=maz@kernel.org \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=shaju.abraham@nutanix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.