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 <huaqiang.wang@xxxxxxxxx>
>> ---
>> 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 <monitor> is found by no <cachetune> is found.
> Thus, we create an empty <cachetune> (one with no <cache> elements). To
> me that's a default environment.
>
> I assume a similar paradigm could exist if there was or wasn't a
> <memorytune> 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 <monitor> entry has
the same 'vcpus' setting as <cachetune>.
<cachetune vcpus='0-1'>
<monitor level='3' vcpus='0-1'/>
</cachetune>
and following example illustrates a <cachetune> with two monitors, one of
them is a default monitor
<cachetune vcpus='0-1'>
<monitor level='3' vcpus='0-1'/>
<monitor level='3' vcpus='0'/>
</cachetune>
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.
<cachetune vcpus='0-1'/>
or
<cachetune vcpus='0-1'>
</cachetune>
>> */
>> 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 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.:
>
> <cachetune vcpus='0-1' ... />
> <cachetune vcpus='2-3' ... />
>
> and
>
> <monitor vcpus='0-1' .../>
> <monitor vcpus='2-3' .../>
>
> so, then it would seem there would be two defaults.
Yes. Two defaults. But I think the <monitor .. > entry should be placed under
<cachetune> entry.
I'd like to rewrite your configuration in following way:
<cachetune vcpus='0-1'>
<monitor levels='3' vcpus='0-1'/>
<cachetune/>
<cachetune vcpus='2-3'>
<monitor levels='3' vcpus='2-3'/>
<cachetune/>
upper configuration creates two allocations and one default monitor for each
allocation.
By the way
<cachetune vcpus='0-1' />
has no effect for resctrl but not be considered as an error.
Unlike default allocation, default monitor could be created for multiple
times in scope of system, but with a limitation that you can only create one
default monitor for one allocation at most.
There is only one default allocation in whole system.
Every monitor is linked with some specific allocation, either default allocation
or non-default allocation (or just 'allocation').
The upper manner of monitor is determined by kernel's 'resctrl' file system.
If I created an allocation by creating a directory 'p0' under '/sys/fs/resctrl/',
and after adding two vcpus' PID to file '/sys/fs/resctrl/p0/tasks' and making
some changes to 'sys/fs/resctrl/p0/schemata' file, then
the allocation is functional for resource limitation.
The libvirt CAT code is doing similar operations for VM in an automatic way.
Let me show the files under '/sys/fs/resctrl/p0':
.
├── cpus
├── cpus_list
├── mon_data
│ ├── mon_L3_00
│ │ ├── llc_occupancy
│ │ ├── mbm_local_bytes
│ │ └── mbm_total_bytes
│ └── mon_L3_01
│ ├── llc_occupancy
│ ├── mbm_local_bytes
│ └── mbm_total_bytes
├── mon_groups
├── schemata
└── tasks
We can find some files for the monitoring role, the 'llc_occupancy'
file, the 'mbm_local_bytes' file and the 'mbm_total_bytes' file.
The truth is 'resctrl' fs create a monitoring group for each allocation
group along with its creation, and this monitoring group is what I
introduced default monitor.
The default monitor shares the same 'tasks' file with allocation, so
it monitors the resource utilization for all pids existing in 'tasks' file.
Since this monitoring group is created whenever creating a libvirt
allocation, in the design of libvirt resctrl monitor, I choose to make it
shown not shown according to the XML configuration.
To create other monitoring groups just making sub-directories under
the 'mon_group' directory, and adding corresponding vcpu PID to
that sub-directory's 'tasks' file.
The virResctrlMonitorCreate function creates this kind of sub-directory
under this 'mon_group' directory for non-default monitor.
For non-default monitor, you can specify a subset of pids of that in
allocation 'tasks' file, and no pid overlap allowed between non-default
monitors.
For an allocation with one default monitor and a non-default monitor
(or just 'monitor' in wording), the files layout are like these:
.
├── cpus
├── cpus_list
├── mon_data <--- default monitor interface
│ ├── mon_L3_00
│ │ ├── llc_occupancy
│ │ ├── mbm_local_bytes
│ │ └── mbm_total_bytes
│ └── mon_L3_01
│ ├── llc_occupancy
│ ├── mbm_local_bytes
│ └── mbm_total_bytes
├── mon_groups
│ └── mon1
│ ├── cpus
│ ├── cpus_list
│ ├── mon_data <--- non-default monitor interface
│ │ ├── mon_L3_00
│ │ │ ├── llc_occupancy
│ │ │ ├── mbm_local_bytes
│ │ │ └── mbm_total_bytes
│ │ └── mon_L3_01
│ │ ├── llc_occupancy
│ │ ├── mbm_local_bytes
│ │ └── mbm_total_bytes
│ └── tasks
├── schemata
└── tasks
This patch is trying to let monitor has the capability to mark a monitor
as a default monitor, and the default monitor is 'physically' existed in
kernel 'resctrl' file system, and which has a different manner with
other monitors.
>
> Is all this being done to save a few steps in
> virResctrlMonitorDeterminePath? If so, then I see no value. It only adds
> confusion.
default monitor has a different role with other monitors, hope I have
documented it clearly.
Without identifying the default monitor, all monitors will be create under
allocation's 'mon_group' directory, the following configuration will not be
supported due to overlap between monitors.
<cachetune vcpus='0-1'>
<monitor level='3' vcpus='0-1'/>
<monitor level='3' vcpus='0'/>
</cachetune>
So default monitor is valuable if you get to know the backend mechanisim
of kernel resctrl file system.
>
> John
Thanks for review.
Huaqiang
>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
