Re: [PATCH v4 04/39] x86/resctrl: Use schema type to determine how to parse schema values

[Reinette Chatre]

Hi James,

On 8/2/24 10:28 AM, James Morse wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 9ca542a8e2d4..57c88e1c2adf 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -72,7 +72,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
>   			.mon_scope		= RESCTRL_L3_CACHE,
>   			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_L3),
>   			.mon_domains		= mon_domain_init(RDT_RESOURCE_L3),
> -			.parse_ctrlval		= parse_cbm,
> +			.schema_fmt		= RESCTRL_SCHEMA_BITMAP,
>   			.format_str		= "%d=%0*x",
>   		},
>   		.msr_base		= MSR_IA32_L3_CBM_BASE,
> @@ -85,7 +85,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
>   			.name			= "L2",
>   			.ctrl_scope		= RESCTRL_L2_CACHE,
>   			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_L2),
> -			.parse_ctrlval		= parse_cbm,
> +			.schema_fmt		= RESCTRL_SCHEMA_BITMAP,
>   			.format_str		= "%d=%0*x",
>   		},
>   		.msr_base		= MSR_IA32_L2_CBM_BASE,
> @@ -98,7 +98,11 @@ struct rdt_hw_resource rdt_resources_all[] = {
>   			.name			= "MB",
>   			.ctrl_scope		= RESCTRL_L3_CACHE,
>   			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_MBA),
> -			.parse_ctrlval		= parse_bw,
> +			/*
> +			 * MBA schema_fmt is modified by
> +			 * __rdt_get_mem_config_amd()
> +			 */
> +			.schema_fmt		= RESCTRL_SCHEMA_PERCENTAGE,
>   			.format_str		= "%d=%*u",
>   		},
>   	},
> @@ -109,7 +113,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
>   			.name			= "SMBA",
>   			.ctrl_scope		= RESCTRL_L3_CACHE,
>   			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_SMBA),
> -			.parse_ctrlval		= parse_bw,
> +			.schema_fmt		= RESCTRL_SCHEMA_MBPS,
>   			.format_str		= "%d=%*u",
>   		},
>   	},

...

> @@ -195,6 +204,19 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>   	return 0;
>   }
>   
> +static ctrlval_parser_t *get_parser(struct rdt_resource *r)
> +{
> +	switch (r->schema_fmt) {
> +	case RESCTRL_SCHEMA_BITMAP:
> +		return &parse_cbm;
> +	case RESCTRL_SCHEMA_PERCENTAGE:
> +	case RESCTRL_SCHEMA_MBPS:
> +		return &parse_bw;
> +	}
> +
> +	return NULL;
> +}
> +
>   /*
>    * For each domain in this resource we expect to find a series of:
>    *	id=mask

...

> @@ -192,6 +191,18 @@ enum resctrl_scope {
>   	RESCTRL_L3_NODE,
>   };
>   
> +/**
> + * enum resctrl_schema_fmt - The format user-space provides for a schema.
> + * @RESCTRL_SCHEMA_BITMAP:	The schema is a bitmap in hex.
> + * @RESCTRL_SCHEMA_PERCENTAGE:	The schema is a decimal percentage value.
> + * @RESCTRL_SCHEMA_MBPS:	The schema is a decimal MBps value.
> + */
> +enum resctrl_schema_fmt {
> +	RESCTRL_SCHEMA_BITMAP,
> +	RESCTRL_SCHEMA_PERCENTAGE,
> +	RESCTRL_SCHEMA_MBPS,
> +};
> +

I believe that the choice of RESCTRL_SCHEMA_PERCENTAGE and RESCTRL_SCHEMA_MBPS has
potential for significant confusion. The closest place to where user space can enter
a MBps value (which is actually MiBps) is on Intel when resctrl is mounted with mba_MBps,
and as per above it will have the "RESCTRL_SCHEMA_PERCENTAGE" format. What is considered
here as RESCTRL_SCHEMA_MBPS also cannot really be considered as "MBPS" since it is used to
cover AMD's values that are "multiples of one eighth GB/s". Any new resource that
_actually_ uses MBPS will thus not be able to use RESCTRL_SCHEMA_MBPS.

Considering that RESCTRL_SCHEMA_PERCENTAGE and RESCTRL_SCHEMA_MBPS use the same parser,
could "RESCTRL_SCHEMA_RANGE" be more fitting? I acknowledge that it is very generic and better
ideas are welcome. A "range" does seem to be appropriate considering the later patch (x86/resctrl:
Add max_bw to struct resctrl_membw) that codes an explicit max.

Reinette