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

[John Ferlan]

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.

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.

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 8804d3d..a5c134a 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1276,17 +1276,14 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
>      }
>  
>      /* Temporarily add the hostdev to the domain definition. This is needed
> -     * because qemuDomainRequiresMlock() and qemuDomainGetMlockLimitBytes()
> -     * require the hostdev to be already part of the domain definition, but
> -     * other functions like qemuAssignDeviceHostdevAlias() used below expect
> -     * it *not* to be there. A better way to handle this would be nice */
> +     * because qemuDomainAdjustMaxMemLock() requires the hostdev to be already
> +     * part of the domain definition, but other functions like
> +     * qemuAssignDeviceHostdevAlias() used below expect it *not* to be there.
> +     * A better way to handle this would be nice */
>      vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> -    if (qemuDomainRequiresMlock(vm->def)) {
> -        if (virProcessSetMaxMemLock(vm->pid,
> -                                    qemuDomainGetMlockLimitBytes(vm->def)) < 0) {
> -            vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
> -            goto error;
> -        }
> +    if (qemuDomainAdjustMaxMemLock(vm) < 0) {
> +        vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
> +        goto error;
>      }
>      vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
>  
> @@ -1772,7 +1769,6 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      virJSONValuePtr props = NULL;
>      virObjectEventPtr event;
>      bool fix_balloon = false;
> -    bool mlock = false;
>      int id;
>      int ret = -1;
>  
> @@ -1804,12 +1800,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>  
> -    mlock = qemuDomainRequiresMlock(vm->def);
> -
> -    if (mlock &&
> -        virProcessSetMaxMemLock(vm->pid,
> -                                qemuDomainGetMlockLimitBytes(vm->def)) < 0) {
> -        mlock = false;
> +    if (qemuDomainAdjustMaxMemLock(vm) < 0) {

The current code says don't run the "reset" code if the SetMaxMemLock
failed... With this change, in the removedef code you'll call the
AdjustMaxMemLock regardless of whether we failed to set.  That fail to
set (at this point in time) could be the "GetMaxMemLock" failed or the
"SetMaxMemLock" failed.

The point being at removedef, how do we know if the failure was because
we failed to Adjust or something else.  If we failed to Adjust, then
calling it again will of course more than likely fail.


John
>          virJSONValueFree(props);
>          goto removedef;
>      }
> @@ -1870,13 +1861,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>          mem = NULL;
>  
>      /* reset the mlock limit */
> -    if (mlock) {
> -        virErrorPtr err = virSaveLastError();
> -        ignore_value(virProcessSetMaxMemLock(vm->pid,
> -                                             qemuDomainGetMlockLimitBytes(vm->def)));
> -        virSetError(err);
> -        virFreeError(err);
> -    }
> +    virErrorPtr err = virSaveLastError();
> +    ignore_value(qemuDomainAdjustMaxMemLock(vm));
> +    virSetError(err);
> +    virFreeError(err);
>  
>      goto audit;
>  }
> @@ -2970,9 +2958,7 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
>      virDomainMemoryDefFree(mem);
>  
>      /* decrease the mlock limit after memory unplug if necessary */
> -    if (qemuDomainRequiresMlock(vm->def))
> -        ignore_value(virProcessSetMaxMemLock(vm->pid,
> -                                             qemuDomainGetMlockLimitBytes(vm->def)));
> +    ignore_value(qemuDomainAdjustMaxMemLock(vm));
>  
>      return 0;
>  }
>