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

[Reinette Chatre]

Hi James,

On 8/2/24 10:28 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. Remove
> rdt_init_padding(), its work is be done by schemata_list_add(),

"its work is be done by" -> "its work is done by"

> allowing max_name_width and max_data_width to be moved out of core.c
> which has no counterpart after the move to fs.
>
> The logic for calculating max_name_width was moved in earlier patches,

("patches" in changelog can be a trigger, maybe "moved in earlier patches"
-> "moved earlier"?)

> but the definition was not moved.
>
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> Tested-by: Carl Worth <carl@xxxxxxxxxxxxxxxxxxxxxx> # arm64
> ---
> Changes since v3:
>   * Moved some words around in the commit message - maybe this is the right mood?
>   * Added a full-stop to an existing comment.
>
> 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 f16014ee48aa..cb1d634274b4 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -43,12 +43,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.
> @@ -224,7 +218,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;
> @@ -264,8 +257,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;
>   
> @@ -285,7 +276,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;
> @@ -781,20 +771,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,
> @@ -1092,8 +1068,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 1ce851447923..ed06384f9161 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -57,6 +57,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.
> + */
> +int max_name_width, max_data_width;
> +
>   static struct seq_buf last_cmd_status;
>   static char last_cmd_status_buf[512];
>   
> @@ -2603,15 +2609,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);
> +

To me this emphasizes that RESCTRL_SCHEMA_PERCENTAGE and
RESCTRL_SCHEMA_MBPS are not appropriate. Note how the minimum data width
of RESCTRL_SCHEMA_MBPS is 4, this is unexpected from an actual MBps
value. The choice of "4" is specific to AMD's input but that information
is lost in this change.
We are fortunate that data_width is a minimum, allowing the software controller
to be enabled with longer data values, but that is subtle and already
breaks the goal of "making things tabular".

I wonder how useful the data_width actually is. The "make things tabular"
motivation seems to only apply to resources that have the exact same scope
and as noted earlier seems to be broken already.
I am skeptical that user space will be impacted if this is removed.

Reinette