Re: [libvirt] [PATCH 4/6] qemu: Use qemuDomainAdjustMaxMemLock()

On Wed, 2015-12-09 at 15:15 -0500, John Ferlan wrote: > On 11/24/2015 08:56 AM, Andrea Bolognani wrote: >> Replace all uses of the qemuDomainRequiresMlock/virProcessSetMaxMemLock >> combination with the equivalent qemuDomainAdjustMaxMemLock() call. >> --- >> src/qemu/qemu_hotplug.c | 40 +++++++++++++--------------------------- >> 1 file changed, 13 insertions(+), 27 deletions(-) > > OK - so now I see how this is going to be used... patch 3's multi > purpose MaxMemLock makes a bit more sense; however, I'm still wondering > how we'd ever get to a point where the a last removal wouldn't have set > the lockbytes value to anything but the original (yes, patch 5 is > missing from the current removal)... > > Seems like perhaps the original_memlock is fixing the bug that was shown > in patch 5 - that the hostdev removal didn't return our value properly. > > IOW: If the original_memlock adjustment was taken out, is there a case > (after patch 5 is applied) where when memory locking is no longer > required that the value after the removal wouldn't have been what was > saved in original_memlock. Hi John, first of all, thanks for the review. I'm in the process of going through all your comments, but I wanted to address this one first. I tried to give a somewhat detailed overview of the series in the cover letter to help reviewers understand how the single commits fit toghether, but it's always hard to balance the amount of information when you've just finished implementing something and all the details are still so completely clear in your head. Any tips on how the overview could have been more helpful are appreciated!

The whole issue stems from the use of this kind of construct: if (qemuDomainRequiresMlock(vm->def)) virProcessSetMaxMemLock(vm->pid, qemuDomainGetMlockLimitBytes(vm->def)); qemuDomainRequiresMlock() is always true on ppc64, but on x86 it's true only if at least one VFIO device is currently assigned to the guest, which means it can return false both because no VFIO device has been assigned to the guest yet or because we just removed the last one.

In the first case, we don't need to adjust the memory lock limit and everything is peachy; in the latter case, though, we *had* increased the memory locking limit when we assigned the first VFIO device and we should lower it now that's no longer required, but because of the code above we don't. qemuDomainAdjustMaxMemLock() solves this by storing the previous memory locking limit when first increasing it, so now we can tell whether memory locking is simply not needed or *no longer* needed, and we can behave accordingly - not doing anything in the former case, restoring the previous value in the latter. > Perhaps would have been easier to have added the Adjust code without > original_memlock, then replace code as is done here, then add the > hostdev case, then add the original value. Just trying to separate new > functionality from code motion/creation of helper code. It's never easy to order changes, is it? :)

I can see how using the ordering you propose would make it easier to follow along, though. I will shuffle commits according to your suggestion for v2. Cheers.