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

[Reinette Chatre]

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.

Considering this, why is the sysfs file associated with each
monitor group?

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?


> 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.

>  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)

> +	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?


> +		ret = rdtgroup_kn_set_ugid(kn_evt_config);
> +		if (ret) {
> +			kernfs_remove(kn_evt_config);
> +			kernfs_remove(kn_evt);
> +			return ret;
> +		}
> +	}
> +
>  	return ret;
>  }
>  

Reinette