Re: [libvirt] [PATCHv7 13/18] qemu: enable resctrl monitor in qemu
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(a)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(a)redhat.com>
John
Thanks for review.
Huaqiang
[...]
Attachments:
- attachment.html (text/html — 40.3 KB)