Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 1 Aug 2022 15:10:23 -0700
From:   David Matlack <dmatlack@google.com>
To:     Vipin Sharma <vipinsh@google.com>
Cc:     seanjc@google.com, pbonzini@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: x86/mmu: Make page tables for eager page splitting
 NUMA aware
Message-ID: <YuhPT2drgqL+osLl@google.com>
References: <20220801151928.270380-1-vipinsh@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220801151928.270380-1-vipinsh@google.com>
Precedence: bulk

On Mon, Aug 01, 2022 at 08:19:28AM -0700, Vipin Sharma wrote:
>

In the shortlog, clarify that this only applies to the TDP MMU.

> tdp_mmu_alloc_sp_for_split() allocates page tables for Eager Page
> Splitting.  Currently it does not specify a NUMA node preference, so it
> will try to allocate from the local node. The thread doing eager page

nit: Instead of "try to allocate from the local node", say something
like "allocate using the current threads mempolicy, which is most likely
going to be MPOL_LOCAL, i.e. allocate from the local node."

> splitting is supplied by the userspace and may not be running on the same
> node where it would be best for page tables to be allocated.

Suggest comparing eager page splitting to vCPU faults, which is the
other place that TDP page tables are allocated. e.g.

  Note that TDP page tables allocated during fault handling are less
  likely to suffer from the same NUMA locality issue because they will
  be allocated on the same node as the vCPU thread (assuming its
  mempolicy is MPOL_LOCAL), which is often the right node.

That being said, KVM currently has a gap where a guest doing a lot of
remote memory accesses when touching memory for the first time will
cause KVM to allocate the TDP page tables on the arguably wrong node.

> 
> We can improve TDP MMU eager page splitting by making
> tdp_mmu_alloc_sp_for_split() NUMA-aware. Specifically, when splitting a
> huge page, allocate the new lower level page tables on the same node as the
> huge page.
> 
> __get_free_page() is replaced by alloc_page_nodes(). This introduces two
> functional changes.
> 
>   1. __get_free_page() removes gfp flag __GFP_HIGHMEM via call to
>   __get_free_pages(). This should not be an issue  as __GFP_HIGHMEM flag is
>   not passed in tdp_mmu_alloc_sp_for_split() anyway.
> 
>   2. __get_free_page() calls alloc_pages() and use thread's mempolicy for
>   the NUMA node allocation. From this commit, thread's mempolicy will not
>   be used and first preference will be to allocate on the node where huge
>   page was present.

It would be worth noting that userspace could change the mempolicy of
the thread doing eager splitting to prefer allocating from the target
NUMA node, as an alternative approach.

I don't prefer the alternative though since it bleeds details from KVM
into userspace, such as the fact that enabling dirty logging does eager
page splitting, which allocates page tables. It's also unnecessary since
KVM can infer an appropriate NUMA placement without the help of
userspace, and I can't think of a reason for userspace to prefer a
different policy.

> 
> dirty_log_perf_test for 416 vcpu and 1GB/vcpu configuration on a 8 NUMA
> node machine showed dirty memory time improvements between 2% - 35% in
> multiple runs.

That's a pretty wide range.

What's probably happening is vCPU threads are being migrated across NUMA
nodes during the test. So for example, a vCPU thread might first run on
Node 0 during the Populate memory phase, so the huge page will be
allocated on Node 0 as well. But then if the thread gets migrated to
Node 1 later, it will perform poorly because it's memory is on a remote
node.

I would recommend pinning vCPUs to specific NUMA nodes to prevent
cross-node migrations. e.g. For a 416 vCPU VM, pin 52 to Node 0, 52 to
Node 1, etc. That should results in more consistent performance and will
be more inline with how large NUMA VMs are actually configured to run.

> 
> Suggested-by: David Matlack <dmatlack@google.com>
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index bf2ccf9debcaa..1e30e18fc6a03 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -1402,9 +1402,19 @@ 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)
> +/*
> + * Caller's responsibility to pass a valid spte which has the shadow page
> + * present.
> + */
> +static int tdp_mmu_spte_to_nid(u64 spte)

I think this name is ambiguous because it could be getting the node ID
of the SPTE itself, or the node ID of the page the SPTE is pointing to.

Maybe tdp_mmu_spte_pfn_nid()? Or just drop the helper an open code this
calculation in tdp_mmu_alloc_sp_for_split().

> +{
> +	return page_to_nid(pfn_to_page(spte_to_pfn(spte)));
> +}
> +
> +static struct kvm_mmu_page *__tdp_mmu_alloc_sp_for_split(int nid, gfp_t gfp)
>  {
>  	struct kvm_mmu_page *sp;
> +	struct page *spt_page;
>  
>  	gfp |= __GFP_ZERO;
>  
> @@ -1412,11 +1422,12 @@ static struct kvm_mmu_page *__tdp_mmu_alloc_sp_for_split(gfp_t gfp)
>  	if (!sp)
>  		return NULL;
>  
> -	sp->spt = (void *)__get_free_page(gfp);
> -	if (!sp->spt) {
> +	spt_page = alloc_pages_node(nid, gfp, 0);
> +	if (!spt_page) {
>  		kmem_cache_free(mmu_page_header_cache, sp);
>  		return NULL;
>  	}
> +	sp->spt = page_address(spt_page);
>  
>  	return sp;
>  }
> @@ -1426,6 +1437,9 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
>  						       bool shared)
>  {
>  	struct kvm_mmu_page *sp;
> +	int nid;
> +
> +	nid = tdp_mmu_spte_to_nid(iter->old_spte);
>  
>  	/*
>  	 * Since we are allocating while under the MMU lock we have to be
> @@ -1436,7 +1450,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(nid, GFP_NOWAIT | __GFP_ACCOUNT);
>  	if (sp)
>  		return sp;
>  
> @@ -1448,7 +1462,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
>  		write_unlock(&kvm->mmu_lock);
>  
>  	iter->yielded = true;
> -	sp = __tdp_mmu_alloc_sp_for_split(GFP_KERNEL_ACCOUNT);
> +	sp = __tdp_mmu_alloc_sp_for_split(nid, GFP_KERNEL_ACCOUNT);
>  
>  	if (shared)
>  		read_lock(&kvm->mmu_lock);
> -- 
> 2.37.1.455.g008518b4e5-goog
>