| Message ID | c0122f66-f428-417e-a360-b25fc0f154a0@p183 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: do not account temporary allocations to kmem | expand |
On Mon, Jun 10, 2024, Alexey Dobriyan wrote: > Some allocations done by KVM are temporary, they are created as result > of program actions, but can't exists for arbitrary long times. > > They should have been GFP_TEMPORARY (rip!). Wouldn't GFP_USER be more appropriate for all of these? E.g. KVM_SET_REGS uses memdup_user() and thus GFP_USER. > OTOH, kvm-nx-lpage-recovery and kvm-pit kernel threads exist for as long > as VM exists but their task_struct memory is not accounted. > This is story for another day. > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > --- > > virt/kvm/kvm_main.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4427,7 +4427,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > struct kvm_regs *kvm_regs; > > r = -ENOMEM; > - kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL_ACCOUNT); > + kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL); > if (!kvm_regs) > goto out; > r = kvm_arch_vcpu_ioctl_get_regs(vcpu, kvm_regs); > @@ -4454,8 +4454,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > break; > } > case KVM_GET_SREGS: { > - kvm_sregs = kzalloc(sizeof(struct kvm_sregs), > - GFP_KERNEL_ACCOUNT); > + kvm_sregs = kzalloc(sizeof(struct kvm_sregs), GFP_KERNEL); > r = -ENOMEM; > if (!kvm_sregs) > goto out; > @@ -4547,7 +4546,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > break; > } > case KVM_GET_FPU: { > - fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL_ACCOUNT); > + fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL); > r = -ENOMEM; > if (!fpu) > goto out; > @@ -6210,7 +6209,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) > active = kvm_active_vms; > mutex_unlock(&kvm_lock); > > - env = kzalloc(sizeof(*env), GFP_KERNEL_ACCOUNT); > + env = kzalloc(sizeof(*env), GFP_KERNEL); > if (!env) > return; > > @@ -6226,7 +6225,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) > add_uevent_var(env, "PID=%d", kvm->userspace_pid); > > if (!IS_ERR(kvm->debugfs_dentry)) { > - char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT); > + char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL); > > if (p) { > tmp = dentry_path_raw(kvm->debugfs_dentry, p, PATH_MAX);
On Mon, Jun 10, 2024 at 11:53 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Jun 10, 2024, Alexey Dobriyan wrote: > > Some allocations done by KVM are temporary, they are created as result > > of program actions, but can't exists for arbitrary long times. > > > > They should have been GFP_TEMPORARY (rip!). > > Wouldn't GFP_USER be more appropriate for all of these? E.g. KVM_SET_REGS uses > memdup_user() and thus GFP_USER. The only difference between GFP_KERNEL and GFP_USER (worst name ever...) is that the latter strictly respects the cpuset policy, see cpuset_node_allowed(). It's not needed for allocations such as these ones, which are bounded in both size and lifetime. Paolo > > > OTOH, kvm-nx-lpage-recovery and kvm-pit kernel threads exist for as long > > as VM exists but their task_struct memory is not accounted. > > This is story for another day. > > > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > > --- > > > > virt/kvm/kvm_main.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -4427,7 +4427,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > > struct kvm_regs *kvm_regs; > > > > r = -ENOMEM; > > - kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL_ACCOUNT); > > + kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL); > > if (!kvm_regs) > > goto out; > > r = kvm_arch_vcpu_ioctl_get_regs(vcpu, kvm_regs); > > @@ -4454,8 +4454,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > > break; > > } > > case KVM_GET_SREGS: { > > - kvm_sregs = kzalloc(sizeof(struct kvm_sregs), > > - GFP_KERNEL_ACCOUNT); > > + kvm_sregs = kzalloc(sizeof(struct kvm_sregs), GFP_KERNEL); > > r = -ENOMEM; > > if (!kvm_sregs) > > goto out; > > @@ -4547,7 +4546,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > > break; > > } > > case KVM_GET_FPU: { > > - fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL_ACCOUNT); > > + fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL); > > r = -ENOMEM; > > if (!fpu) > > goto out; > > @@ -6210,7 +6209,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) > > active = kvm_active_vms; > > mutex_unlock(&kvm_lock); > > > > - env = kzalloc(sizeof(*env), GFP_KERNEL_ACCOUNT); > > + env = kzalloc(sizeof(*env), GFP_KERNEL); > > if (!env) > > return; > > > > @@ -6226,7 +6225,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) > > add_uevent_var(env, "PID=%d", kvm->userspace_pid); > > > > if (!IS_ERR(kvm->debugfs_dentry)) { > > - char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT); > > + char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL); > > > > if (p) { > > tmp = dentry_path_raw(kvm->debugfs_dentry, p, PATH_MAX); >
--- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4427,7 +4427,7 @@ static long kvm_vcpu_ioctl(struct file *filp, struct kvm_regs *kvm_regs; r = -ENOMEM; - kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL_ACCOUNT); + kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL); if (!kvm_regs) goto out; r = kvm_arch_vcpu_ioctl_get_regs(vcpu, kvm_regs); @@ -4454,8 +4454,7 @@ static long kvm_vcpu_ioctl(struct file *filp, break; } case KVM_GET_SREGS: { - kvm_sregs = kzalloc(sizeof(struct kvm_sregs), - GFP_KERNEL_ACCOUNT); + kvm_sregs = kzalloc(sizeof(struct kvm_sregs), GFP_KERNEL); r = -ENOMEM; if (!kvm_sregs) goto out; @@ -4547,7 +4546,7 @@ static long kvm_vcpu_ioctl(struct file *filp, break; } case KVM_GET_FPU: { - fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL_ACCOUNT); + fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL); r = -ENOMEM; if (!fpu) goto out; @@ -6210,7 +6209,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) active = kvm_active_vms; mutex_unlock(&kvm_lock); - env = kzalloc(sizeof(*env), GFP_KERNEL_ACCOUNT); + env = kzalloc(sizeof(*env), GFP_KERNEL); if (!env) return; @@ -6226,7 +6225,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) add_uevent_var(env, "PID=%d", kvm->userspace_pid); if (!IS_ERR(kvm->debugfs_dentry)) { - char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT); + char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL); if (p) { tmp = dentry_path_raw(kvm->debugfs_dentry, p, PATH_MAX);
Some allocations done by KVM are temporary, they are created as result of program actions, but can't exists for arbitrary long times. They should have been GFP_TEMPORARY (rip!). OTOH, kvm-nx-lpage-recovery and kvm-pit kernel threads exist for as long as VM exists but their task_struct memory is not accounted. This is story for another day. Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> --- virt/kvm/kvm_main.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)