Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200925212302.3979661-1-bgardon@google.com> <20200925212302.3979661-21-bgardon@google.com>
 <20200930181556.GJ32672@linux.intel.com> <d2bcf512-00f3-8499-420d-b31690bdb511@redhat.com>
In-Reply-To: <d2bcf512-00f3-8499-420d-b31690bdb511@redhat.com>
From:   Ben Gardon <bgardon@google.com>
Date:   Wed, 30 Sep 2020 15:33:50 -0700
Message-ID: <CANgfPd9h_Epb8qZZ6qCP5BKD0DOYv5v2NQm-J2ajq9_wXfWb1g@mail.gmail.com>
Subject: Re: [PATCH 20/22] kvm: mmu: NX largepage recovery for TDP MMU
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
> >>
> >
>