Re: [PATCH v4 09/13] x86/resctrl: Add sysfs interface files to read/write event configuration

[Reinette Chatre]

Hi Babu,

On 9/19/2022 8:46 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 9/16/22 10:58, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/7/2022 11:01 AM, Babu Moger wrote:
>>> Add two new sysfs files to read/write the event configuration if
>>> the feature Bandwidth Monitoring Event Configuration (BMEC) is
>>> supported. The file mbm_local_config is for the configuration
>>> of the event mbm_local_bytes and the file mbm_total_config is
>>> for the configuration of mbm_total_bytes.
>>>
>>> $ls /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local*
>>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes
>>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_config
>>>
>>> $ls /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total*
>>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
>>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_config
>>>
>> This patch makes the mbm*config files per monitor group. Looking
>> ahead at later patches how the configuration is set it is not clear
>> to me that this is the right place for these configuration files.
>>
>> Looking ahead to patch 10 there is neither rmid nor closid within
>> the (MSR_IA32_EVT_CFG_BASE + index) register - it only takes
>> the bits indicating what access types needs to be counted. Also
>> in patch 10 I understand that the scope of this register is per L3 cache
>> domain.
> Yes. Scope of  MSR_IA32_EVT_CFG_BASE per L3 domain.
>>
>> Considering this, why is the sysfs file associated with each
>> monitor group?
> Please see the response below.
>>
>> For example, consider the following scenario:
>> # cd /sys/fs/resctrl
>> # mkdir g2
>> # mkdir mon_groups/m1
>> # mkdir mon_groups/m2
>> # find . | grep mbm_local_config
>> ./mon_data/mon_L3_00/mbm_local_config
>> ./mon_data/mon_L3_01/mbm_local_config
>> ./g2/mon_data/mon_L3_00/mbm_local_config
>> ./g2/mon_data/mon_L3_01/mbm_local_config
>> ./mon_groups/m2/mon_data/mon_L3_00/mbm_local_config
>> ./mon_groups/m2/mon_data/mon_L3_01/mbm_local_config
>> ./mon_groups/m1/mon_data/mon_L3_00/mbm_local_config
>> ./mon_groups/m1/mon_data/mon_L3_01/mbm_local_config
>>
>>
>> From what I understand, the following sysfs files are
>> associated with cache domain #0 and thus writing to any of these
>> files would change the same configuration:
>> ./mon_data/mon_L3_00/mbm_local_config
>> ./g2/mon_data/mon_L3_00/mbm_local_config
>> ./mon_groups/m2/mon_data/mon_L3_00/mbm_local_config
>> ./mon_groups/m1/mon_data/mon_L3_00/mbm_local_config
>>
>> Could you please correct me where I am wrong?
> 
> For example, we have CPUs 0-7 in domain 0. We have two counters which are
> configurable.
> 
> Lets consider same example as your mentioned about.
> 
> g2 is a control group.
> 
> m1 and m2 are monitor group.
> 
> We can have control group g2 with CPUs 0-7 to limit the L3 bandwidth (or
> memory bandwidth with required schemata setting).
> 
> We can have mon group m1 with cpus 0-3 to monitor mbm_local_bytes.
> 
> We can have mon group m2 with cpus  4-7 to monitor mbm_total_bytes.
> 
> Each group is independently, monitoring two separate thing. Without having

Right, because monitoring, the actual counting of the events, is per monitor
group. When a monitor group is created a new RMID is created and when the
counter is read it is per-RMID. 

The event configuration is independent from the RMID using the counter.

> sysfs file (mbm_local_config and mbm_total_config) in each monitor group,
> we wont be able to configure the above configuration.

I do not understand this reasoning. From what I understand the
event configuration is independent from the monitoring group. Thus, changing
an event configuration for one monitoring group would impact all
monitoring groups using that event counter. This implementation associates
an event configuration with each monitoring group and by doing so it
implies that it is unique to the monitoring group, but that is not
how it works.

Reinette