Re: [PATCH v16 25/34] fs/resctrl: Add event configuration directory under info/L3_MON/

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

Hi Babu,

On 7/25/25 11:29 AM, Babu Moger wrote:


> ---
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 4c24c5f3f4c1..3dfc177f9792 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -310,6 +310,38 @@ with the following files:
>  	  # cat /sys/fs/resctrl/info/L3_MON/available_mbm_cntrs
>  	  0=30;1=30
>  
> +"event_configs":
> +	Directory that exists when "mbm_event" counter assignment mode is supported.
> +	Contains sub-directory for each MBM event that can be assigned to a counter.

"Contains sub-directory" -> "Contains a sub-directory"?

> +
> +	Two MBM events are supported by default: mbm_local_bytes and mbm_total_bytes.
> +	Each MBM event's sub-directory contains a file named "event_filter" that is
> +	used to view and modify which memory transactions the MBM event is configured
> +	with.
> +
> +	List of memory transaction types supported:
> +
> +	==========================  ========================================================
> +	Name			    Description
> +	==========================  ========================================================
> +	dirty_victim_writes_all     Dirty Victims from the QOS domain to all types of memory
> +	remote_reads_slow_memory    Reads to slow memory in the non-local NUMA domain
> +	local_reads_slow_memory     Reads to slow memory in the local NUMA domain
> +	remote_non_temporal_writes  Non-temporal writes to non-local NUMA domain
> +	local_non_temporal_writes   Non-temporal writes to local NUMA domain
> +	remote_reads                Reads to memory in the non-local NUMA domain
> +	local_reads                 Reads to memory in the local NUMA domain
> +	==========================  ========================================================
> +
> +	For example::
> +
> +	  # cat /sys/fs/resctrl/info/L3_MON/event_configs/mbm_total_bytes/event_filter
> +	  local_reads,remote_reads,local_non_temporal_writes,remote_non_temporal_writes,
> +	  local_reads_slow_memory,remote_reads_slow_memory,dirty_victim_writes_all
> +
> +	  # cat /sys/fs/resctrl/info/L3_MON/event_configs/mbm_local_bytes/event_filter
> +	  local_reads,local_non_temporal_writes,local_reads_slow_memory
> +
>  "max_threshold_occupancy":
>  		Read/write file provides the largest value (in
>  		bytes) at which a previously used LLC_occupancy

...

> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 16bcfeeb89e6..fa5f63126682 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -929,6 +929,29 @@ struct mbm_transaction mbm_transactions[NUM_MBM_TRANSACTIONS] = {
>  	{"dirty_victim_writes_all", DIRTY_VICTIMS_TO_ALL_MEM},
>  };
>  
> +int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v)
> +{
> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
> +	bool sep = false;
> +	int i;
> +
> +	mutex_lock(&rdtgroup_mutex);
> +

There is inconsistency among the files introduced on how
"mbm_event mode disabled" case is handled. Some files return failure
from their _show()/_write() when "mbm_event mode is disabled", some don't. 

The "event_filter" file always prints the MBM transactions monitored
when assignable counters are supported, whether mbm_event mode is enabled
or not. This means that the MBM event's configuration values are printed
when "default" mode is enabled.  I have two concerns about this
1) This is potentially very confusing since switching to "default" will
   make the BMEC files visible that will enable the user to modify the
   event configurations per domain. Having this file print a global event
   configuration while there are potentially various different domain-specific
   configuration active will be confusing.
2) Can it be guaranteed that the MBM events will monitor the default
   assignable counter memory transactions when in "default" mode? It has
   never been possible to query which memory transactions are monitored by
   the default X86_FEATURE_CQM_MBM_TOTAL and X86_FEATURE_CQM_MBM_LOCAL features
   so this seems to use one feature to deduce capabilities or another?



> +	for (i = 0; i < NUM_MBM_TRANSACTIONS; i++) {
> +		if (mevt->evt_cfg & mbm_transactions[i].val) {
> +			if (sep)
> +				seq_putc(seq, ',');
> +			seq_printf(seq, "%s", mbm_transactions[i].name);
> +			sep = true;
> +		}
> +	}
> +	seq_putc(seq, '\n');
> +
> +	mutex_unlock(&rdtgroup_mutex);
> +
> +	return 0;
> +}
> +
>  /**
>   * resctrl_mon_resource_init() - Initialise global monitoring structures.
>   *
> @@ -982,6 +1005,7 @@ int resctrl_mon_resource_init(void)
>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
>  		resctrl_file_fflags_init("available_mbm_cntrs",
>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
> +		resctrl_file_fflags_init("event_filter", RFTYPE_ASSIGN_CONFIG);
>  	}
>  
>  	return 0;

...

> @@ -2295,6 +2339,18 @@ static int rdtgroup_mkdir_info_resdir(void *priv, char *name,
>  		return ret;
>  
>  	ret = rdtgroup_add_files(kn_subdir, fflags);
> +	if (ret)
> +		return ret;
> +
> +	if ((fflags & RFTYPE_MON_INFO) == RFTYPE_MON_INFO) {
> +		r = priv;
> +		if (r->mon.mbm_cntr_assignable) {
> +			ret = resctrl_mkdir_event_configs(r, kn_subdir);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
>  	if (!ret)
>  		kernfs_activate(kn_subdir);
>  

Looks like the "if (!ret)" above can be dropped to always call "kernfs_activate(kn_subdir)"
on exit making it clear that this is success path and function exits early on any error.

Reinette