Re: [PATCH v3 07/10] x86/resctrl: Add sysfs interface files to read/write event configuration

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 8/22/2022 6:43 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
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |    3 +++
>  arch/x86/kernel/cpu/resctrl/monitor.c  |    2 ++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   32 ++++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c049a274383c..fc725f5e9024 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -72,11 +72,13 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>   * struct mon_evt - Entry in the event list of a resource
>   * @evtid:		event id
>   * @name:		name of the event
> + * @config:	current configuration
>   * @list:		entry in &rdt_resource->evt_list
>   */
>  struct mon_evt {
>  	u32			evtid;
>  	char			*name;
> +	char			*config;
>  	struct list_head	list;
>  };
>  
> @@ -95,6 +97,7 @@ union mon_data_bits {
>  		unsigned int rid	: 10;
>  		unsigned int evtid	: 8;
>  		unsigned int domid	: 14;
> +		unsigned int mon_config : 32;
>  	} u;
>  };
>  

This does not seem to be used in this patch.

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index b9de417dac1c..3f900241dbab 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -656,11 +656,13 @@ static struct mon_evt llc_occupancy_event = {
>  static struct mon_evt mbm_total_event = {
>  	.name		= "mbm_total_bytes",
>  	.evtid		= QOS_L3_MBM_TOTAL_EVENT_ID,
> +	.config 	= "mbm_total_config",
>  };
>  
>  static struct mon_evt mbm_local_event = {
>  	.name		= "mbm_local_bytes",
>  	.evtid		= QOS_L3_MBM_LOCAL_EVENT_ID,
> +	.config 	= "mbm_local_config",
>  };
>  
>  /*
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 855483b297a8..30d2182d4fda 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,
> +};
> +
>  static bool is_cpu_list(struct kernfs_open_file *of)
>  {
>  	struct rftype *rft = of->kn->priv;
> @@ -2534,6 +2538,25 @@ void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r, unsigned int dom_id)
>  	}
>  }
>  
> +static int mon_config_addfile(struct kernfs_node *parent_kn, const char *name,
> +			      void *priv)
> +{
> +	struct kernfs_node *kn;
> +	int ret = 0;
> +
> +	kn = __kernfs_create_file(parent_kn, name, 0644,
> +			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
> +			&kf_mondata_config_ops, priv, NULL, NULL);
> +	if (IS_ERR(kn))
> +		return PTR_ERR(kn);
> +
> +	ret = rdtgroup_kn_set_ugid(kn);
> +	if (ret)
> +		kernfs_remove(kn);
> +
> +	return ret;
> +}
> +
>  static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>  				struct rdt_domain *d,
>  				struct rdt_resource *r, struct rdtgroup *prgrp)
> @@ -2568,6 +2591,15 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>  		if (ret)
>  			goto out_destroy;
>  
> +		/* Create the sysfs event configuration files */
> +		if (r->mon_configurable &&
> +		    (mevt->evtid == QOS_L3_MBM_TOTAL_EVENT_ID ||
> +		     mevt->evtid == QOS_L3_MBM_LOCAL_EVENT_ID)) {
> +			ret = mon_config_addfile(kn, mevt->config, priv.priv);
> +			if (ret)
> +				goto out_destroy;
> +		}
> +

This seems complex to have event features embedded in the code in this way. Could
the events not be configured during system enumeration? For example, instead
of hardcoding the config like above to always set:

  static struct mon_evt mbm_local_event = {
  	.name		= "mbm_local_bytes",
  	.evtid		= QOS_L3_MBM_LOCAL_EVENT_ID,
 +	.config 	= "mbm_local_config",


What if instead this information is dynamically set in rdt_get_mon_l3_config()? To
make things simpler struct mon_evt could get a new member "configurable" and the
events that actually support configuration will have this set only
if system has X86_FEATURE_BMEC (struct rdt_resource->configurable then
becomes unnecessary?). Being configurable thus becomes an event property, not
a resource property. The "config" member introduced here could then be "config_name".

I think doing so will also make this file creation simpler with a single 
mon_config_addfile() (possibly with more parameters) used to add both files to
avoid the code duplication introduced by mon_config_addfile() above.

What do you think?

>  		if (is_mbm_event(mevt->evtid))
>  			mon_event_read(&rr, r, d, prgrp, mevt->evtid, true);
>  	}
> 
> 

Reinette