* [PATCH][v2] KVM: x86/mmu: fix counting of rmap entries in pte_list_add
@ 2020-09-23 4:58 Li RongQing
2020-09-25 16:43 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Li RongQing @ 2020-09-23 4:58 UTC (permalink / raw)
To: lirongqing, kvm, x86, sean.j.christopherson
counting of rmap entries was missed when desc->sptes is full
and desc->more is NULL
and merging two PTE_LIST_EXT-1 check as one, to avoids the
extra comparison to give slightly optimization
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
diff with v1: merge two check as one
arch/x86/kvm/mmu/mmu.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index a5d0207e7189..c4068be6bb3f 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1273,12 +1273,14 @@ static int pte_list_add(struct kvm_vcpu *vcpu, u64 *spte,
} else {
rmap_printk("pte_list_add: %p %llx many->many\n", spte, *spte);
desc = (struct pte_list_desc *)(rmap_head->val & ~1ul);
- while (desc->sptes[PTE_LIST_EXT-1] && desc->more) {
- desc = desc->more;
+ while (desc->sptes[PTE_LIST_EXT-1]) {
count += PTE_LIST_EXT;
- }
- if (desc->sptes[PTE_LIST_EXT-1]) {
- desc->more = mmu_alloc_pte_list_desc(vcpu);
+
+ if (!desc->more) {
+ desc->more = mmu_alloc_pte_list_desc(vcpu);
+ desc = desc->more;
+ break;
+ }
desc = desc->more;
}
for (i = 0; desc->sptes[i]; ++i)
--
2.16.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH][v2] KVM: x86/mmu: fix counting of rmap entries in pte_list_add
2020-09-23 4:58 [PATCH][v2] KVM: x86/mmu: fix counting of rmap entries in pte_list_add Li RongQing
@ 2020-09-25 16:43 ` Sean Christopherson
2020-09-26 6:34 ` Li,Rongqing
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2020-09-25 16:43 UTC (permalink / raw)
To: Li RongQing; +Cc: kvm, x86
On Wed, Sep 23, 2020 at 12:58:58PM +0800, Li RongQing wrote:
> counting of rmap entries was missed when desc->sptes is full
> and desc->more is NULL
>
> and merging two PTE_LIST_EXT-1 check as one, to avoids the
> extra comparison to give slightly optimization
Please write complete sentences, and use proper capitalization and punctuation.
It's not a big deal for short changelogs, but it's crucial for readability of
larger changelogs.
E.g.
Fix an off-by-one style bug in pte_list_add() where it failed to account
the last full set of SPTEs, i.e. when desc->sptes is full and desc->more
is NULL.
Merge the two "PTE_LIST_EXT-1" checks as part of the fix to avoid an
extra comparison.
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
No need to give me credit, I just nitpicked the code, identifying the bug
and the fix was all you. :-)
Thanks for the fix!
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
Paolo,
Although it's a bug fix, I don't think this needs a Fixes / Cc:stable. The bug
only results in rmap recycling being delayed by one rmap. Stable kernels can
probably live with an off-by-one bug given that RMAP_RECYCLE_THRESHOLD is
completely arbitrary. :-)
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> diff with v1: merge two check as one
>
> arch/x86/kvm/mmu/mmu.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index a5d0207e7189..c4068be6bb3f 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1273,12 +1273,14 @@ static int pte_list_add(struct kvm_vcpu *vcpu, u64 *spte,
> } else {
> rmap_printk("pte_list_add: %p %llx many->many\n", spte, *spte);
> desc = (struct pte_list_desc *)(rmap_head->val & ~1ul);
> - while (desc->sptes[PTE_LIST_EXT-1] && desc->more) {
> - desc = desc->more;
> + while (desc->sptes[PTE_LIST_EXT-1]) {
> count += PTE_LIST_EXT;
> - }
> - if (desc->sptes[PTE_LIST_EXT-1]) {
> - desc->more = mmu_alloc_pte_list_desc(vcpu);
> +
> + if (!desc->more) {
> + desc->more = mmu_alloc_pte_list_desc(vcpu);
> + desc = desc->more;
> + break;
> + }
> desc = desc->more;
> }
> for (i = 0; desc->sptes[i]; ++i)
> --
> 2.16.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [PATCH][v2] KVM: x86/mmu: fix counting of rmap entries in pte_list_add
2020-09-25 16:43 ` Sean Christopherson
@ 2020-09-26 6:34 ` Li,Rongqing
0 siblings, 0 replies; 3+ messages in thread
From: Li,Rongqing @ 2020-09-26 6:34 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm@vger.kernel.org, x86@kernel.org
> -----Original Message-----
> From: Sean Christopherson [mailto:sean.j.christopherson@intel.com]
> Sent: Saturday, September 26, 2020 12:44 AM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: kvm@vger.kernel.org; x86@kernel.org
> Subject: Re: [PATCH][v2] KVM: x86/mmu: fix counting of rmap entries in
> pte_list_add
>
> On Wed, Sep 23, 2020 at 12:58:58PM +0800, Li RongQing wrote:
> > counting of rmap entries was missed when desc->sptes is full and
> > desc->more is NULL
> >
> > and merging two PTE_LIST_EXT-1 check as one, to avoids the extra
> > comparison to give slightly optimization
>
> Please write complete sentences, and use proper capitalization and
> punctuation.
> It's not a big deal for short changelogs, but it's crucial for readability of larger
> changelogs.
>
> E.g.
>
> Fix an off-by-one style bug in pte_list_add() where it failed to account
> the last full set of SPTEs, i.e. when desc->sptes is full and desc->more
> is NULL.
>
> Merge the two "PTE_LIST_EXT-1" checks as part of the fix to avoid an
> extra comparison.
>
> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
>
> No need to give me credit, I just nitpicked the code, identifying the bug and the
> fix was all you. :-)
>
> Thanks for the fix!
>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
>
> Paolo,
>
> Although it's a bug fix, I don't think this needs a Fixes / Cc:stable. The bug
> only results in rmap recycling being delayed by one rmap. Stable kernels can
> probably live with an off-by-one bug given that RMAP_RECYCLE_THRESHOLD is
> completely arbitrary. :-)
>
> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
>
Thank you very much , I will send V3
-Li
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-26 6:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-23 4:58 [PATCH][v2] KVM: x86/mmu: fix counting of rmap entries in pte_list_add Li RongQing
2020-09-25 16:43 ` Sean Christopherson
2020-09-26 6:34 ` Li,Rongqing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox