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

[John Ferlan]

On 12/10/2015 05:43 AM, Andrea Bolognani wrote:
> 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!

I always appreciate extra comments and cover letters, but they are a
delicate balance. It's hard when you're so embroiled in your patches to
step back and take that 10,000 foot view. There's no secret sauce for
that. Yes I read cover letters, but then I start reviewing patches and
that cover letter can easily get paged out of short term memory. When I
review I try to take patches one at a time, but I will peek ahead as
well when I get curious about the why something is being done. I'll also
try to provide enough context in reviews to try an "show" what I was
thinking while reviewing. Seeing terse reviews of "this is wrong" or "I
wouldn't do it that way" in my mind are not very helpful.

> 
> 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.

As I got to patch 4 & 5 I think it started to sink in again that the
issue was more because of x86. Once I read and thought about patch 5,
then I went back to my patch 4 comments and added the extra wording
around whether or not using original_memlock would be useful and of
course to separate out "code motion" from "new code".  Of course by that
time patch 3 was already sent so I couldn't go "adjust" my comments there.

> 
> 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? :)

Well better than what my former job had - here's a steaming pile of
changes with an explanation of the changes. Sometimes you got a good
explanation of what the changes did, while other times it was "this
fixes bug #NNNNN".  Um, gee thanks!

John
> 
> 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.
>