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

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&lt;huaqiang.wang(a)intel.com&gt; > --- > 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 */ >