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

["Moger, Babu" <babu.moger@amd.com>]

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@amd.com>
>> ---
>>  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