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
sysfs file (mbm_local_config and mbm_total_config) in each monitor group,
we wont be able to configure the above configuration.
>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 40 ++++++++++++++++++++++++--------
>> 1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index f55a693fa958..da11fdad204d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -254,6 +254,10 @@ static const struct kernfs_ops kf_mondata_ops = {
>> .seq_show = rdtgroup_mondata_show,
>> };
>>
>> +static const struct kernfs_ops kf_mondata_config_ops = {
>> + .atomic_write_len = PAGE_SIZE,
>> +};
>> +
> Please use coding style (tabs vs spaces) that is consistent with area
> you are contributing to.
Sure
>
>> static bool is_cpu_list(struct kernfs_open_file *of)
>> {
>> struct rftype *rft = of->kn->priv;
>> @@ -2478,24 +2482,40 @@ static struct file_system_type rdt_fs_type = {
>> .kill_sb = rdt_kill_sb,
>> };
>>
>> -static int mon_addfile(struct kernfs_node *parent_kn, const char *name,
>> +static int mon_addfile(struct kernfs_node *parent_kn, struct mon_evt *mevt,
>> void *priv)
>> {
>> - struct kernfs_node *kn;
>> + struct kernfs_node *kn_evt, *kn_evt_config;
>> int ret = 0;
>>
>> - kn = __kernfs_create_file(parent_kn, name, 0444,
>> - GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
>> - &kf_mondata_ops, priv, NULL, NULL);
>> - if (IS_ERR(kn))
>> - return PTR_ERR(kn);
>> + kn_evt = __kernfs_create_file(parent_kn, mevt->name, 0444,
>> + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
>> + &kf_mondata_ops, priv, NULL, NULL);
> Please run your series through checkpatch (alignment issue above)
Sure
>
>> + if (IS_ERR(kn_evt))
>> + return PTR_ERR(kn_evt);
>>
>> - ret = rdtgroup_kn_set_ugid(kn);
>> + ret = rdtgroup_kn_set_ugid(kn_evt);
>> if (ret) {
>> - kernfs_remove(kn);
>> + kernfs_remove(kn_evt);
>> return ret;
>> }
>>
>> + if (mevt->configurable) {
>> + kn_evt_config = __kernfs_create_file(parent_kn,
>> + mevt->config_name, 0644,
>> + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
>> + &kf_mondata_config_ops, priv, NULL, NULL);
>> + if (IS_ERR(kn_evt_config))
>> + return PTR_ERR(kn_evt_config);
>> +
> Since an error is returned here it seems that some cleanup (kn_evt) is missing?
Yes. That is correct. Will fix it.
Thanks
Babu
>
>
>> + ret = rdtgroup_kn_set_ugid(kn_evt_config);
>> + if (ret) {
>> + kernfs_remove(kn_evt_config);
>> + kernfs_remove(kn_evt);
>> + return ret;
>> + }
>> + }
>> +
>> return ret;
>> }
>>
> Reinette
--
Thanks
Babu Moger
