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

[Wang, Huaqiang]

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 at 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 at redhat.com>
>
> John

Thanks for review.
Huaqiang

> [...]

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181112/401b3931/attachment-0001.htm>