Re: [PATCH] KVM: x86/mmu: Make page tables for eager page splitting NUMA aware

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/2/22 21:12, Sean Christopherson wrote:
Ah crud, I misread the patch.  I was thinking tdp_mmu_spte_to_nid() was getting
the node for the existing shadow page, but it's actually getting the node for the
underlying physical huge page.

That means this patch is broken now that KVM doesn't require a "struct page" to
create huge SPTEs (commit  a8ac499bb6ab ("KVM: x86/mmu: Don't require refcounted
"struct page" to create huge SPTEs").  I.e. this will explode as pfn_to_page()
may return garbage.

	return page_to_nid(pfn_to_page(spte_to_pfn(spte)));

I was about to say that yesterday. However my knowledge of struct page things has been proved to be spotty enough, that I wasn't 100% sure of that. But yeah, with a fresh mind it's quite obvious that anything that goes through hva_to_pfn_remap and similar paths will fail.

That said, I still don't like this patch, at least not as a one-off thing.  I do
like the idea of having page table allocations follow the node requested for the
target pfn, what I don't like is having one-off behavior that silently overrides
userspace policy.

Yes, I totally agree with making it all or nothing.

I would much rather we solve the problem for all page table allocations, maybe
with a KVM_CAP or module param?  Unfortunately, that's a very non-trivial change
because it will probably require having a per-node cache in each of the per-vCPU
caches.

A module parameter is fine. If you care about performance to this level, your userspace is probably homogeneous enough.

Paolo

Hmm, actually, a not-awful alternative would be to have the fault path fallback
to the current task's policy if allocation fails.  I.e. make it best effort.

E.g. as a rough sketch...

---
  arch/x86/kvm/mmu/tdp_mmu.c | 37 ++++++++++++++++++++++++++++++-------
  1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index bf2ccf9debca..e475f5b55794 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -273,10 +273,11 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,

  static struct kvm_mmu_page *tdp_mmu_alloc_sp(struct kvm_vcpu *vcpu)
  {
+	struct kvm_mmu_memory_cache *cache = &vcpu->arch.mmu_shadow_page_cache;
  	struct kvm_mmu_page *sp;

  	sp = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache);
-	sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
+	sp->spt = kvm_mmu_alloc_shadow_page_table(cache, GFP_NOWAIT, pfn);

  	return sp;
  }
@@ -1190,7 +1191,7 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
  			if (is_removed_spte(iter.old_spte))
  				break;

-			sp = tdp_mmu_alloc_sp(vcpu);
+			sp = tdp_mmu_alloc_sp(vcpu, fault->pfn);
  			tdp_mmu_init_child_sp(sp, &iter);

  			if (tdp_mmu_link_sp(vcpu->kvm, &iter, sp, account_nx, true)) {
@@ -1402,17 +1403,39 @@ bool kvm_tdp_mmu_wrprot_slot(struct kvm *kvm,
  	return spte_set;
  }

-static struct kvm_mmu_page *__tdp_mmu_alloc_sp_for_split(gfp_t gfp)
+void *kvm_mmu_alloc_shadow_page_table(struct kvm_mmu_memory_cache *cache,
+				      gfp_t gfp, kvm_pfn_t pfn)
+{
+	struct page *page = kvm_pfn_to_refcounted_page(pfn);
+	struct page *spt_page;
+	int node;
+
+	gfp |= __GFP_ZERO | __GFP_ACCOUNT;
+
+	if (page) {
+		spt_page = alloc_pages_node(page_to_nid(page), gfp, 0);
+		if (spt_page)
+			return page_address(spt_page);
+	} else if (!cache) {
+		return (void *)__get_free_page(gfp);
+	}
+
+	if (cache)
+		return kvm_mmu_memory_cache_alloc(cache);
+
+	return NULL;
+}
+
+static struct kvm_mmu_page *__tdp_mmu_alloc_sp_for_split(gfp_t gfp,
+							 kvm_pfn_t pfn)
  {
  	struct kvm_mmu_page *sp;

-	gfp |= __GFP_ZERO;
-
  	sp = kmem_cache_alloc(mmu_page_header_cache, gfp);
  	if (!sp)
  		return NULL;

-	sp->spt = (void *)__get_free_page(gfp);
+	sp->spt = kvm_mmu_alloc_shadow_page_table(NULL, gfp, pfn);
  	if (!sp->spt) {
  		kmem_cache_free(mmu_page_header_cache, sp);
  		return NULL;
@@ -1436,7 +1459,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
  	 * If this allocation fails we drop the lock and retry with reclaim
  	 * allowed.
  	 */
-	sp = __tdp_mmu_alloc_sp_for_split(GFP_NOWAIT | __GFP_ACCOUNT);
+	sp = __tdp_mmu_alloc_sp_for_split(GFP_NOWAIT);
  	if (sp)
  		return sp;


base-commit: f8990bfe1eab91c807ca8fc0d48705e8f986b951
--





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux