Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
tied to a single task/VM. For x86, the allocations were accounted up
until the allocation code was moved to common KVM. For all other
architectures, vCPU allocations were never previously accounted, but only
because most architectures lack accounting in general (for KVM).
Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
Signed-off-by: Sean Christopherson <[email protected]>
---
virt/kvm/kvm_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 383df23514b9..3884e9f30251 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
if (r)
goto vcpu_decrement;
- vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
+ vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
if (!vcpu) {
r = -ENOMEM;
goto vcpu_decrement;
--
2.31.0.291.g576ba9dcdaf-goog
On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <[email protected]> wrote:
>
> Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
> tied to a single task/VM. For x86, the allocations were accounted up
> until the allocation code was moved to common KVM. For all other
> architectures, vCPU allocations were never previously accounted, but only
> because most architectures lack accounting in general (for KVM).
>
> Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> virt/kvm/kvm_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 383df23514b9..3884e9f30251 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> if (r)
> goto vcpu_decrement;
>
> - vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> + vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
kvm_vcpu_cache is created with SLAB_ACCOUNT flag in kvm_init(), this
flag will guarantee further slab alloc will be charged to memcg.
Please refer to memcg_slab_pre_alloc_hook(). So the patch is
unnecessary.
Wanpeng
On Wed, Mar 31, 2021, Wanpeng Li wrote:
> On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <[email protected]> wrote:
> >
> > Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
> > tied to a single task/VM. For x86, the allocations were accounted up
> > until the allocation code was moved to common KVM. For all other
> > architectures, vCPU allocations were never previously accounted, but only
> > because most architectures lack accounting in general (for KVM).
> >
> > Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > virt/kvm/kvm_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 383df23514b9..3884e9f30251 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> > if (r)
> > goto vcpu_decrement;
> >
> > - vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> > + vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
>
> kvm_vcpu_cache is created with SLAB_ACCOUNT flag in kvm_init(), this
> flag will guarantee further slab alloc will be charged to memcg.
> Please refer to memcg_slab_pre_alloc_hook(). So the patch is
> unnecessary.
Hmm, I missed that. However, AFICT only SLAB/SLUB enforce SLAB_ACCOUNT, SLOB
does not appear to honor the flag. The caveat to SLOB is that the
GFP_KERNEL_ACCOUNT will only come into play when allocating new pages, and so
allocations smaller than a page will be accounted incorrectly (I think).
But, a vcpu is larger than a page (on x86), which means the vcpu allocation will
always be correctly accounted.
I've no idea if anyone actually uses KVM+SLOB, let alone cares about accounting
in the that case. But, it would be nice for KVM to be consistent with the other
kmem_cache usage in KVM, all of which do double up on SLAB_ACCOUNT +
GFP_KERNEL_ACCOUNT.
Maybe rewrite the changelog and drop the Fixes?
On Wed, 31 Mar 2021 at 11:24, Sean Christopherson <[email protected]> wrote:
>
> On Wed, Mar 31, 2021, Wanpeng Li wrote:
> > On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <[email protected]> wrote:
> > >
> > > Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
> > > tied to a single task/VM. For x86, the allocations were accounted up
> > > until the allocation code was moved to common KVM. For all other
> > > architectures, vCPU allocations were never previously accounted, but only
> > > because most architectures lack accounting in general (for KVM).
> > >
> > > Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
> > > Signed-off-by: Sean Christopherson <[email protected]>
> > > ---
> > > virt/kvm/kvm_main.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 383df23514b9..3884e9f30251 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> > > if (r)
> > > goto vcpu_decrement;
> > >
> > > - vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> > > + vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
> >
> > kvm_vcpu_cache is created with SLAB_ACCOUNT flag in kvm_init(), this
> > flag will guarantee further slab alloc will be charged to memcg.
> > Please refer to memcg_slab_pre_alloc_hook(). So the patch is
> > unnecessary.
>
> Hmm, I missed that. However, AFICT only SLAB/SLUB enforce SLAB_ACCOUNT, SLOB
> does not appear to honor the flag. The caveat to SLOB is that the
> GFP_KERNEL_ACCOUNT will only come into play when allocating new pages, and so
> allocations smaller than a page will be accounted incorrectly (I think).
> But, a vcpu is larger than a page (on x86), which means the vcpu allocation will
> always be correctly accounted.
>
> I've no idea if anyone actually uses KVM+SLOB, let alone cares about accounting
I asked maintainer Christoph in 2013, he told me "Well, I have never
seen non experimental systems that use SLOB. Others have claimed they
exist. It's mostly of academic interest."
> in the that case. But, it would be nice for KVM to be consistent with the other
> kmem_cache usage in KVM, all of which do double up on SLAB_ACCOUNT +
> GFP_KERNEL_ACCOUNT.
>
> Maybe rewrite the changelog and drop the Fixes?
Agreed.
Wanpeng