Re: [libvirt] [PATCHv5 08/19] util: Add interface for creating monitor group
On 10/12/2018 10:27 PM, John Ferlan wrote:
[...]
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.
That usually is best. I think the order has me off a bit too, but there
is no perfect solution for order. I find myself looking forward a lot
and trying to keep track.
I'll change the order of the series to fix this.
John
Thanks for review.
Huaqiang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv5 08/19] util: Add interface for creating monitor group
[...]
> 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.
>
That usually is best. I think the order has me off a bit too, but there
is no perfect solution for order. I find myself looking forward a lot
and trying to keep track.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv5 08/19] util: Add interface for creating monitor group
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
>> ---
>> 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
Re: [libvirt] [PATCHv5 08/19] util: Add interface for creating monitor group
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
> ---
> 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
> 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)"?
Furthermore, if we Ref it here, but return -1 in the subsequent steps
are we sure that the Unref gets done.
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.
> +
> +if (virResctrlMonitorDeterminePath(monitor, machinename) < 0)
> +return -1;
At least for now virResctrlMonitorDeterminePath can handle a NULL
monitor->alloc...
John
> +
> +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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv5 08/19] util: Add interface for creating monitor group
Add interface for creating the resource monitoring group according
to '@virResctrlMonitor->path'.
Signed-off-by: Wang Huaqiang
---
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);
}
+
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);
+
+if (virResctrlMonitorDeterminePath(monitor, machinename) < 0)
+return -1;
+
+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__ */
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
