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?
>
> >
> > >
> > > /* Target CPU and feature flags */
> > > int target;
> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > index bc2aba953299..9c853c529b49 100644
> > > --- a/arch/arm64/kvm/mmu.c
> > > +++ b/arch/arm64/kvm/mmu.c
> > > @@ -765,7 +765,8 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > > {
> > > phys_addr_t addr;
> > > int ret = 0;
> > > - struct kvm_mmu_memory_cache cache = { 0, __GFP_ZERO, NULL, };
> > > + DEFINE_KVM_MMU_MEMORY_CACHE(cache) page_cache = {};
> > > + struct kvm_mmu_memory_cache *cache = &page_cache.cache;
> > > struct kvm_pgtable *pgt = kvm->arch.mmu.pgt;
> > > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_DEVICE |
> > > KVM_PGTABLE_PROT_R |
> > > @@ -774,18 +775,17 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > > if (is_protected_kvm_enabled())
> > > return -EPERM;
> > >
> > > + cache->gfp_zero = __GFP_ZERO;
> >
> > nit: consider this instead, which preserves the existing flow:
>
> Will do.
>
> >
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 26d6c53be083..86a7ebd03a44 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -764,7 +764,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > {
> > phys_addr_t addr;
> > int ret = 0;
> > - DEFINE_KVM_MMU_MEMORY_CACHE(cache) page_cache = {};
> > + DEFINE_KVM_MMU_MEMORY_CACHE(cache) page_cache = {
> > + .cache = { .gfp_zero = __GFP_ZERO},
> > + };
> > struct kvm_mmu_memory_cache *cache = &page_cache.cache;
> > struct kvm_pgtable *pgt = kvm->arch.mmu.pgt;
> > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_DEVICE |
> > @@ -774,7 +776,6 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > if (is_protected_kvm_enabled())
> > return -EPERM;
> >
> > - cache->gfp_zero = __GFP_ZERO;
> > size += offset_in_page(guest_ipa);
> > guest_ipa &= PAGE_MASK;
> >
> > but whole "declare the outer structure and just use the inner one"
> > hack is... huh... :-/
>
> Yeah it's not great. Unfortunately (or maybe fortunately?) anonymous
> structs cannot be defined in functions. So naming the outer struct is
> necessary even though we only need to use the inner one.
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?
>
> >
> > This hunk also conflicts with what currently sits in -next. Not a big
> > deal, but just so you know.
>
> Ack.
>
> >
> > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > index dceac12c1ce5..9575fb8d333f 100644
> > > --- a/include/linux/kvm_types.h
> > > +++ b/include/linux/kvm_types.h
> > > @@ -78,14 +78,34 @@ struct gfn_to_pfn_cache {
> > > * MMU flows is problematic, as is triggering reclaim, I/O, etc... while
> > > * holding MMU locks. Note, these caches act more like prefetch buffers than
> > > * classical caches, i.e. objects are not returned to the cache on being freed.
> > > + *
> > > + * The storage for the cache objects is laid out after the struct to allow
> > > + * different declarations to choose different capacities. If the capacity field
> > > + * is 0, the capacity is assumed to be KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE.
> > > */
> > > struct kvm_mmu_memory_cache {
> > > int nobjs;
> > > + int capacity;
> > > gfp_t gfp_zero;
> > > struct kmem_cache *kmem_cache;
> > > - void *objects[KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE];
> > > + void *objects[0];
> >
> > The VLA police is going to track you down ([0] vs []).
>
> Thanks!
>
>
> >
> > M.
> >
> > --
> > Without deviation from the norm, progress is not possible.
