From: Ben Gardon <bgardon@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>,
LKML <linux-kernel@vger.kernel.org>, kvm <kvm@vger.kernel.org>,
Cannon Matthews <cannonmatthews@google.com>,
Peter Xu <peterx@redhat.com>, Peter Shier <pshier@google.com>,
Peter Feiner <pfeiner@google.com>,
Junaid Shahid <junaids@google.com>,
Jim Mattson <jmattson@google.com>,
Yulei Zhang <yulei.kernel@gmail.com>,
Wanpeng Li <kernellwp@gmail.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Xiao Guangrong <xiaoguangrong.eric@gmail.com>
Subject: Re: [PATCH 20/22] kvm: mmu: NX largepage recovery for TDP MMU
Date: Wed, 30 Sep 2020 15:33:50 -0700 [thread overview]
Message-ID: <CANgfPd9h_Epb8qZZ6qCP5BKD0DOYv5v2NQm-J2ajq9_wXfWb1g@mail.gmail.com> (raw)
In-Reply-To: <d2bcf512-00f3-8499-420d-b31690bdb511@redhat.com>
On Wed, Sep 30, 2020 at 12:56 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 30/09/20 20:15, Sean Christopherson wrote:
> > On Fri, Sep 25, 2020 at 02:23:00PM -0700, Ben Gardon wrote:
> >> +/*
> >> + * Clear non-leaf SPTEs and free the page tables they point to, if those SPTEs
> >> + * exist in order to allow execute access on a region that would otherwise be
> >> + * mapped as a large page.
> >> + */
> >> +void kvm_tdp_mmu_recover_nx_lpages(struct kvm *kvm)
> >> +{
> >> + struct kvm_mmu_page *sp;
> >> + bool flush;
> >> + int rcu_idx;
> >> + unsigned int ratio;
> >> + ulong to_zap;
> >> + u64 old_spte;
> >> +
> >> + rcu_idx = srcu_read_lock(&kvm->srcu);
> >> + spin_lock(&kvm->mmu_lock);
> >> +
> >> + ratio = READ_ONCE(nx_huge_pages_recovery_ratio);
> >> + to_zap = ratio ? DIV_ROUND_UP(kvm->stat.nx_lpage_splits, ratio) : 0;
> >
> > This is broken, and possibly related to Paolo's INIT_LIST_HEAD issue. The TDP
> > MMU never increments nx_lpage_splits, it instead has its own counter,
> > tdp_mmu_lpage_disallowed_page_count. Unless I'm missing something, to_zap is
> > guaranteed to be zero and thus this is completely untested.
>
> Except if you do shadow paging (through nested EPT) and then it bombs
> immediately. :)
>
> > I don't see any reason for a separate tdp_mmu_lpage_disallowed_page_count,
> > a single VM can't have both a legacy MMU and a TDP MMU, so it's not like there
> > will be collisions with other code incrementing nx_lpage_splits. And the TDP
> > MMU should be updating stats anyways.
>
> This is true, but having two counters is necessary (in the current
> implementation) because otherwise you zap more than the requested ratio
> of pages.
>
> The simplest solution is to add a "bool tdp_page" to struct
> kvm_mmu_page, so that you can have a single list of
> lpage_disallowed_pages and a single thread. The while loop can then
> dispatch to the right "zapper" code.
I actually did add that bool in patch 4: kvm: mmu: Allocate and free
TDP MMU roots.
I'm a little nervous about putting them in the same list, but I agree
it would definitely simplify the implementation of reclaim.
>
> Anyway this patch is completely broken, so let's kick it away to the
> next round.
Understood, sorry I didn't test this one better. I'll incorporate your
feedback and include it in the next series.
>
> Paolo
>
> >> +
> >> + while (to_zap &&
> >> + !list_empty(&kvm->arch.tdp_mmu_lpage_disallowed_pages)) {
> >> + /*
> >> + * We use a separate list instead of just using active_mmu_pages
> >> + * because the number of lpage_disallowed pages is expected to
> >> + * be relatively small compared to the total.
> >> + */
> >> + sp = list_first_entry(&kvm->arch.tdp_mmu_lpage_disallowed_pages,
> >> + struct kvm_mmu_page,
> >> + lpage_disallowed_link);
> >> +
> >> + old_spte = *sp->parent_sptep;
> >> + *sp->parent_sptep = 0;
> >> +
> >> + list_del(&sp->lpage_disallowed_link);
> >> + kvm->arch.tdp_mmu_lpage_disallowed_page_count--;
> >> +
> >> + handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), sp->gfn,
> >> + old_spte, 0, sp->role.level + 1);
> >> +
> >> + flush = true;
> >> +
> >> + if (!--to_zap || need_resched() ||
> >> + spin_needbreak(&kvm->mmu_lock)) {
> >> + flush = false;
> >> + kvm_flush_remote_tlbs(kvm);
> >> + if (to_zap)
> >> + cond_resched_lock(&kvm->mmu_lock);
> >> + }
> >> + }
> >> +
> >> + if (flush)
> >> + kvm_flush_remote_tlbs(kvm);
> >> +
> >> + spin_unlock(&kvm->mmu_lock);
> >> + srcu_read_unlock(&kvm->srcu, rcu_idx);
> >> +}
> >> +
> >> diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> >> index 2ecb047211a6d..45ea2d44545db 100644
> >> --- a/arch/x86/kvm/mmu/tdp_mmu.h
> >> +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> >> @@ -43,4 +43,6 @@ void kvm_tdp_mmu_zap_collapsible_sptes(struct kvm *kvm,
> >>
> >> bool kvm_tdp_mmu_write_protect_gfn(struct kvm *kvm,
> >> struct kvm_memory_slot *slot, gfn_t gfn);
> >> +
> >> +void kvm_tdp_mmu_recover_nx_lpages(struct kvm *kvm);
> >> #endif /* __KVM_X86_MMU_TDP_MMU_H */
> >> --
> >> 2.28.0.709.gb0816b6eb0-goog
> >>
> >
>
next prev parent reply other threads:[~2020-09-30 22:34 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-25 21:22 [PATCH 00/22] Introduce the TDP MMU Ben Gardon
2020-09-25 21:22 ` [PATCH 01/22] kvm: mmu: Separate making SPTEs from set_spte Ben Gardon
2020-09-30 4:55 ` Sean Christopherson
2020-09-30 23:03 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 02/22] kvm: mmu: Introduce tdp_iter Ben Gardon
2020-09-26 0:04 ` Paolo Bonzini
2020-09-30 5:06 ` Sean Christopherson
2020-09-26 0:54 ` Paolo Bonzini
2020-09-30 5:08 ` Sean Christopherson
2020-09-30 5:24 ` Sean Christopherson
2020-09-30 6:24 ` Paolo Bonzini
2020-09-30 23:20 ` Eric van Tassell
2020-09-30 23:34 ` Paolo Bonzini
2020-10-01 0:07 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU Ben Gardon
2020-09-26 0:06 ` Paolo Bonzini
2020-09-30 5:34 ` Sean Christopherson
2020-09-30 18:36 ` Ben Gardon
2020-09-30 16:57 ` Sean Christopherson
2020-09-30 17:39 ` Paolo Bonzini
2020-09-30 18:42 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 04/22] kvm: mmu: Allocate and free TDP MMU roots Ben Gardon
2020-09-30 6:06 ` Sean Christopherson
2020-09-30 6:26 ` Paolo Bonzini
2020-09-30 15:38 ` Sean Christopherson
2020-10-12 22:59 ` Ben Gardon
2020-10-12 23:59 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 05/22] kvm: mmu: Add functions to handle changed TDP SPTEs Ben Gardon
2020-09-26 0:39 ` Paolo Bonzini
2020-09-28 17:23 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 06/22] kvm: mmu: Make address space ID a property of memslots Ben Gardon
2020-09-30 6:10 ` Sean Christopherson
2020-09-30 23:11 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 07/22] kvm: mmu: Support zapping SPTEs in the TDP MMU Ben Gardon
2020-09-26 0:14 ` Paolo Bonzini
2020-09-30 6:15 ` Sean Christopherson
2020-09-30 6:28 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 08/22] kvm: mmu: Separate making non-leaf sptes from link_shadow_page Ben Gardon
2020-09-25 21:22 ` [PATCH 09/22] kvm: mmu: Remove disallowed_hugepage_adjust shadow_walk_iterator arg Ben Gardon
2020-09-30 16:19 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 10/22] kvm: mmu: Add TDP MMU PF handler Ben Gardon
2020-09-26 0:24 ` Paolo Bonzini
2020-09-30 16:37 ` Sean Christopherson
2020-09-30 16:55 ` Paolo Bonzini
2020-09-30 17:37 ` Paolo Bonzini
2020-10-06 22:35 ` Ben Gardon
2020-10-06 22:33 ` Ben Gardon
2020-10-07 20:55 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 11/22] kvm: mmu: Factor out allocating a new tdp_mmu_page Ben Gardon
2020-09-26 0:22 ` Paolo Bonzini
2020-09-30 18:53 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 12/22] kvm: mmu: Allocate struct kvm_mmu_pages for all pages in TDP MMU Ben Gardon
2020-09-25 21:22 ` [PATCH 13/22] kvm: mmu: Support invalidate range MMU notifier for " Ben Gardon
2020-09-30 17:03 ` Sean Christopherson
2020-09-30 23:15 ` Ben Gardon
2020-09-30 23:24 ` Sean Christopherson
2020-09-30 23:27 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 14/22] kvm: mmu: Add access tracking for tdp_mmu Ben Gardon
2020-09-26 0:32 ` Paolo Bonzini
2020-09-30 17:48 ` Sean Christopherson
2020-10-06 23:38 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU Ben Gardon
2020-09-26 0:33 ` Paolo Bonzini
2020-09-28 15:11 ` Paolo Bonzini
2020-10-07 16:53 ` Ben Gardon
2020-10-07 17:18 ` Paolo Bonzini
2020-10-07 17:30 ` Ben Gardon
2020-10-07 17:54 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 16/22] kvm: mmu: Add dirty logging handler for changed sptes Ben Gardon
2020-09-26 0:45 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 17/22] kvm: mmu: Support dirty logging for the TDP MMU Ben Gardon
2020-09-26 1:04 ` Paolo Bonzini
2020-10-08 18:27 ` Ben Gardon
2020-09-29 15:07 ` Paolo Bonzini
2020-09-30 18:04 ` Sean Christopherson
2020-09-30 18:08 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 18/22] kvm: mmu: Support disabling dirty logging for the tdp MMU Ben Gardon
2020-09-26 1:09 ` Paolo Bonzini
2020-10-07 16:30 ` Ben Gardon
2020-10-07 17:21 ` Paolo Bonzini
2020-10-07 17:28 ` Ben Gardon
2020-10-07 17:53 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 19/22] kvm: mmu: Support write protection for nesting in " Ben Gardon
2020-09-30 18:06 ` Sean Christopherson
2020-09-25 21:23 ` [PATCH 20/22] kvm: mmu: NX largepage recovery for TDP MMU Ben Gardon
2020-09-26 1:14 ` Paolo Bonzini
2020-09-30 22:23 ` Ben Gardon
2020-09-29 18:24 ` Paolo Bonzini
2020-09-30 18:15 ` Sean Christopherson
2020-09-30 19:56 ` Paolo Bonzini
2020-09-30 22:33 ` Ben Gardon [this message]
2020-09-30 22:27 ` Ben Gardon
2020-09-25 21:23 ` [PATCH 21/22] kvm: mmu: Support MMIO in the " Ben Gardon
2020-09-30 18:19 ` Sean Christopherson
2020-09-25 21:23 ` [PATCH 22/22] kvm: mmu: Don't clear write flooding count for direct roots Ben Gardon
2020-09-26 1:25 ` Paolo Bonzini
2020-10-05 22:48 ` Ben Gardon
2020-10-05 23:44 ` Sean Christopherson
2020-10-06 16:19 ` Ben Gardon
2020-09-26 1:14 ` [PATCH 00/22] Introduce the TDP MMU Paolo Bonzini
2020-09-28 17:31 ` Paolo Bonzini
2020-09-29 17:40 ` Ben Gardon
2020-09-29 18:10 ` Paolo Bonzini
2020-09-30 6:19 ` Sean Christopherson
2020-09-30 6:30 ` Paolo Bonzini
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=CANgfPd9h_Epb8qZZ6qCP5BKD0DOYv5v2NQm-J2ajq9_wXfWb1g@mail.gmail.com \
--to=bgardon@google.com \
--cc=cannonmatthews@google.com \
--cc=jmattson@google.com \
--cc=junaids@google.com \
--cc=kernellwp@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=peterx@redhat.com \
--cc=pfeiner@google.com \
--cc=pshier@google.com \
--cc=sean.j.christopherson@intel.com \
--cc=vkuznets@redhat.com \
--cc=xiaoguangrong.eric@gmail.com \
--cc=yulei.kernel@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).