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:   Tue, 2 Aug 2022 09:31:35 -0700
From:   David Matlack <dmatlack@google.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Vipin Sharma <vipinsh@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: <YulRZ+uXFOE1y2dj@google.com>
References: <20220801151928.270380-1-vipinsh@google.com>
 <YuhPT2drgqL+osLl@google.com>
 <YuhoJUoPBOu5eMz8@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YuhoJUoPBOu5eMz8@google.com>
Precedence: bulk

On Mon, Aug 01, 2022 at 11:56:21PM +0000, Sean Christopherson wrote:
> On Mon, Aug 01, 2022, David Matlack wrote:
> > On Mon, Aug 01, 2022 at 08:19:28AM -0700, Vipin Sharma wrote:
> > 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.
> 
> Userspace can solve this by setting the NUMA policy on a VMA or shared-object
> basis.  E.g. create dedicated memslots for each NUMA node, then bind each of the
> backing stores to the appropriate host node.
> 
> If there is a gap, e.g. a backing store we want to use doesn't properly support
> mempolicy for shared mappings, then we should enhance the backing store.

Just to clarify: this patch, and my comment here, is about the NUMA
locality of KVM's page tables, not guest memory.

KVM allocates all page tables through __get_free_page() which uses the
current task's mempolicy. This means the only way for userspace to
control the page table NUMA locality is to set mempolicies on the
threads on which KVM will allocate page tables (vCPUs and any thread
doing eager page splitting, i.e. enable dirty logging or
KVM_CLEAR_DIRTY_LOG).

The ideal setup from a NUMA locality perspective would be that page
tables are always on the local node, but that would require KVM maintain
multiple copies of page tables (at minimum one per physical NUMA node),
which is extra memory and complexity.

With the current KVM MMU (one page table hierarchy shared by all vCPUs),
the next best setup, I'd argue, is to co-locate the page tables with the
memory they are mapping. This setup would ensure that a vCPU accessing
memory in its local virtual node would primarily be accessing page
tables in the local physical node when doing page walks. Obviously page
tables at levels 5, 4, and 3, which likely map memory spanning multiple
nodes, could not always be co-located with all the memory they map.

My comment here is saying that there is no way for userspace to actually
enforce the above policy. There is no mempolicy userspace can set on
vCPU threads to ensure that KVM co-locates page tables with the memory
they are mapping. All userspace can do is force KVM to allocate page
tables on the same node as the vCPU thread, or on a specific node.

For eager page splitting, userspace can control what range of memory is
going to be split by controlling the memslot layout, so it is possible
for userspace to set an appropriate mempolicy on that thread. But if you
agree about my point about vCPU threads above, I think it would be a
step in the right direction to have KVM start forcibly co-locating page
tables with memory for eager page splitting (and we can fix the vCPU
case later).

> 
> > > 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. 
> 
> As above, if userspace is cares about vNUMA, then it already needs to be aware of
> some of KVM/kernel details.  Separate memslots aren't strictly necessary, e.g.
> userspace could stitch together contiguous VMAs to create a single mega-memslot,
> but that seems like it'd be more work than just creating separate memslots.
> 
> And because eager page splitting for dirty logging runs with mmu_lock held for,
> userspace might also benefit from per-node memslots as it can do the splitting on
> multiple tasks/CPUs.
> 
> Regardless of what we do, the behavior needs to be document, i.e. KVM details will
> bleed into userspace.  E.g. if KVM is overriding the per-task NUMA policy, then
> that should be documented.

+1

> 
> > 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.
> 
> I can't think of a reason why userspace would want to have a different policy for
> the task that's enabling dirty logging, but I also can't think of a reason why
> KVM should go out of its way to ignore that policy.
> 
> IMO this is a "bug" in dirty_log_perf_test, though it's probably a good idea to
> document how to effectively configure vNUMA-aware memslots.