* [PATCH 0/2] KVM: Fix missing GFP_KERNEL_ACCOUNT usage
@ 2021-03-31 2:30 Sean Christopherson
2021-03-31 2:30 ` [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' Sean Christopherson
2021-03-31 2:30 ` [PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations Sean Christopherson
0 siblings, 2 replies; 7+ messages in thread
From: Sean Christopherson @ 2021-03-31 2:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Fix (almost) all cases in KVM x86 where allocations that are tied to a
task/VM are not correctly accounted.
There are a handful of allocations in SEV code that I intentionally didn't
fix in this series. I'm 95% certain those allocations can be eliminated
completely, changing them in this series only to delete them seemed
pointless.
The allocations in questions are for structs that are used to communicate
with the PSP; they are temporary (freed in the same function that does
the allocation) and small (some are _tiny_, e.g. 4 bytes). AFAICT, the
sole reason they are dynamically allocated is because the CCP driver uses
__pa() to retrieve the physical address that is passed to the PSP, and
__pa() does not work for vmalloc'd memory, which is in play when running
with CONFIG_VMAP_STACKS=y.
I have functional code that uses a scratch buffer as a bounce buffer to
cleanly handle vmalloc'd memory in the CCP driver. I'll hopefully get
that posted tomorrow.
Sean Christopherson (2):
KVM: Account memory allocations for 'struct kvm_vcpu'
KVM: x86: Account a variety of miscellaneous allocations
arch/x86/kvm/svm/nested.c | 4 ++--
arch/x86/kvm/svm/sev.c | 6 +++---
arch/x86/kvm/vmx/vmx.c | 2 +-
virt/kvm/kvm_main.c | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
--
2.31.0.291.g576ba9dcdaf-goog
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' 2021-03-31 2:30 [PATCH 0/2] KVM: Fix missing GFP_KERNEL_ACCOUNT usage Sean Christopherson @ 2021-03-31 2:30 ` Sean Christopherson 2021-03-31 3:02 ` Wanpeng Li 2021-03-31 2:30 ` [PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations Sean Christopherson 1 sibling, 1 reply; 7+ messages in thread From: Sean Christopherson @ 2021-03-31 2:30 UTC (permalink / raw) To: Paolo Bonzini Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel 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 <seanjc@google.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' 2021-03-31 2:30 ` [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' Sean Christopherson @ 2021-03-31 3:02 ` Wanpeng Li 2021-03-31 3:24 ` Sean Christopherson 0 siblings, 1 reply; 7+ messages in thread From: Wanpeng Li @ 2021-03-31 3:02 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, LKML On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <seanjc@google.com> 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 <seanjc@google.com> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' 2021-03-31 3:02 ` Wanpeng Li @ 2021-03-31 3:24 ` Sean Christopherson 2021-03-31 4:59 ` Wanpeng Li 0 siblings, 1 reply; 7+ messages in thread From: Sean Christopherson @ 2021-03-31 3:24 UTC (permalink / raw) To: Wanpeng Li Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, LKML On Wed, Mar 31, 2021, Wanpeng Li wrote: > On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <seanjc@google.com> 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 <seanjc@google.com> > > --- > > 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' 2021-03-31 3:24 ` Sean Christopherson @ 2021-03-31 4:59 ` Wanpeng Li 0 siblings, 0 replies; 7+ messages in thread From: Wanpeng Li @ 2021-03-31 4:59 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, LKML On Wed, 31 Mar 2021 at 11:24, Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Mar 31, 2021, Wanpeng Li wrote: > > On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <seanjc@google.com> 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 <seanjc@google.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations 2021-03-31 2:30 [PATCH 0/2] KVM: Fix missing GFP_KERNEL_ACCOUNT usage Sean Christopherson 2021-03-31 2:30 ` [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' Sean Christopherson @ 2021-03-31 2:30 ` Sean Christopherson 2021-03-31 9:39 ` Paolo Bonzini 1 sibling, 1 reply; 7+ messages in thread From: Sean Christopherson @ 2021-03-31 2:30 UTC (permalink / raw) To: Paolo Bonzini Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel Switch to GFP_KERNEL_ACCOUNT for a handful of allocations that are clearly associated with a single task/VM. Note, there are a several SEV allocations that aren't accounted, but those can (hopefully) be fixed by using the local stack for memory. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/nested.c | 4 ++-- arch/x86/kvm/svm/sev.c | 6 +++--- arch/x86/kvm/vmx/vmx.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 8523f60adb92..4f9e8b80ef99 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -1234,8 +1234,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, return -EINVAL; ret = -ENOMEM; - ctl = kzalloc(sizeof(*ctl), GFP_KERNEL); - save = kzalloc(sizeof(*save), GFP_KERNEL); + ctl = kzalloc(sizeof(*ctl), GFP_KERNEL_ACCOUNT); + save = kzalloc(sizeof(*save), GFP_KERNEL_ACCOUNT); if (!ctl || !save) goto out_free; diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 83e00e524513..883ce6bf23b9 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -637,7 +637,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) } ret = -ENOMEM; - blob = kmalloc(params.len, GFP_KERNEL); + blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT); if (!blob) goto e_free; @@ -1074,7 +1074,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp) } ret = -ENOMEM; - blob = kmalloc(params.len, GFP_KERNEL); + blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT); if (!blob) goto e_free; @@ -1775,7 +1775,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) len, GHCB_SCRATCH_AREA_LIMIT); return false; } - scratch_va = kzalloc(len, GFP_KERNEL); + scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT); if (!scratch_va) return false; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index c8a4a548e96b..5ab25376d718 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -562,7 +562,7 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu) * evmcs in singe VM shares same assist page. */ if (!*p_hv_pa_pg) - *p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL); + *p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT); if (!*p_hv_pa_pg) return -ENOMEM; -- 2.31.0.291.g576ba9dcdaf-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations 2021-03-31 2:30 ` [PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations Sean Christopherson @ 2021-03-31 9:39 ` Paolo Bonzini 0 siblings, 0 replies; 7+ messages in thread From: Paolo Bonzini @ 2021-03-31 9:39 UTC (permalink / raw) To: Sean Christopherson Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel On 31/03/21 04:30, Sean Christopherson wrote: > Switch to GFP_KERNEL_ACCOUNT for a handful of allocations that are > clearly associated with a single task/VM. > > Note, there are a several SEV allocations that aren't accounted, but > those can (hopefully) be fixed by using the local stack for memory. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/svm/nested.c | 4 ++-- > arch/x86/kvm/svm/sev.c | 6 +++--- > arch/x86/kvm/vmx/vmx.c | 2 +- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index 8523f60adb92..4f9e8b80ef99 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -1234,8 +1234,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, > return -EINVAL; > > ret = -ENOMEM; > - ctl = kzalloc(sizeof(*ctl), GFP_KERNEL); > - save = kzalloc(sizeof(*save), GFP_KERNEL); > + ctl = kzalloc(sizeof(*ctl), GFP_KERNEL_ACCOUNT); > + save = kzalloc(sizeof(*save), GFP_KERNEL_ACCOUNT); > if (!ctl || !save) > goto out_free; > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 83e00e524513..883ce6bf23b9 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -637,7 +637,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) > } > > ret = -ENOMEM; > - blob = kmalloc(params.len, GFP_KERNEL); > + blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT); > if (!blob) > goto e_free; > > @@ -1074,7 +1074,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp) > } > > ret = -ENOMEM; > - blob = kmalloc(params.len, GFP_KERNEL); > + blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT); > if (!blob) > goto e_free; > > @@ -1775,7 +1775,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > len, GHCB_SCRATCH_AREA_LIMIT); > return false; > } > - scratch_va = kzalloc(len, GFP_KERNEL); > + scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT); > if (!scratch_va) > return false; > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index c8a4a548e96b..5ab25376d718 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -562,7 +562,7 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu) > * evmcs in singe VM shares same assist page. > */ > if (!*p_hv_pa_pg) > - *p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL); > + *p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT); > > if (!*p_hv_pa_pg) > return -ENOMEM; > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-31 9:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-31 2:30 [PATCH 0/2] KVM: Fix missing GFP_KERNEL_ACCOUNT usage Sean Christopherson 2021-03-31 2:30 ` [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu' Sean Christopherson 2021-03-31 3:02 ` Wanpeng Li 2021-03-31 3:24 ` Sean Christopherson 2021-03-31 4:59 ` Wanpeng Li 2021-03-31 2:30 ` [PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations Sean Christopherson 2021-03-31 9:39 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).