On 11/6/2018 2:01 AM, John Ferlan wrote:
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@intel.com>
---
 src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e9c7618..fba4fb4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c

[...]


@@ -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++) {

s/vm->def->resctrls[i]/ct/  (above and below)


Yes. Will make such kind of substitution for all cases.


+                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;

It seems this break should be inside the IsBitSet, right (as is for the
ct->alloc, vcpupid match)?  Otherwise, we run the loop just once and not
run until we find our vcpuid in mon->vcpus

Yes. You are right. 

+            }
+

The next loop is duplicitous and can be removed, right?


In my original code logic, which is I need a @monitor->pids array to track all pids belonging to
@monitor, these two loops are necessary. The first loop ignores all non-default monitors
and adds the default monitor's PIDs to its 'tasks' file if default monitor exists. The second
loop adds non-default monitors' PIDs. This order is intentionally designed because file writing
for default monitor's 'tasks' file will remove PIDs that existing in other monitor's 'tasks' file.

But I have taken your suggestion that the @monitor->pids is meaningless and removed this
array, then, these two loops are not needed any more, only the second loop is kept. Along
with the review comments you made the code would be:

@@ -5440,11 +5451,26 @@ 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;
+
+            for (j = 0; j < ct->nmonitors; j++) {
+                mon = ct->monitors[j];
+
+                if (virBitmapEqual(ct->vcpus, mon->vcpus))
+                    continue;
+
+                if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
+                    if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0)
+                        return -1;
+                    break;
+                }
+            }
+
             break;
         }
     }


with some adjustments (which I can make as described),

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

Thanks for review.
Huaqiang

[...]