[libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev

[John Ferlan]

On 12/10/2015 12:36 PM, Andrea Bolognani wrote:
> On Thu, 2015-12-10 at 12:11 -0500, John Ferlan wrote:
>>>> Since the add was in qemuDomainAttachHostPCIDevice, why is the remove
>>>> not in qemuDomainDetachHostPCIDevice?  Doing it here would mean it's
>>>> call for any host device removal - whether or not it was ever added.
>>>
>>> Because qemuDomainDetachHostPCIDevice() is where we ask for the device
>>> to be removed from the guest, but we have to wait for it to actually
>>> be removed (and for the guest definition to be updated) before changing
>>> the memory locking limit.
>>>
>>> I guess I could move this code to qemuDomainDetachThisHostDevice(), but
>>> keeping it close to where the guest definition is updated looks cleaner
>>> to me.
>>
>> Yeah - this all got confusing as I was paging back and forth...
>> "DetachDevice" and "RemoveDevice"... Then I reviewed another change for
>> shmem and got even more confused.
> 
> I know what you mean: the code could sure use some moving around to
> make it a bit more symmetric. And what about the names?
> 
>   qemuDomainDetachDiskDevice()
>   qemuDomainDetachDeviceDiskLive()
> 
> Ehm...
> 
>   qemuDomainDetachHostPCIDevice()
>   qemuDomainDetachHostDevice()
>   qemuDomainDetachThisHostDevice()
> 
> Oh boy :)
> 
>>> I also fail to see how a device that was never added could be removed,
>>> could you please elaborate?
>>
>> Since qemuDomainRemoveHostDevice could be called for any host device
>> removal and not specifically the HostPCIDevice, I was pointing out by
>> calling qemuDomainAdjustMaxMemLock there you could be calling it even
>> when a HostPCIDevice wasn't being removed.
> 
> Oh, I get it now.
> 
> Calling qemuDomainAdjustMaxMemLock() more than necessary shouldn't
> really cause any harm, but adding a check for
> 
>   hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI
> 
> around the call is even better.
> 
> Would that address your concerns?
> 

You mean of course before the pesky "virDomainHostdevDefFree(hostdev)"?

You could perhaps do it after the qemuDomainRemovePCIHostDevice in the
switch or within that qemuDomainRemovePCIHostDevice code.

John
>> It was clear to me when I wrote it ;-)
> 
> Again, I know exactly what you mean ;)
>