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 17:22:17 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     David Matlack <dmatlack@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: <YuldSf4T2j4rIrIo@google.com>
References: <20220801151928.270380-1-vipinsh@google.com>
 <YuhPT2drgqL+osLl@google.com>
 <YuhoJUoPBOu5eMz8@google.com>
 <YulRZ+uXFOE1y2dj@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YulRZ+uXFOE1y2dj@google.com>
Precedence: bulk

On Tue, Aug 02, 2022, David Matlack wrote:
> 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.

Oooh, I overlooked the "TDP page tables" part in the above paragraph.

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

Hmm, it actually wouldn't be much complexity, e.g. a few bits in the role, but
the memory would indeed be a problem.

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

I agree that there's a gap with respect to a vCPU being the first to touch a
remote node, but I disagree that forcibly co-locating page tables for eager page
splitting is necessary.

Userspace can already force the ideal setup for eager page splitting by configuring
vNUMA-aware memslots and using a task with appropriate policy to toggle dirty
logging.  And userspace really should be encouraged to do that, because otherwise
walking the page tables in software to do the split is going to be constantly
accessing remote memory.

And if anyone ever wants to fix the aforementioned gap, the two fixes that come to
mind would be to tie the policy to the memslot, or to do page_to_nid() on the
underlying page (with the assumption that memory that's not backed by struct page
isn't NUMA-aware, so fall back to task policy).  At that point, having one-off code
to pull the node from the existing page tables is also unnecessary.