On Sat, Mar 5, 2022 at 8:55 AM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > On Fri, 04 Mar 2022 21:59:12 +0000, > David Matlack <dmatlack@xxxxxxxxxx> wrote: > > > > On Thu, Feb 24, 2022 at 11:20 AM David Matlack <dmatlack@xxxxxxxxxx> wrote: > > > > > > On Thu, Feb 24, 2022 at 3:29 AM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > > > > > > > On Thu, 03 Feb 2022 01:00:47 +0000, > > > > David Matlack <dmatlack@xxxxxxxxxx> 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. That is a good point. And that risk is very real given that kvm_mmu_topup_memory_cache() assumes the capacity is KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE if the capacity field is 0. One way to mitigate this would be to get rid of the capacity hack in kvm_mmu_topup_memory_cache() and require the capacity field be explicitly initialized. That will make it harder to trip over this and/or easier to debug because kvm_mmu_topup_memory_cache() can issue a WARN() if the capacity is 0. Once you see that warning and go to initialize the capacity field you'll realize why it needs to be set in the first place. The diff will just be slightly larger to set capacity for each cache. > > [...] > > > 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. Ok I'll play with #2. Thanks for the feedback. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
