[libvirt] [PATCHv7 13/18] qemu: enable resctrl monitor in qemu

[John Ferlan]

On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> Add functions for creating, destroying, reconnecting resctrl
> monitor in qemu according to the configuration in domain XML.
> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> ---
>  src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 

[...]

> @@ -5430,6 +5441,7 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
>  {
>      pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid);
>      virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
> +    virDomainResctrlMonDefPtr mon = NULL;
>      size_t i = 0;
>  
>      if (qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU,
> @@ -5440,11 +5452,42 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
>          return -1;
>  
>      for (i = 0; i < vm->def->nresctrls; i++) {
> +        size_t j = 0;
>          virDomainResctrlDefPtr ct = vm->def->resctrls[i];
>  
>          if (virBitmapIsBitSet(ct->vcpus, vcpuid)) {
>              if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0)
>                  return -1;
> +
> +            /* The order of invoking virResctrlMonitorAddPID matters, it is
> +             * required to invoke this function first for monitor that has
> +             * the same vcpus setting as the allocation in same def->resctrl.
> +             * Otherwise, some other monitor's pid may be removed from its
> +             * resource group's 'tasks' file.*/
> +            for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
> +                mon = vm->def->resctrls[i]->monitors[j];
> +
> +                if (!virBitmapEqual(ct->vcpus, mon->vcpus))
> +                    continue;
> +
> +                if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
> +                    if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0)
> +                        return -1;
> +                }
> +                break;
> +            }
> +
> +            for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
> +                mon = vm->def->resctrls[i]->monitors[j];
> +
> +                if (virBitmapEqual(ct->vcpus, mon->vcpus))
> +                    continue;
> +
> +                if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
> +                    if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0)
> +                        return -1;
> +                }
> +            }
>              break;
>          }
>      }

As I'm reading the subsequent patch, I'm wondering about the order of
the patches, what happens on the vcpu hotunplug case, and are we doing
the "correct thing" on the reconnect path.

1. with respect to order - it probably doesn't really matter, but I
guess adding another list to manage in the next patch got my attention
and thinking well shouldn't the above code for MonitorAddPID be "ready"
and not changed afterwards.

2. Since qemuProcessSetupVcpu is called from qemuDomainHotplugAddVcpu
that means we probably need to handle qemuDomainHotplugDelVcpu
processing. Of course, when Martin added the resctrl support in commit
9a2fc2db8, the corollary wasn't handled.

So the *tasks file could have stale pids in it if vcpus were unplugged
since there's nothing (obvious to me) that removes the pid from the
file.  Furthermore a stale pid in the grand scheme of pid reusage could
become not stale and what happens if a pid is found - do we end up in
some error path...

3. In the reconnect logic - it would seem that we would be starting from
scratch again, right? So would the *tasks file even be valid since vcpus
could be added/deleted and we didn't get notified... So perhaps we need
some logic in the reconnect code to reinitialize the tasks file (IOW:
delete it since we're going to recreate it - I assume).

Perhaps more questions now vis-a-vis any sort of ACK/push for patches
starting here. At least 1-12 are in decent shape.

John

> @@ -7207,8 +7250,18 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>      /* Remove resctrl allocation after cgroups are cleaned up which makes it
>       * kind of safer (although removing the allocation should work even with
>       * pids in tasks file */
> -    for (i = 0; i < vm->def->nresctrls; i++)
> +    for (i = 0; i < vm->def->nresctrls; i++) {
> +        size_t j = 0;
> +
> +        for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
> +            virDomainResctrlMonDefPtr mon = NULL;
> +
> +            mon = vm->def->resctrls[i]->monitors[j];
> +            virResctrlMonitorRemove(mon->instance);
> +        }
> +
>          virResctrlAllocRemove(vm->def->resctrls[i]->alloc);
> +    }
>  
>      qemuProcessRemoveDomainStatus(driver, vm);
>  
> @@ -7939,9 +7992,20 @@ qemuProcessReconnect(void *opaque)
>          goto error;
>  
>      for (i = 0; i < obj->def->nresctrls; i++) {
> +        size_t j = 0;
> +
>          if (virResctrlAllocDeterminePath(obj->def->resctrls[i]->alloc,
>                                           priv->machineName) < 0)
>              goto error;
> +
> +        for (j = 0; j < obj->def->resctrls[i]->nmonitors; j++) {
> +            virDomainResctrlMonDefPtr mon = NULL;
> +
> +            mon = obj->def->resctrls[i]->monitors[j];
> +            if (virResctrlMonitorDeterminePath(mon->instance,
> +                                               priv->machineName) < 0)
> +                goto error;
> +        }
>      }
>  
>      /* update domain state XML with possibly updated state in virDomainObj */
>