<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 11/6/2018 2:01 AM, John Ferlan wrote: </div> <blockquote type="cite" cite="mid:b7907f39-1cb3-c867-6987-306828c133e4@redhat.com"> <pre wrap="">On 10/22/18 4:01 AM, Wang Huaqiang wrote: </pre> <blockquote type="cite"> <pre wrap="">Add functions for creating, destroying, reconnecting resctrl monitor in qemu according to the configuration in domain XML. Signed-off-by: Wang Huaqiang <a class="moz-txt-link-rfc2396E" href="mailto:huaqiang.wang@intel.com"><huaqiang.wang@intel.com></a> --- 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 </pre> </blockquote> <pre wrap="">[...] </pre> <blockquote type="cite"> <pre wrap="">@@ -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++) { </pre> </blockquote> <pre wrap="">s/vm->def->resctrls[i]/ct/ (above and below)</pre> </blockquote>  Yes. Will make such kind of substitution for all cases.  <blockquote type="cite" cite="mid:b7907f39-1cb3-c867-6987-306828c133e4@redhat.com"> <blockquote type="cite"> <pre wrap="">+ 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; </pre> </blockquote> <pre wrap="">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</pre> </blockquote> Yes. You are right. <blockquote type="cite" cite="mid:b7907f39-1cb3-c867-6987-306828c133e4@redhat.com"> <blockquote type="cite"> <pre wrap="">+ } + </pre> </blockquote> <pre wrap="">The next loop is duplicitous and can be removed, right? </pre> </blockquote> 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; } } <blockquote type="cite" cite="mid:b7907f39-1cb3-c867-6987-306828c133e4@redhat.com"> <pre wrap="">with some adjustments (which I can make as described), Reviewed-by: John Ferlan <a class="moz-txt-link-rfc2396E" href="mailto:jferlan@redhat.com"><jferlan@redhat.com></a> John </pre> </blockquote> Thanks for review. Huaqiang <blockquote type="cite" cite="mid:b7907f39-1cb3-c867-6987-306828c133e4@redhat.com"> <pre wrap="">[...] </pre> </blockquote> </body> </html>