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 <huaqiang.wang@xxxxxxxxx>
>>> ---
>>> 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 <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'/>
This gets tagged as a default monitor just because the vcpus match?
> <monitor level='3' vcpus='0'/>
and this is not a default monitor because the vcpus don't match
> </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>
>
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 <cache>, that gets added to some cache list.
When you see a <monitor> 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)
> 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.
and a default monitor vs. a non-default monitor has no specific meaning
afaict.
>
> 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').
>
Simplify the code - default allocation means no <cache> lines true?
Is monitor the related to the cache or the cachetune? If you had:
<cachetune 0-4>
<cache 0>
<cache 1-2>
<cache 3>
<cache 4>
...
Then is :
<monitor 0-4>
<monitor 2>
acceptible? If so, then I'm not seeing the need for default monitor
and/or default 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.
>
The next hunk I'll need to look at later - too many other things cycling
through my head right now. Nice picture, but correlating this to code is
not clicking. My quick look though - I see the same files in both the
default and non-default pictures - the path to get there is different,
but that path afaik is generated as part of the processing and shouldn't
know whether it's default or non-default. It's just a path to data based
on some other "base" path.
John
> 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.
>
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
