On Mon, Mar 7, 2022 at 3:49 PM David Matlack <dmatlack@xxxxxxxxxx> wrote: > > 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: > > > 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. #2 is very clean to implement but it ends up being a bit silly. It increases the size of struct kvm_arch by 336 bytes for all VMs but only ever gets used during kvm_vgic_map_resources(), which is only called the first time a vCPU is run (according to the comment in kvm_arch_vcpu_run_pid_change()). I think stack allocation makes the most sense for this object, I don't think it's worth dancing around that solely to avoid the inner struct grottiness.
