Re: [PATCH v8 5/7] x86/resctrl: Add "mba_MBps_event" file to ctrl_mon directories

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

Hi Tony,

On 10/29/24 10:28 AM, Tony Luck wrote:
> When the mba_MBps mount option is used, provide a file in each
> ctrl_mon directory to show which memory monitoring event is
> being used.

Could the changelog be expanded a bit more to inform reader what
the monitoring event is used for?

I would also like to remind about the expectations documented in
"Changelog" section of Documentation/process/maintainer-tip.rst:
"A good structure is to explain the context, the problem and the solution
in separate paragraphs and this order."

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h    |  2 ++
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 25 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 17 +++++++++++++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index a6f051fb2e69..5f3438ca9e2b 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -609,6 +609,8 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  				char *buf, size_t nbytes, loff_t off);
>  int rdtgroup_schemata_show(struct kernfs_open_file *of,
>  			   struct seq_file *s, void *v);
> +int rdtgroup_mba_mbps_event_show(struct kernfs_open_file *of,
> +				 struct seq_file *s, void *v);
>  bool rdtgroup_cbm_overlaps(struct resctrl_schema *s, struct rdt_ctrl_domain *d,
>  			   unsigned long cbm, int closid, bool exclusive);
>  unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r, struct rdt_ctrl_domain *d,
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 200d89a64027..b9ba419e5c88 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -518,6 +518,31 @@ static int smp_mon_event_count(void *arg)
>  	return 0;
>  }
>  
> +int rdtgroup_mba_mbps_event_show(struct kernfs_open_file *of,
> +				 struct seq_file *s, void *v)
> +{
> +	struct rdtgroup *rdtgrp;
> +
> +	rdtgrp = rdtgroup_kn_lock_live(of->kn);
> +
> +	if (rdtgrp) {
> +		switch (rdtgrp->mba_mbps_event) {
> +		case QOS_L3_MBM_LOCAL_EVENT_ID:
> +			seq_puts(s, "mbm_local_bytes\n");
> +			break;
> +		case QOS_L3_MBM_TOTAL_EVENT_ID:
> +			seq_puts(s, "mbm_total_bytes\n");
> +			break;
> +		case QOS_L3_OCCUP_EVENT_ID:
> +			break;

Having a value of QOS_L3_OCCUP_EVENT_ID would surely be a kernel bug.
What do you think of a WARN_ON_ONCE()/pr_warn_once() here?

If mba_mbps_event is indeed expected to have a value of "0" to
reflect "uninitialized" then it could also be handled here
to catch any kernel bugs.

> +		}

The custom is to return -ENOENT if no rdtgrp.

> +	}
> +
> +	rdtgroup_kn_unlock(of->kn);
> +
> +	return 0;
> +}
> +
>  void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  		    struct rdt_mon_domain *d, struct rdtgroup *rdtgrp,
>  		    cpumask_t *cpumask, int evtid, int first)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5034a3dd0430..3ba81963e981 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1943,6 +1943,12 @@ static struct rftype res_common_files[] = {
>  		.seq_show	= rdtgroup_schemata_show,
>  		.fflags		= RFTYPE_CTRL_BASE,
>  	},
> +	{
> +		.name		= "mba_MBps_event",
> +		.mode		= 0644,

Please only support writing to file when appropriate callback exists.

> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= rdtgroup_mba_mbps_event_show,
> +	},
>  	{
>  		.name		= "mode",
>  		.mode		= 0644,
> @@ -2042,6 +2048,15 @@ void __init mbm_config_rftype_init(const char *config)
>  		rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
>  }
>  
> +static void mba_mbps_event_init(bool enable)

fyi ...
https://lore.kernel.org/all/237409fb566288d9f3dc7568385e6488b62dbba0.1730244116.git.babu.moger@amd.com/

> +{
> +	struct rftype *rft;
> +
> +	rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
> +	if (rft)
> +		rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;

I think this sets this file to be created for all CTRL groups, even when not supporting
monitoring?

> +}
> +
>  /**
>   * rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file
>   * @r: The resource group with which the file is associated.
> @@ -2371,6 +2386,8 @@ static int set_mba_sc(bool mba_sc)
>  			d->mbps_val[i] = MBA_MAX_MBPS;
>  	}
>  
> +	mba_mbps_event_init(mba_sc);
> +
>  	return 0;
>  }
>  

Reinette