First off---thanks for reviewing my patch!
With respect to the size of the struct, here's what I'm seeing on an
upstream
version of the kernel:
(gdb) p sizeof(struct kvm)
$1 = 42200
(gdb) p sizeof(struct kvm_arch)
$2 = 35496
I'm pretty new to KVM--so it's not obvious to me:
- Why the size of this structure differs on my machine vs. yours
- What fields in 'struct kvm_arch' vary by architecture
When I first read your email, my reaction was:
Let's vzalloc 'struct kvm' and kzalloc 'struct kvm_arch', but after looking
at
the size of 'struct kvm_arch' (see above), that obviously won't help :-).
I'll add that my initial reaction when I was posed with this problem was
that
40kB is NOTHING. But more experienced colleagues quickly explained to me
that
40 kB is actually a lot for Linux to guarantee contiguously via kalloc. I'm
not
sure if the <13 kB that you're seeing in your environment is much better,
but
regardless, it seems like we're better off fixing this now rather than
dealing
with it later when the structure inevitably bloats, both in the upstream
kernel
and within people's domain-specific use cases.
Thus, I wonder if folks have a suggestion on how to refactor these structs
so
that we can use valloc for most of the fields and partition
architecture-specific fields into a sub struct so that they can continue
being
allocated via kalloc.
Thanks,
Marc
On Fri, Apr 20, 2018 at 1:34 AM Christian Borntraeger <
borntraeger@xxxxxxxxxx> wrote:
> On 04/19/2018 08:29 PM, Marc Orr wrote:
> > The kvm struct is (currently) tens of kilo-bytes, which turns out to be
a
> > large amount of memory to allocate contiguously via kzalloc. Thus, this
> > patch changes the kzalloc to a vzalloc, so that the memory allocation is
> > less likely to fail in resource-constrained environments.
> >
> > Signed-off-by: Marc Orr <marcorr@xxxxxxxxxx>
> > ---
> > include/linux/kvm_host.h | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 6930c63126c7..cf7b6a3a4197 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -19,6 +19,7 @@
> > #include <linux/preempt.h>
> > #include <linux/msi.h>
> > #include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > #include <linux/rcupdate.h>
> > #include <linux/ratelimit.h>
> > #include <linux/err.h>
> > @@ -810,12 +811,12 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu
*vcpu);
> > #ifndef __KVM_HAVE_ARCH_VM_ALLOC
> > static inline struct kvm *kvm_arch_alloc_vm(void)
> > {
> > - return kzalloc(sizeof(struct kvm), GFP_KERNEL);
> > + return vzalloc(sizeof(struct kvm));
> > }
> >
> > static inline void kvm_arch_free_vm(struct kvm *kvm)
> > {
> > - kfree(kvm);
> > + vfree(kvm);
> > }
> > #endif
> >
> I dont think this is necessarily safe. This contains kvm_arch and you
> would need to verify that no architecture has data in struct kvm_arch
> that must be allocated with kmalloc. Fo example on s390 kernel virtual
> == kernel real for kmalloc so some driver might rely on that.
> FWIW, it looks like the s390 struct kvm_arch is fine, but this requires
> more review I guess.
> Another thing:
> In s390 this is 14760 bytes and on x86 it seems to be 12440 on my system.
> This should never fail, no?
