Re: [libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev
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?
It was clear to me when I wrote it ;-)
Again, I know exactly what you mean ;)
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team