3:48 a.m.
On 10/11/2018 3:13 AM, John Ferlan wrote:
On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> Add interface for creating the resource monitoring group according
> to '@virResctrlMonitor->path'.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virresctrl.c | 28 ++++++++++++++++++++++++++++
> src/util/virresctrl.h | 6 ++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e175c8b..a878083 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2681,6 +2681,7 @@ virResctrlInfoGetMonitorPrefix;
> virResctrlInfoMonFree;
> virResctrlInfoNew;
> virResctrlMonitorAddPID;
> +virResctrlMonitorCreate;
> virResctrlMonitorDeterminePath;
> virResctrlMonitorNew;
>
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 8b617a6..b3d20cc 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2475,6 +2475,7 @@ virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
> return virResctrlAddPID(monitor->path, pid);
> }
>
> +
This should have been squashed into patch6
OK. Two lines between each function.
> int
> virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
> const char *machinename)
> @@ -2506,3 +2507,30 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>
> return 0;
> }
> +
> +
> +int
> +virResctrlMonitorCreate(virResctrlAllocPtr alloc,
> + virResctrlMonitorPtr monitor,
> + const char *machinename)
> +{
> + int lockfd = -1;
> + int ret = -1;
> +
> + if (!monitor)
> + return 0;
> +
> + monitor->alloc = virObjectRef(alloc);
Can @alloc be NULL here? I see that the eventual caller from
qemuProcessResctrlCreate would pass vm->def->resctrls[i]->alloc after a
virResctrlAllocCreate, but that API can return 0 immediately "if
(!alloc)"?
@alloc could be NULL.
In virResctrlAllocCreate, if the passed in @alloc is NULL, the virResctrlAllocCreate
function will return 0, this is not considered as an error.
Following the code invoking virResctrlMonitorCreate, if
@vm->def->resctrls[i]->alloc
is NULL, then the virResctrlAllocCreate function will return 0, this is not an error,
code going on, then virResctrlMonitorCreate will be invoked if @nmonitors is not 0,
in this case, the first argument,the @alloc, of virResctrlMonitorCreate is NULL.
2613 for (i = 0; i < vm->def->nresctrls; i++) {
2614 size_t j = 0;
2615 if (virResctrlAllocCreate(caps->host.resctrl,
2616 vm->def->resctrls[i]->alloc,
2617 priv->machineName) < 0)
2618 goto cleanup;
2619
2620 for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
2621 virDomainResctrlMonDefPtr mon = NULL;
2622
2623 mon = vm->def->resctrls[i]->monitors[j];
2624 if (virResctrlMonitorCreate(vm->def->resctrls[i]->alloc,
2625 mon->instance,
2626 priv->machineName) < 0)
2627 goto cleanup;
2628
2629 }
2630 }
Furthermore, if we Ref it here, but return -1 in the subsequent steps
are we sure that the Unref gets done.
If in later steps error happens and returns a -1, then it will go through the resource
releasing functions by calling virResctrlMonitorDispose and virResctrlAllocDispose.
The unref of @monitor->alloc is done in virResctrlMonitorDispose (function
introduced in patch2):
401 static void
402 virResctrlMonitorDispose(void *obj)
403 {
404 virResctrlMonitorPtr monitor = obj;
405
406 virObjectUnref(monitor->alloc);
407 VIR_FREE(monitor->id);
408 VIR_FREE(monitor->path);
409 }
The one "thing" about the order of patches here is that it forces me to
look forward to ensure decisions made in previous patches will be
handled in the future.
Maybe combine virResctrlMonitorDispose and virResctrlMonitorCreate in one
patch? Will that make you look better for understanding how @monitor->alloc
is used.
> +
> + if (virResctrlMonitorDeterminePath(monitor, machinename) < 0)
> + return -1;
At least for now virResctrlMonitorDeterminePath can handle a NULL
monitor->alloc...
John
Thanks for review.
Huaqiang
> +
> + lockfd = virResctrlLockWrite();
> + if (lockfd < 0)
> + return -1;
> +
> + ret = virResctrlCreateGroupPath(monitor->path);
> +
> + virResctrlUnlock(lockfd);
> + return ret;
> +}
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index 69b6b1d..1efe394 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -196,7 +196,13 @@ virResctrlMonitorNew(void);
> int
> virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
> pid_t pid);
> +
> int
> virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
> const char *machinename);
> +
> +int
> +virResctrlMonitorCreate(virResctrlAllocPtr alloc,
> + virResctrlMonitorPtr monitor,
> + const char *machinename);
> #endif /* __VIR_RESCTRL_H__ */
>