Re: [PATCH v3 06/38] x86/resctrl: Move data_width to be a schema property

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

Hi James,

On 6/14/24 8:00 AM, James Morse wrote:
> The resctrl architecture code gets to specify the width of the schema
> entries that are used by resctrl. These are determined by the schema
> format, e.g. percentage or bitmap.
> 
> Move this property into struct resctrl_schema and get the filesystem
> parts of resctrl to set it based on the schema format.
> 
> This allows rdt_init_padding() to be removed, its work can be done
> by schemata_list_add(), allowing max_name_width and max_data_width
> to be moved out of core.c which has no counterpart after the
> move to fs.

Please do write commit messages in imperative mood.

> 
> The logic for calculating max_name_width was moved in earlier patches,
> but the definition was not moved.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v2:
>   * This patch is new.
> ---
>   arch/x86/kernel/cpu/resctrl/core.c     | 26 --------------------------
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 11 +++++++++++
>   include/linux/resctrl.h                |  4 ++--
>   3 files changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 4a5216a13b46..4de7d20aa5aa 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -44,12 +44,6 @@ static DEFINE_MUTEX(domain_list_lock);
>    */
>   DEFINE_PER_CPU(struct resctrl_pqr_state, pqr_state);
>   
> -/*
> - * Used to store the max resource name width and max resource data width
> - * to display the schemata in a tabular format
> - */
> -int max_name_width, max_data_width;
> -
>   /*
>    * Global boolean for rdt_alloc which is true if any
>    * resource allocation is enabled.
> @@ -222,7 +216,6 @@ static bool __get_mem_config_intel(struct rdt_resource *r)
>   			return false;
>   		r->membw.arch_needs_linear = false;
>   	}
> -	r->data_width = 3;
>   
>   	if (boot_cpu_has(X86_FEATURE_PER_THREAD_MBA))
>   		r->membw.throttle_mode = THREAD_THROTTLE_PER_THREAD;
> @@ -262,8 +255,6 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>   	r->membw.throttle_mode = THREAD_THROTTLE_UNDEFINED;
>   	r->membw.min_bw = 0;
>   	r->membw.bw_gran = 1;
> -	/* Max value is 2048, Data width should be 4 in decimal */
> -	r->data_width = 4;
>   
>   	r->alloc_capable = true;
>   
> @@ -283,7 +274,6 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>   	r->cache.cbm_len = eax.split.cbm_len + 1;
>   	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>   	r->cache.shareable_bits = ebx & r->default_ctrl;
> -	r->data_width = (r->cache.cbm_len + 3) / 4;
>   	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
>   		r->cache.arch_has_sparse_bitmasks = ecx.split.noncont;
>   	r->alloc_capable = true;
> @@ -631,20 +621,6 @@ static int resctrl_arch_offline_cpu(unsigned int cpu)
>   	return 0;
>   }
>   
> -/*
> - * Choose a width for the resource name and resource data based on the
> - * resource that has widest name and cbm.
> - */
> -static __init void rdt_init_padding(void)
> -{
> -	struct rdt_resource *r;
> -
> -	for_each_alloc_capable_rdt_resource(r) {
> -		if (r->data_width > max_data_width)
> -			max_data_width = r->data_width;
> -	}
> -}
> -
>   enum {
>   	RDT_FLAG_CMT,
>   	RDT_FLAG_MBM_TOTAL,
> @@ -942,8 +918,6 @@ static int __init resctrl_late_init(void)
>   	if (!get_rdt_resources())
>   		return -ENODEV;
>   
> -	rdt_init_padding();
> -
>   	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>   				  "x86/resctrl/cat:online:",
>   				  resctrl_arch_online_cpu,
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index af9968328771..4f8e20cc06eb 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -58,6 +58,12 @@ static struct kernfs_node *kn_mongrp;
>   /* Kernel fs node for "mon_data" directory under root */
>   static struct kernfs_node *kn_mondata;
>   
> +/*
> + * Used to store the max resource name width and max resource data width
> + * to display the schemata in a tabular format
> + */

I understand that you just copied existing text here, but since you touch this line
could you please have this sentence end with a period?

> +int max_name_width, max_data_width;
> +
>   static struct seq_buf last_cmd_status;
>   static char last_cmd_status_buf[512];
>   
> @@ -2600,15 +2606,20 @@ static int schemata_list_add(struct rdt_resource *r, enum resctrl_conf_type type
>   	switch (r->schema_fmt) {
>   	case RESCTRL_SCHEMA_BITMAP:
>   		s->fmt_str = "%d=%0*x";
> +		s->data_width = (r->cache.cbm_len + 3) / 4;
>   		break;
>   	case RESCTRL_SCHEMA_PERCENTAGE:
>   		s->fmt_str = "%d=%0*u";
> +		s->data_width = 3;
>   		break;
>   	case RESCTRL_SCHEMA_MBPS:
>   		s->fmt_str = "%d=%0*u";
> +		s->data_width = 4;
>   		break;
>   	}
>   
> +	max_data_width = max(max_data_width, s->data_width);
> +
>   	INIT_LIST_HEAD(&s->list);
>   	list_add(&s->list, &resctrl_schema_all);
>   
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index abecbd92ac93..ddcd938972d2 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -182,7 +182,6 @@ enum resctrl_schema_fmt {
>    * @membw:		If the component has bandwidth controls, their properties.
>    * @domains:		RCU list of all domains for this resource
>    * @name:		Name to use in "schemata" file.
> - * @data_width:		Character width of data when displaying
>    * @default_ctrl:	Specifies default cache cbm or memory B/W percent.
>    * @schema_fmt:	Which format string and parser is used for this schema.
>    * @evt_list:		List of monitoring events
> @@ -198,7 +197,6 @@ struct rdt_resource {
>   	struct resctrl_membw	membw;
>   	struct list_head	domains;
>   	char			*name;
> -	int			data_width;
>   	u32			default_ctrl;
>   	enum resctrl_schema_fmt	schema_fmt;
>   	struct list_head	evt_list;
> @@ -218,6 +216,7 @@ struct rdt_resource *resctrl_arch_get_resource(enum resctrl_res_level l);
>    * @list:	Member of resctrl_schema_all.
>    * @name:	The name to use in the "schemata" file.
>    * @fmt_str:	Format string to show domain value
> + * @data_width:	Character width of data when displaying
>    * @conf_type:	Whether this schema is specific to code/data.
>    * @res:	The resource structure exported by the architecture to describe
>    *		the hardware that is configured by this schema.
> @@ -229,6 +228,7 @@ struct resctrl_schema {
>   	struct list_head		list;
>   	char				name[8];
>   	const char			*fmt_str;
> +	int				data_width;
>   	enum resctrl_conf_type		conf_type;
>   	struct rdt_resource		*res;
>   	u32				num_closid;

Reinette