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@xxxxxxxxx>
>> ---
>> 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__ */
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
