Re: [PATCH v10 12/41] KVM: guest_memfd: Call arch make_shared callback for to-shared conversion
Ackerley Tng <[email protected]>
| Newsgroups | gmane.linux.kernel,gmane.comp.emulators.kvm.devel,gmane.linux.documentation,gmane.linux.kernel.mm |
|---|---|
| Message-ID | <CAEvNRgEojt8Kpot9VaFUyVbVH5QVEdW-TgQUbVnpVCNzFtdVag@mail.gmail.com> |
Sean Christopherson <[email protected]> writes: > On Sun, Aug 16, 2026, Ackerley Tng wrote: >> Sean Christopherson <[email protected]> writes: >> >> > On Thu, Aug 13, 2026, Ackerley Tng wrote: >> >> Sean Christopherson <[email protected]> writes: >> >> > That's why I think it's worth analyzing the cost: if it's in the >> >> > noise, leave it alone. If it's meaningful, figure out a not-too-gross way to skip >> >> > the entire thing if kvm_arch_gmem_make_shared() is a glorified nop in the end. >> >> >> >> Is noise defined relative to the entire conversion process? Would this >> >> benchmark look like >> >> >> >> 1. Convert 4G to shared on TDX with CONFIG_AMD_SEV defined >> >> 2. Convert 4G to shared on TDX without CONFIG_AMD_SEV defined >> >> >> >> and then compare the difference in time taken? >> > >> > That'd work, though I was envisioning something even simpler: use rdtsc() to >> > count the cycles it takes to iterate over various ranges of memory. Do whatever >> > is easiest for you though. >> >> I made some changes to add rdtsc() for the conversion process as Sean >> suggested [1], and exercised conversion like this [2]: >> >> 1. Initialize some memory as private >> 2. Get the guest to fault them into Secure EPTs >> 3. Converts the memory to shared <<== this is being benchmarked >> 4. Converts memory back to private >> >> I made it build the VM once and convert 5 times: >> >> ./gmem_benchmark_tdx_convert --iterations=5 --size=1g > > ... > >> And here's the above, tabulated: >> >> nr_pages make_shared total percentage >> ---------- --------------- --------------- ------------ >> 1 930 39278 2.3677% >> 1 252 28060 0.8981% >> 1 176 26952 0.6530% >> 1 176 27038 0.6509% >> 1 176 26980 0.6523% >> 1 1072 37236 2.8789% >> 1 316 28338 1.1151% >> 1 176 27182 0.6475% >> 1 176 26972 0.6525% >> 1 176 26886 0.6546% >> 262144 15041018 6616067680 0.2273% >> 262144 14937462 6608542680 0.2260% >> 262144 15138858 6599494898 0.2294% >> 262144 15721972 6610219850 0.2378% >> 262144 15000406 6615114540 0.2268% >> 1048576 61902982 26400884028 0.2345% >> 1048576 61746114 26401170984 0.2339% >> 1048576 61096794 26404409058 0.2314% >> 1048576 61446290 26447461896 0.2323% >> 1048576 61774646 26444608360 0.2336% >> >> Looks to me it is within noise. >> >> I also actually tried measuring the conversion time from userspace with >> CONFIG_AMD_SEV enabled and disabled. Converting a 1G-sized TD was faster >> by 0.2%, which is in line with the above table. Interestingly, when >> converting a 4G-sized TD, skipping kvm_gmem_make_shared() was _slower_ >> over 2 runs. I don't have an explanation for that. > > Might be some cache/memory locality benefits? Though with a conversion that big, > it could also be nothing more than bad luck. > >> I think the code was correct. (If it makes a difference, I skipped >> kvm_gmem_make_shared() using a custom guest_memfd creation time flag and >> skipped make_shared if the flag was set on the inode.) >> >> I thought adding a kvm_arch_has_gmem_make_shared(), defaulting it to > > I would do kvm_arch_has_gmem_convert() for consistency with the Kconfigs, and > because the cost of the reclaim invocation is a non-issue. > >> false for all archs and having x86 override with >> !!kvm_x86_ops.gmem_make_shared is not too bad either: >> + doesn't leak anything, since the function being called is >> kvm_arch_gmem_make_shared and the accompanying function is >> kvm_arch_has_gmem_make_shared. Or maybe just a little, since all the >> other ops don't have the accompanying _has_ function >> + it's a kernel-internal thing >> + not too many lines of code, not too complex > > It also provides a good excuse to kill off the #idfefs in guest_memfd.c. > Compile tested only, but I'm thinking this? > > From: Sean Christopherson <[email protected]> > Date: Mon, 17 Aug 2026 12:31:50 -0700 > Subject: [PATCH] KVM: guest_memfd: Optimize away conversion overheads via > dead-code elimination > > Add and use kvm_arch_has_gmem_convert() to guard guest_memfd's invocation > of arch hooks related to converting memory between private and shared, as > only one half of the x86 CoCo duo needs the runtime hooks (any pre-work is > pure overhead for TDX). At this exact moment, the overhead is negligible, > but that will change when in-place conversion comes along, at which point > to-shared conversions will "need" to find all affected folios prior to > calling into arch code. In quotes because very technically that work could > be pushed to arch code, but that would bleed guest_memfd details into arch > code and would be far worse than adding yet another kvm_arch_has... hook. > > Opportunistically provide the kvm_arch_gmem_make_private() declaration, and > rely on dead-code elimination to eliminate the call to non-existent code > when CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT=n. > > Reported-by: Binbin Wu <[email protected]> > Closes: https://lore.kernel.org/all/[email protected] > Suggested-by: Ackerley Tng <[email protected]> > Signed-off-by: Sean Christopherson <[email protected]> > --- > arch/x86/include/asm/kvm_host.h | 3 +++ > include/linux/kvm_host.h | 3 ++- > virt/kvm/guest_memfd.c | 5 ++--- > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 283847619ff8..5d5a7723abb6 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1854,6 +1854,9 @@ enum kvm_intr_type { > #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES > #define kvm_arch_has_private_mem(kvm) ((kvm)->arch.has_private_mem) > #endif > +#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT > +#define kvm_arch_has_gmem_convert() (!!kvm_x86_ops.gmem_make_private) > +#endif > > #define kvm_arch_has_readonly_mem(kvm) (!(kvm)->arch.has_protected_state) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 03bfc92864b6..e824ba59c60c 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -2599,9 +2599,10 @@ static inline int kvm_gmem_get_pfn(struct kvm *kvm, > } > #endif /* CONFIG_KVM_GUEST_MEMFD */ > > -#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT > int kvm_arch_gmem_make_private(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, > kvm_pfn_t nr_pages); > +#ifndef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT > +#define kvm_arch_has_gmem_convert() false > #endif > > #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_POPULATE > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c > index b596486d184c..39d94938b5f6 100644 > --- a/virt/kvm/guest_memfd.c > +++ b/virt/kvm/guest_memfd.c > @@ -773,11 +773,10 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, > folio_mark_uptodate(folio); > } > > -#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT > - if (kvm_gmem_is_private_mem(file_inode(file), index)) > + if (kvm_arch_has_gmem_convert() && > + kvm_gmem_is_private_mem(file_inode(file), index)) > r = kvm_arch_gmem_make_private(kvm, gfn, *pfn, > (kvm_pfn_t)1 << *max_order); > -#endif > > folio_unlock(folio); > > > base-commit: 1b731e5ded480bd1e5546aed35584238661ce72e > -- I'll inserted this as the first patch for v11. Then, the later patch "Call arch make_shared callback for to-shared conversion" is now: if (!to_private && kvm_arch_has_gmem_convert()) kvm_gmem_make_shared(inode, start, end); kvm_gmem_make_shared()'s definition is still guarded by #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT, and there's still a stub for kvm_gmem_make_shared() since kvm_gmem_make_shared() is defined and used in the same file and the optimizing out only happens later.