Re: [libvirt] [PATCHv5 10/19] util: Introduce default monitor
On 10/11/18 8:02 AM, Wang, Huaqiang wrote:
> Answers refined.
>
> On 10/11/2018 3:14 AM, John Ferlan wrote:
>>
>> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>>> In resctrl file system, more than one monitoring groups
>>> could be created within one allocation group, along with
>>> the creation of allocation group, a monitoring group is
>>> created at the same, which monitors the resource
>>> utilization information of whole allocation group.
>>>
>>> This patch is introducing the concept of default monitor,
>>> which represents the particular monitoring group that
>>> created along with the creation of allocation group.
>>>
>>> Default monitor shares the common 'vcpu' list with the
>>> allocation.
>>>
>>> Signed-off-by: Wang Huaqiang
>>> ---
>>> src/libvirt_private.syms | 1 +
>>> src/util/virresctrl.c| 23 +++
>>> src/util/virresctrl.h| 2 ++
>>> 3 files changed, 26 insertions(+)
>>>
I assume the two responses to my review are very similar, but I'll use
this second one since it's timewise later...
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index c93d19f..4b22ed4 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -2689,6 +2689,7 @@ virResctrlMonitorGetID;
>>> virResctrlMonitorNew;
>>> virResctrlMonitorRemove;
>>> virResctrlMonitorSetCacheLevel;
>>> +virResctrlMonitorSetDefault;
>>> virResctrlMonitorSetID;
>>>
[...]
>>> + * a monitoring group is created at the same, which monitors the resource
>>> + * utilization information of whole allocation group.
>> Reword - it's a bit redundant
>
> Rewording like these:
>
> * There are two type of monitors, the default monitor and the non-default
> * monitor. Within one allocation, up to one default monitor and more than
> * one non-default monitors could be created.
> *
> * The flag 'default_monitor' defined in structure virResctrlMonitor denotes
> * if a monitor is the default one or not.
>
Starting with "Within one allocation,..." - doesn't make much sense to
me as a reader, sorry.
>>> + * A virResctrlMonitor with @default_monitor marked as 'true' is
>>> representing
>>> + * the monitoring group created along with the creation of allocation
>>> group.
>> Well I'm a bit lost, but let's see what happens. I'm not sure what
>> you're trying to delineate here. There is the creation of an
>> resctrl->alloc when a is found by no is found.
>> Thus, we create an empty (one with no elements). To
>> me that's a default environment.
>>
>> I assume a similar paradigm could exist if there was or wasn't a
>> element...
>
> Make you confused, my bad. I was trying to introducing the situation of
> '/sys/fs/resctrl' filesystem and what I was done here at the same time.
>
> Regarding the XML layout for default monitor, following XML lines represents
> a default monitor, the key rule is the default monitor's entry has
> the same 'vcpus' setting as .
>
>
>
>
>
> and following example illustrates a with two monitors, one of
> them is a default monitor
>
>
This gets tagged as a default monitor just because the vcpus match?
>
and this is not a default monitor because the vcpus don't match
>
>
> Particularly, following XML layout doesn't define any monitor or allocation.
> Following XML lines does not have any effect, and does not indicate an
> error.
>
>
>
> or
>
>
>
>
I understand that, but that wasn't my point. If someone modified their
XML and added just that, then add the resctrl->alloc as soon as you see
it. Then when you see a , that gets added to some cache list.
When you see a that gets added to some monitor list.
The whole concept of calling some a default something just isn't working
for me. It's a cache or monitor for either all or some subset of the
vcpus for the cachetune (or resctrl->alloc). Nothing more, nothing less.
>>> */
>>> struct _virResctrlMonitor {
>>> virObject parent;
>>> @@ -355,6 +362,8 @@ struct _virResctrlMonitor {
>>> /* libvirt-generated path in /sys/fs/resctrl for this particular
>>> * monitor */
>>> char *path;
>>> +/* Boolean flag for default monitor */
>>> +bool default_monitor;
>>> /* The cache 'level', special for cache monitor */
>>> unsigned int cache_level;
>>> };
>>> @@ -2499,6 +2508,13 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
>>> monitor,
>>> return -1;
>>> }
>>>
>>> +if (monitor->default_monitor) {
>>> +if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
>> See check below ... at this point monitor->alloc could be NULL and won't
>> make this STRDUP very happy.
>
> Thanks for catching my bug. I did not run the code on my machine with default
> monitor, because I have trouble in run libvirt with CAT, creating non-default
> resctrl allocation. I am working on it, and will more tests for monitor.
>
> Using following code to fix it: (also changed next lines)
>
> if (monitor->alloc)
>
Re: [libvirt] [PATCHv5 10/19] util: Introduce default monitor
Answers refined.
On 10/11/2018 3:14 AM, John Ferlan wrote:
>
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>> In resctrl file system, more than one monitoring groups
>> could be created within one allocation group, along with
>> the creation of allocation group, a monitoring group is
>> created at the same, which monitors the resource
>> utilization information of whole allocation group.
>>
>> This patch is introducing the concept of default monitor,
>> which represents the particular monitoring group that
>> created along with the creation of allocation group.
>>
>> Default monitor shares the common 'vcpu' list with the
>> allocation.
>>
>> Signed-off-by: Wang Huaqiang
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virresctrl.c| 23 +++
>> src/util/virresctrl.h| 2 ++
>> 3 files changed, 26 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index c93d19f..4b22ed4 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2689,6 +2689,7 @@ virResctrlMonitorGetID;
>> virResctrlMonitorNew;
>> virResctrlMonitorRemove;
>> virResctrlMonitorSetCacheLevel;
>> +virResctrlMonitorSetDefault;
>> virResctrlMonitorSetID;
>>
>>
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index fca1f6f..41e8d48 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -340,6 +340,13 @@ struct _virResctrlAlloc {
>> * bandwidth technology (MBM), as well as the CAT and MBA, are all
>> orthogonal
>> * features. The monitor will be created under the scope of default
>> allocation
>> * if no CAT or MBA supported in the system.
>> + *
>> + * In resctrl file sytem, more than one monitoring groups could be created'
> In the resctrl file system, more than one monitoring group could...
Got.
>
>> + * within one allocation group, along with the creation of allocation group,
> s/group, along/group. Along/
Got
>
>> + * a monitoring group is created at the same, which monitors the resource
>> + * utilization information of whole allocation group.
> Reword - it's a bit redundant
Rewording like these:
* There are two type of monitors, the default monitor and the non-default
* monitor. Within one allocation, up to one default monitor and more than
* one non-default monitors could be created.
*
* The flag 'default_monitor' defined in structure virResctrlMonitor denotes
* if a monitor is the default one or not.
>> + * A virResctrlMonitor with @default_monitor marked as 'true' is
>> representing
>> + * the monitoring group created along with the creation of allocation group.
> Well I'm a bit lost, but let's see what happens. I'm not sure what
> you're trying to delineate here. There is the creation of an
> resctrl->alloc when a is found by no is found.
> Thus, we create an empty (one with no elements). To
> me that's a default environment.
>
> I assume a similar paradigm could exist if there was or wasn't a
> element...
Make you confused, my bad. I was trying to introducing the situation of
'/sys/fs/resctrl' filesystem and what I was done here at the same time.
Regarding the XML layout for default monitor, following XML lines represents
a default monitor, the key rule is the default monitor's entry has
the same 'vcpus' setting as .
and following example illustrates a with two monitors, one of
them is a default monitor
Particularly, following XML layout doesn't define any monitor or allocation.
Following XML lines does not have any effect, and does not indicate an
error.
or
>> */
>> struct _virResctrlMonitor {
>> virObject parent;
>> @@ -355,6 +362,8 @@ struct _virResctrlMonitor {
>> /* libvirt-generated path in /sys/fs/resctrl for this particular
>> * monitor */
>> char *path;
>> +/* Boolean flag for default monitor */
>> +bool default_monitor;
>> /* The cache 'level', special for cache monitor */
>> unsigned int cache_level;
>> };
>> @@ -2499,6 +2508,13 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
>> monitor,
>> return -1;
>> }
>>
>> +if (monitor->default_monitor) {
>> +if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
> See check below ... at this point monitor->alloc could be NULL and won't
> make this STRDUP very happy.
Thanks for catching my bug. I did not run the code on my machine with default
monitor, because I have trouble in run libvirt with CAT, creating non-default
resctrl allocation. I am working on it, and will more tests for monitor.
Using following code to fix it: (also changed next lines)
if (monitor->alloc)
alloc_path = monitor->alloc->path;
else
alloc_path = SYSFS_RESCTRL_PATH;
if (monitor->default_monitor) {
if (VIR_STRDUP(monitor->path, alloc_path) < 0)
return -1;
return 0;
}
>
>> +return -1;
>> +
>> +return
Re: [libvirt] [PATCHv5 10/19] util: Introduce default monitor
On 10/11/2018 3:14 AM, John Ferlan wrote:
>
>
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>> In resctrl file system, more than one monitoring groups
>> could be created within one allocation group, along with
>> the creation of allocation group, a monitoring group is
>> created at the same, which monitors the resource
>> utilization information of whole allocation group.
>>
>> This patch is introducing the concept of default monitor,
>> which represents the particular monitoring group that
>> created along with the creation of allocation group.
>>
>> Default monitor shares the common 'vcpu' list with the
>> allocation.
>>
>> Signed-off-by: Wang Huaqiang
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virresctrl.c| 23 +++
>> src/util/virresctrl.h| 2 ++
>> 3 files changed, 26 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index c93d19f..4b22ed4 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2689,6 +2689,7 @@ virResctrlMonitorGetID;
>> virResctrlMonitorNew;
>> virResctrlMonitorRemove;
>> virResctrlMonitorSetCacheLevel;
>> +virResctrlMonitorSetDefault;
>> virResctrlMonitorSetID;
>>
>>
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index fca1f6f..41e8d48 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -340,6 +340,13 @@ struct _virResctrlAlloc {
>> * bandwidth technology (MBM), as well as the CAT and MBA, are all
>> orthogonal
>> * features. The monitor will be created under the scope of default
>> allocation
>> * if no CAT or MBA supported in the system.
>> + *
>> + * In resctrl file sytem, more than one monitoring groups could be created'
>
> In the resctrl file system, more than one monitoring group could...
Got.
>
>
>> + * within one allocation group, along with the creation of allocation group,
>
> s/group, along/group. Along/
Got
>
>
>> + * a monitoring group is created at the same, which monitors the resource
>> + * utilization information of whole allocation group.
>
> Reword - it's a bit redundant
Rewording like these:
* There are two type of monitors, the default monitor and the non-default
* monitor. Within one allocation, up to one default monitor and more than
* one non-default monitors could be created.
*
* The flag 'default_monitor' defined in structure virResctrlMonitor denotes
* if a monitor is the default one or not.
>
>> + * A virResctrlMonitor with @default_monitor marked as 'true' is
>> representing
>> + * the monitoring group created along with the creation of allocation group.
>
> Well I'm a bit lost, but let's see what happens. I'm not sure what
> you're trying to delineate here. There is the creation of an
> resctrl->alloc when a is found by no is found.
> Thus, we create an empty (one with no elements). To
> me that's a default environment.
>
> I assume a similar paradigm could exist if there was or wasn't a
> element...
Make you confused, my bad. I was trying to introducing the situation of
'/sys/fs/resctrl' filesystem and what I was done here at the same time.
Regarding the XML layout for default monitor, following XML lines represents
a default monitor, the key rule is the default monitor's entry has
the same 'vcpus' setting as .
and following example illustrates a with two monitors, one of
them is a default monitor
Particularly, following XML layout doesn't define any monitor or allocation.
Following XML lines does not have any effect, and does not indicate an
error.
or
>
>> */
>> struct _virResctrlMonitor {
>> virObject parent;
>> @@ -355,6 +362,8 @@ struct _virResctrlMonitor {
>> /* libvirt-generated path in /sys/fs/resctrl for this particular
>> * monitor */
>> char *path;
>> +/* Boolean flag for default monitor */
>> +bool default_monitor;
>> /* The cache 'level', special for cache monitor */
>> unsigned int cache_level;
>> };
>> @@ -2499,6 +2508,13 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
>> monitor,
>> return -1;
>> }
>>
>> +if (monitor->default_monitor) {
>> +if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
>
> See check below ... at this point monitor->alloc could be NULL and won't
> make this STRDUP very happy.
Thanks for catching my bug. I did not run the code on my machine with default
monitor, because I have trouble in run libvirt with CAT, creating non-default
resctrl allocation. I am working on it, and will more tests for monitor.
Using following code to fix it: (also changed next lines)
if (monitor->alloc)
alloc_path = monitor->alloc->path;
else
alloc_path = SYSFS_RESCTRL_PATH;
if (monitor->default_monitor) {
if (VIR_STRDUP(monitor->path, alloc_path) < 0)
return -1;
return 0;
}
>
>
>> +return -1;
>> +
>> +
Re: [libvirt] [PATCHv5 10/19] util: Introduce default monitor
On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> In resctrl file system, more than one monitoring groups
> could be created within one allocation group, along with
> the creation of allocation group, a monitoring group is
> created at the same, which monitors the resource
> utilization information of whole allocation group.
>
> This patch is introducing the concept of default monitor,
> which represents the particular monitoring group that
> created along with the creation of allocation group.
>
> Default monitor shares the common 'vcpu' list with the
> allocation.
>
> Signed-off-by: Wang Huaqiang
> ---
> src/libvirt_private.syms | 1 +
> src/util/virresctrl.c| 23 +++
> src/util/virresctrl.h| 2 ++
> 3 files changed, 26 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c93d19f..4b22ed4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2689,6 +2689,7 @@ virResctrlMonitorGetID;
> virResctrlMonitorNew;
> virResctrlMonitorRemove;
> virResctrlMonitorSetCacheLevel;
> +virResctrlMonitorSetDefault;
> virResctrlMonitorSetID;
>
>
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index fca1f6f..41e8d48 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -340,6 +340,13 @@ struct _virResctrlAlloc {
> * bandwidth technology (MBM), as well as the CAT and MBA, are all orthogonal
> * features. The monitor will be created under the scope of default
> allocation
> * if no CAT or MBA supported in the system.
> + *
> + * In resctrl file sytem, more than one monitoring groups could be created'
In the resctrl file system, more than one monitoring group could...
> + * within one allocation group, along with the creation of allocation group,
s/group, along/group. Along/
> + * a monitoring group is created at the same, which monitors the resource
> + * utilization information of whole allocation group.
Reword - it's a bit redundant
> + * A virResctrlMonitor with @default_monitor marked as 'true' is representing
> + * the monitoring group created along with the creation of allocation group.
Well I'm a bit lost, but let's see what happens. I'm not sure what
you're trying to delineate here. There is the creation of an
resctrl->alloc when a is found by no is found.
Thus, we create an empty (one with no elements). To
me that's a default environment.
I assume a similar paradigm could exist if there was or wasn't a
element...
> */
> struct _virResctrlMonitor {
> virObject parent;
> @@ -355,6 +362,8 @@ struct _virResctrlMonitor {
> /* libvirt-generated path in /sys/fs/resctrl for this particular
> * monitor */
> char *path;
> +/* Boolean flag for default monitor */
> +bool default_monitor;
> /* The cache 'level', special for cache monitor */
> unsigned int cache_level;
> };
> @@ -2499,6 +2508,13 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
> monitor,
> return -1;
> }
>
> +if (monitor->default_monitor) {
> +if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
See check below ... at this point monitor->alloc could be NULL and won't
make this STRDUP very happy.
> +return -1;
> +
> +return 0;
> +}
> +
> if (monitor->alloc)
> alloc_path = monitor->alloc->path;
> else
> @@ -2739,3 +2755,10 @@
> virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
> return virResctrlMonitorGetStatistic(monitor, "llc_occupancy",
> nbank, bankids, bankcaches);
> }
> +
> +
> +void
> +virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor)
> +{
> +monitor->default_monitor = true;
> +}
I really don't see what the value of this is. Looking later on it seems
there's some sort of check that the vcpus desired for the monitor match
that of some cachetune entry and you then set default.
To me that could happen multiple times, e.g.:
and
so, then it would seem there would be two defaults.
Is all this being done to save a few steps in
virResctrlMonitorDeterminePath? If so, then I see no value. It only adds
confusion.
John
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index 6137fee..371df8a 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -228,4 +228,6 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr
> monitor,
> size_t *nbank,
> unsigned int **bankids,
> unsigned int **bankcaches);
> +void
> +virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor);
> #endif /* __VIR_RESCTRL_H__ */
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv5 10/19] util: Introduce default monitor
In resctrl file system, more than one monitoring groups
could be created within one allocation group, along with
the creation of allocation group, a monitoring group is
created at the same, which monitors the resource
utilization information of whole allocation group.
This patch is introducing the concept of default monitor,
which represents the particular monitoring group that
created along with the creation of allocation group.
Default monitor shares the common 'vcpu' list with the
allocation.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 23 +++
src/util/virresctrl.h| 2 ++
3 files changed, 26 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c93d19f..4b22ed4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2689,6 +2689,7 @@ virResctrlMonitorGetID;
virResctrlMonitorNew;
virResctrlMonitorRemove;
virResctrlMonitorSetCacheLevel;
+virResctrlMonitorSetDefault;
virResctrlMonitorSetID;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index fca1f6f..41e8d48 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -340,6 +340,13 @@ struct _virResctrlAlloc {
* bandwidth technology (MBM), as well as the CAT and MBA, are all orthogonal
* features. The monitor will be created under the scope of default allocation
* if no CAT or MBA supported in the system.
+ *
+ * In resctrl file sytem, more than one monitoring groups could be created
+ * within one allocation group, along with the creation of allocation group,
+ * a monitoring group is created at the same, which monitors the resource
+ * utilization information of whole allocation group.
+ * A virResctrlMonitor with @default_monitor marked as 'true' is representing
+ * the monitoring group created along with the creation of allocation group.
*/
struct _virResctrlMonitor {
virObject parent;
@@ -355,6 +362,8 @@ struct _virResctrlMonitor {
/* libvirt-generated path in /sys/fs/resctrl for this particular
* monitor */
char *path;
+/* Boolean flag for default monitor */
+bool default_monitor;
/* The cache 'level', special for cache monitor */
unsigned int cache_level;
};
@@ -2499,6 +2508,13 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
monitor,
return -1;
}
+if (monitor->default_monitor) {
+if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
+return -1;
+
+return 0;
+}
+
if (monitor->alloc)
alloc_path = monitor->alloc->path;
else
@@ -2739,3 +2755,10 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr
monitor,
return virResctrlMonitorGetStatistic(monitor, "llc_occupancy",
nbank, bankids, bankcaches);
}
+
+
+void
+virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor)
+{
+monitor->default_monitor = true;
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 6137fee..371df8a 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -228,4 +228,6 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr
monitor,
size_t *nbank,
unsigned int **bankids,
unsigned int **bankcaches);
+void
+virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor);
#endif /* __VIR_RESCTRL_H__ */
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
