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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5eea2e6a-f062-4ff4-9d11-87d8af0306dd@intel.com>
Date: Tue, 13 Aug 2024 20:56:58 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: James Morse <james.morse@....com>, <x86@...nel.org>,
	<linux-kernel@...r.kernel.org>
CC: Fenghua Yu <fenghua.yu@...el.com>, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, H Peter Anvin
	<hpa@...or.com>, Babu Moger <Babu.Moger@....com>,
	<shameerali.kolothum.thodi@...wei.com>, D Scott Phillips OS
	<scott@...amperecomputing.com>, <carl@...amperecomputing.com>,
	<lcherian@...vell.com>, <bobo.shaobowang@...wei.com>,
	<tan.shaopeng@...itsu.com>, <baolin.wang@...ux.alibaba.com>, Jamie Iles
	<quic_jiles@...cinc.com>, Xin Hao <xhao@...ux.alibaba.com>,
	<peternewman@...gle.com>, <dfustini@...libre.com>, <amitsinght@...vell.com>,
	David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>, Dave
 Martin <dave.martin@....com>
Subject: Re: [PATCH v4 04/39] x86/resctrl: Use schema type to determine how to
 parse schema values

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