On 4/11/19 12:27 PM, Paul Mackerras wrote:
> On Wed, Apr 10, 2019 at 07:04:48PM +0200, Cédric Le Goater wrote:
>> When a P9 sPAPR VM boots, the CAS negotiation process determines which
>> interrupt mode to use (XICS legacy or XIVE native) and invokes a
>> machine reset to activate the chosen mode.
>>
>> To be able to switch from one mode to another, we introduce the
>> capability to release a KVM device without destroying the VM. The KVM
>> device interface is extended with a new 'release' operation which is
>> called when the file descriptor of the device is closed.
>
> Unfortunately, I think there is now a memory leak:
>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index ea2018ae1cd7..ea2619d5ca98 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2938,6 +2938,19 @@ static int kvm_device_release(struct inode *inode, struct file *filp)
>> struct kvm_device *dev = filp->private_data;
>> struct kvm *kvm = dev->kvm;
>>
>> + if (!dev)
>> + return -ENODEV;
>> +
>> + if (dev->kvm != kvm)
>> + return -EPERM;
>> +
>> + if (dev->ops->release) {
>> + mutex_lock(&kvm->lock);
>> + list_del(&dev->vm_node);
>
> Because the device is now no longer in the kvm->devices list,
> kvm_destroy_devices() won't find it there and therefore won't call the
> device's destroy method. In fact now the device's destroy method will
> never get called; I can't see how kvmppc_xive_free() or
> kvmppc_xive_native_free() will ever get called. Thus the memory for
> the kvmppc_xive structs will never get freed as far as I can see.
ah yes. indeed ...
> We could fix that by freeing both of the kvm->arch.xive_devices
> entries at VM destruction time.
That is what I was doing in the first patch I sent :
http://patchwork.ozlabs.org/patch/1082303/
It worked fine and then, I had this better (worse) idea which I included
in v5.
> If it is true that any device that has a release method will never see
> its destroy method being called, then that needs to be documented
> clearly somewhere.
Yes. Closing the fd should take care of it. I have to rework that patch.
Thanks,
C.
