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. > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list