Re: [PATCH 19/23] KVM: Allow for different capacities in kvm_mmu_memory_cache structs

[Marc Zyngier <maz@kernel.org>]

On Fri, 04 Mar 2022 21:59:12 +0000,
David Matlack <dmatlack@google.com> wrote:
> 
> On Thu, Feb 24, 2022 at 11:20 AM David Matlack <dmatlack@google.com> wrote:
> >
> > On Thu, Feb 24, 2022 at 3:29 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 03 Feb 2022 01:00:47 +0000,
> > > David Matlack <dmatlack@google.com> wrote:
> > > >
> 
> [...]
> 
> > > >
> > > >       /* Cache some mmu pages needed inside spinlock regions */
> > > > -     struct kvm_mmu_memory_cache mmu_page_cache;
> > > > +     DEFINE_KVM_MMU_MEMORY_CACHE(mmu_page_cache);
> > >
> > > I must say I'm really not a fan of the anonymous structure trick. I
> > > can see why you are doing it that way, but it feels pretty brittle.
> >
> > Yeah I don't love it. It's really optimizing for minimizing the patch diff.
> >
> > The alternative I considered was to dynamically allocate the
> > kvm_mmu_memory_cache structs. This would get rid of the anonymous
> > struct and the objects array, and also eliminate the rather gross
> > capacity hack in kvm_mmu_topup_memory_cache().
> >
> > The downsides of this approach is more code and more failure paths if
> > the allocation fails.
> 
> I tried changing all kvm_mmu_memory_cache structs to be dynamically
> allocated, but it created a lot of complexity to the setup/teardown
> code paths in x86, arm64, mips, and riscv (the arches that use the
> caches). I don't think this route is worth it, especially since these
> structs don't *need* to be dynamically allocated.
> 
> When you said the anonymous struct feels brittle, what did you have in
> mind specifically?

I can perfectly see someone using a kvm_mmu_memory_cache and searching
for a bit why they end-up with memory corruption. Yes, this would be a
rookie mistake, but this are some expectations all over the kernel
that DEFINE_* and the corresponding structure are the same object.

[...]

> I see two alternatives to make this cleaner:
> 
> 1. Dynamically allocate just this cache. The caches defined in
> vcpu_arch will continue to use DEFINE_KVM_MMU_MEMORY_CACHE(). This
> would get rid of the outer struct but require an extra memory
> allocation.
> 2. Move this cache to struct kvm_arch using
> DEFINE_KVM_MMU_MEMORY_CACHE(). Then we don't need to stack allocate it
> or dynamically allocate it.
> 
> Do either of these approaches appeal to you more than the current one?

Certainly, #2 feels more solid. Dynamic allocations (and the resulting
pointer chasing) are usually costly in terms of performance, so I'd
avoid it if at all possible.

That being said, if it turns out that #2 isn't practical, I won't get
in the way of your current approach. Moving kvm_mmu_memory_cache to
core code was definitely a good cleanup, and I'm not overly excited
with the perspective of *more* arch-specific code.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.