The policy for parsing the configuration values as a string from
user-space is specified by a function pointer the arch code specifies.

These strings are part of resctrl's ABI, and the functions and their
caller both live in the same file. Exporting the parsing functions and
allowing the architecture to choose how a schema is parsed allows an
architecture to get this wrong.

Keep this all in the flesystem parts of resctrl. This should prevent any
architecture's string-parsing behaviour from varying without core code
changes. Use the fflags to spot caches and bandwidth resources, and use
the appropriate helper.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/x86/kernel/cpu/resctrl/core.c        |  4 ----
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 28 +++++++++++++++++++----
 arch/x86/kernel/cpu/resctrl/internal.h    | 10 --------
 include/linux/resctrl.h                   |  7 ------
 4 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index b773b3bdebe3..d07eff7d6750 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -75,7 +75,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.name			= "L3",
 			.cache_level		= 3,
 			.domains		= domain_init(RDT_RESOURCE_L3),
-			.parse_ctrlval		= parse_cbm,
 			.format_str		= "%d=%0*x",
 			.fflags			= RFTYPE_RES_CACHE,
 		},
@@ -89,7 +88,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.name			= "L2",
 			.cache_level		= 2,
 			.domains		= domain_init(RDT_RESOURCE_L2),
-			.parse_ctrlval		= parse_cbm,
 			.format_str		= "%d=%0*x",
 			.fflags			= RFTYPE_RES_CACHE,
 		},
@@ -103,7 +101,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.name			= "MB",
 			.cache_level		= 3,
 			.domains		= domain_init(RDT_RESOURCE_MBA),
-			.parse_ctrlval		= parse_bw,
 			.format_str		= "%d=%*u",
 			.fflags			= RFTYPE_RES_MB,
 		},
@@ -115,7 +112,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.name			= "SMBA",
 			.cache_level		= 3,
 			.domains		= domain_init(RDT_RESOURCE_SMBA),
-			.parse_ctrlval		= parse_bw,
 			.format_str		= "%d=%*u",
 			.fflags			= RFTYPE_RES_MB,
 		},
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 788ac0395645..72a651671c68 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -23,6 +23,15 @@
 
 #include "internal.h"
 
+struct rdt_parse_data {
+	struct rdtgroup		*rdtgrp;
+	char			*buf;
+};
+
+typedef int (ctrlval_parser_t)(struct rdt_parse_data *data,
+			       struct resctrl_schema *s,
+			       struct rdt_domain *d);
+
 /*
  * Check whether MBA bandwidth percentage value is correct. The value is
  * checked against the minimum and max bandwidth values specified by the
@@ -59,8 +68,8 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 	return true;
 }
 
-int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
-	     struct rdt_domain *d)
+static int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
+		    struct rdt_domain *d)
 {
 	struct resctrl_staged_config *cfg;
 	u32 closid = data->rdtgrp->closid;
@@ -138,8 +147,8 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
  * Read one cache bit mask (hex). Check that it is valid for the current
  * resource type.
  */
-int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
-	      struct rdt_domain *d)
+static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
+		     struct rdt_domain *d)
 {
 	struct rdtgroup *rdtgrp = data->rdtgrp;
 	struct resctrl_staged_config *cfg;
@@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 	return 0;
 }
 
+static ctrlval_parser_t *get_parser(struct rdt_resource *res)
+{
+	if (res->fflags & RFTYPE_RES_CACHE)
+		return &parse_cbm;
+	else
+		return &parse_bw;
+}
+
 /*
  * For each domain in this resource we expect to find a series of:
  *	id=mask
@@ -204,6 +221,7 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 static int parse_line(char *line, struct resctrl_schema *s,
 		      struct rdtgroup *rdtgrp)
 {
+	ctrlval_parser_t *parse_ctrlval = get_parser(s->res);
 	enum resctrl_conf_type t = s->conf_type;
 	struct resctrl_staged_config *cfg;
 	struct rdt_resource *r = s->res;
@@ -235,7 +253,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
 		if (d->id == dom_id) {
 			data.buf = dom;
 			data.rdtgrp = rdtgrp;
-			if (r->parse_ctrlval(&data, s, d))
+			if (parse_ctrlval(&data, s, d))
 				return -EINVAL;
 			if (rdtgrp->mode ==  RDT_MODE_PSEUDO_LOCKSETUP) {
 				cfg = &d->staged_config[t];
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 65990def6c79..9048bd32e86f 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -413,11 +413,6 @@ static inline bool is_mbm_event(int e)
 		e <= QOS_L3_MBM_LOCAL_EVENT_ID);
 }
 
-struct rdt_parse_data {
-	struct rdtgroup		*rdtgrp;
-	char			*buf;
-};
-
 /**
  * struct rdt_hw_resource - arch private attributes of a resctrl resource
  * @r_resctrl:		Attributes of the resource used directly by resctrl.
@@ -455,11 +450,6 @@ static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r
 	return container_of(r, struct rdt_hw_resource, r_resctrl);
 }
 
-int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
-	      struct rdt_domain *d);
-int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
-	     struct rdt_domain *d);
-
 extern struct mutex rdtgroup_mutex;
 
 extern struct rdt_hw_resource rdt_resources_all[];
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 168cc9510069..6e87bc95f5ea 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -157,9 +157,6 @@ struct resctrl_membw {
 	u32				*mb_map;
 };
 
-struct rdt_parse_data;
-struct resctrl_schema;
-
 /**
  * struct rdt_resource - attributes of a resctrl resource
  * @rid:		The index of the resource
@@ -174,7 +171,6 @@ struct resctrl_schema;
  * @data_width:		Character width of data when displaying
  * @default_ctrl:	Specifies default cache cbm or memory B/W percent.
  * @format_str:		Per resource format string to show domain value
- * @parse_ctrlval:	Per resource function pointer to parse control values
  * @evt_list:		List of monitoring events
  * @fflags:		flags to choose base and info files
  * @cdp_capable:	Is the CDP feature available on this resource
@@ -192,9 +188,6 @@ struct rdt_resource {
 	int			data_width;
 	u32			default_ctrl;
 	const char		*format_str;
-	int			(*parse_ctrlval)(struct rdt_parse_data *data,
-						 struct resctrl_schema *s,
-						 struct rdt_domain *d);
 	struct list_head	evt_list;
 	unsigned long		fflags;
 	bool			cdp_capable;
-- 
2.39.2

>   /*
>    * Check whether MBA bandwidth percentage value is correct. The value is
>    * checked against the minimum and max bandwidth values specified by the
> @@ -59,8 +68,8 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
>   	return true;
>   }
>   
> -int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
> -	     struct rdt_domain *d)
> +static int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
> +		    struct rdt_domain *d)
>   {
>   	struct resctrl_staged_config *cfg;
>   	u32 closid = data->rdtgrp->closid;
> @@ -138,8 +147,8 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>    * Read one cache bit mask (hex). Check that it is valid for the current
>    * resource type.
>    */
> -int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
> -	      struct rdt_domain *d)
> +static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
> +		     struct rdt_domain *d)
>   {
>   	struct rdtgroup *rdtgrp = data->rdtgrp;
>   	struct resctrl_staged_config *cfg;
> @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>   	return 0;
>   }
>   
> +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
> +{
> +	if (res->fflags & RFTYPE_RES_CACHE)
> +		return &parse_cbm;
> +	else
> +		return &parse_bw;
> +}

Besides what Reinette said, I'd have added here something that would 
fire in case someone adds something unexpected in the future, like

WARN_ON_ONCE(!(res->fflags & (RFTYPE_RES_CACHE|RFTYPE_RES_MB));

At the beginning of the function.


Apart from that, nothing jumped at me.

-- 
Cheers,

David / dhildenb

Hi,

On Tue, Apr 09, 2024 at 05:13:01PM +0200, David Hildenbrand wrote:

[...]

> > @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
> >   	return 0;
> >   }
> > +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
> > +{
> > +	if (res->fflags & RFTYPE_RES_CACHE)
> > +		return &parse_cbm;
> > +	else
> > +		return &parse_bw;
> > +}
> 
> Besides what Reinette said, I'd have added here something that would fire in
> case someone adds something unexpected in the future, like
> 
> WARN_ON_ONCE(!(res->fflags & (RFTYPE_RES_CACHE|RFTYPE_RES_MB));
> 
> At the beginning of the function.
> 
> 
> Apart from that, nothing jumped at me.
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 

Thanks for that -- I guess that would benefit from discussion; please
see my reply to Reinette on this patch.

Cheers
---Dave

Hi James,

On 3/21/2024 9:50 AM, James Morse wrote:
> The policy for parsing the configuration values as a string from
> user-space is specified by a function pointer the arch code specifies.
> 
> These strings are part of resctrl's ABI, and the functions and their
> caller both live in the same file. Exporting the parsing functions and
> allowing the architecture to choose how a schema is parsed allows an
> architecture to get this wrong.
> 
> Keep this all in the flesystem parts of resctrl. This should prevent any

flesystem -> filesystem

> architecture's string-parsing behaviour from varying without core code
> changes. Use the fflags to spot caches and bandwidth resources, and use
> the appropriate helper.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---

...

> @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>  	return 0;
>  }
>  
> +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
> +{
> +	if (res->fflags & RFTYPE_RES_CACHE)
> +		return &parse_cbm;
> +	else
> +		return &parse_bw;
> +}

This is borderline ... at minimum it expands what fflags means and how it
is intended to be used and that needs to be documented because it reads:

	* @fflags:		flags to choose base and info files

I am curious why you picked fflags instead of an explicit check against
rid?

Reinette

On Mon, Apr 08, 2024 at 08:14:47PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:50 AM, James Morse wrote:
> > The policy for parsing the configuration values as a string from
> > user-space is specified by a function pointer the arch code specifies.
> > 
> > These strings are part of resctrl's ABI, and the functions and their
> > caller both live in the same file. Exporting the parsing functions and
> > allowing the architecture to choose how a schema is parsed allows an
> > architecture to get this wrong.
> > 
> > Keep this all in the flesystem parts of resctrl. This should prevent any
> 
> flesystem -> filesystem

Noted, thanks.


> > architecture's string-parsing behaviour from varying without core code
> > changes. Use the fflags to spot caches and bandwidth resources, and use
> > the appropriate helper.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> 
> ..
> 
> > @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
> >  	return 0;
> >  }
> >  
> > +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
> > +{
> > +	if (res->fflags & RFTYPE_RES_CACHE)
> > +		return &parse_cbm;
> > +	else
> > +		return &parse_bw;
> > +}
> 
> This is borderline ... at minimum it expands what fflags means and how it
> is intended to be used and that needs to be documented because it reads:
> 
> 	* @fflags:		flags to choose base and info files
> 
> I am curious why you picked fflags instead of an explicit check against
> rid?
> 
> Reinette

Is fflags already somewhat overloaded?  There seem to be a mix of things
that are independent Boolean flags, while other things seem mutually
exclusive or enum-like.

Do we expect RFTYPE_RES_CACHE | RFTYPE_RES_MB ever to make sense,
as David points out?


With MPAM, we could in theory have cache population control and egress
memory bandwidth controls on a single interconnect component.

If that would always be represented through resctrl as two components
with the MB controls considered one level out from the CACHE controls,
then I guess these control types remain mutually exclusive from
resctrl's point of view.

Allowing a single rdt_resource to sprout multiple control types looks
more invasive in the code, even if it logically makes sense in terms of
the hardware.

(I'm guessing that may have already been ruled out?  Apologies if I
seem to be questioning things that were decided already.  That's not
my intention, and James will already have thought about this in any
case...)


Anyway, for this patch, there seem to be a couple of assumptions:

a) get_parser() doesn't get called except for rdt_resources that
represent resource controls (so, fflags = RFTYPE_RES_foo for some "foo",
with no other flags set), and

b) there are exactly two kinds of "foo", so whatever isn't a CACHE is
a BW.

These assumptions seem to hold today (?)

But the semantics of fflags already look a bit complicated, so I can
see why it might be best to avoid anything that may add more
complexity.


If the main aim is to avoid silly copy-paste errors when coding up
resources for a new arch, would it make sense to go for a more low-
tech approach and just bundle up related fields in a macro?

E.g., something like:

#define RDT_RESOURCE_MB_DEFAULTS		\
	.format_str	= "%d=%*u",		\
	.fflags		= RFTYPE_RES_MB,	\
	.parse_ctrlval	= parse_bw

#define RDT_RESOURCE_CACHE_DEFAULTS		\
	.format_str	= "%d=%0*x",		\
	.fflags		= RFTYPE_RES_CACHE,	\
	.parse_ctrlval	= parse_cbm

This isn't particularly pretty, but would at least help avoid accidents
and reduce the amount of explicit boilerplate in the resource
definitions.

Thoughts?

Cheers
---Dave

Hi Dave,

On 4/12/2024 9:16 AM, Dave Martin wrote:
> On Mon, Apr 08, 2024 at 08:14:47PM -0700, Reinette Chatre wrote:
>> On 3/21/2024 9:50 AM, James Morse wrote:

>>> @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>>>  	return 0;
>>>  }
>>>  
>>> +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
>>> +{
>>> +	if (res->fflags & RFTYPE_RES_CACHE)
>>> +		return &parse_cbm;
>>> +	else
>>> +		return &parse_bw;
>>> +}
>>
>> This is borderline ... at minimum it expands what fflags means and how it
>> is intended to be used and that needs to be documented because it reads:
>>
>> 	* @fflags:		flags to choose base and info files
>>
>> I am curious why you picked fflags instead of an explicit check against
>> rid?
>>
>> Reinette
> 
> Is fflags already somewhat overloaded?  There seem to be a mix of things
> that are independent Boolean flags, while other things seem mutually
> exclusive or enum-like.
> 
> Do we expect RFTYPE_RES_CACHE | RFTYPE_RES_MB ever to make sense,
> as David points out?
> 
> 
> With MPAM, we could in theory have cache population control and egress
> memory bandwidth controls on a single interconnect component.
> 
> If that would always be represented through resctrl as two components
> with the MB controls considered one level out from the CACHE controls,
> then I guess these control types remain mutually exclusive from
> resctrl's point of view.
> 
> Allowing a single rdt_resource to sprout multiple control types looks
> more invasive in the code, even if it logically makes sense in terms of
> the hardware.
> 
> (I'm guessing that may have already been ruled out?  Apologies if I
> seem to be questioning things that were decided already.  That's not
> my intention, and James will already have thought about this in any
> case...)
> 
> 
> Anyway, for this patch, there seem to be a couple of assumptions:
> 
> a) get_parser() doesn't get called except for rdt_resources that
> represent resource controls (so, fflags = RFTYPE_RES_foo for some "foo",
> with no other flags set), and
> 
> b) there are exactly two kinds of "foo", so whatever isn't a CACHE is
> a BW.
> 
> These assumptions seem to hold today (?)

(c) the parser for user provided data is based on the resource type.

As I understand (c) may not be true for MPAM that supports different
partitioning controls for a single resource. For example, for a cache
MPAM supports portion as well as maximum capacity controls that
I expect would need different parsers (perhaps mapping to different
schemata entries?) from user space but will be used to control the
same resource.

I do now know if the goal is to support this MPAM capability via
resctrl but do accomplish this I wonder if it may not be more appropriate
to associate the parser with the schema entry that is presented to user space.

> But the semantics of fflags already look a bit complicated, so I can
> see why it might be best to avoid anything that may add more
> complexity.

ack.

> If the main aim is to avoid silly copy-paste errors when coding up
> resources for a new arch, would it make sense to go for a more low-
> tech approach and just bundle up related fields in a macro?

I understand this as more than avoiding copy-paste errors. I understand
the goal is to prevent architectures from having architecture specific
parsers.

> 
> E.g., something like:
> 
> #define RDT_RESOURCE_MB_DEFAULTS		\
> 	.format_str	= "%d=%*u",		\
> 	.fflags		= RFTYPE_RES_MB,	\
> 	.parse_ctrlval	= parse_bw
> 
> #define RDT_RESOURCE_CACHE_DEFAULTS		\
> 	.format_str	= "%d=%0*x",		\
> 	.fflags		= RFTYPE_RES_CACHE,	\
> 	.parse_ctrlval	= parse_cbm
> 
> This isn't particularly pretty, but would at least help avoid accidents
> and reduce the amount of explicit boilerplate in the resource
> definitions.
> 
> Thoughts?

I understand the goal of this patch to make the parser something that
the fs code owns. This is done in support of a consistent user interface.
It is not clear how turning this into macros prevents arch code from
still overriding the parser.

You do highlight another point though, shouldn't the fs code own the
format_str also? I do not think we want arch code to control the
print format, this is also something that should be consistent between
all archs and owned by fs code, again perhaps more appropriate for
a schema entry.

Reinette

On Mon, Apr 15, 2024 at 10:44:34AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/12/2024 9:16 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:14:47PM -0700, Reinette Chatre wrote:
> >> On 3/21/2024 9:50 AM, James Morse wrote:
> 
> >>> @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
> >>> +{
> >>> +	if (res->fflags & RFTYPE_RES_CACHE)
> >>> +		return &parse_cbm;
> >>> +	else
> >>> +		return &parse_bw;
> >>> +}
> >>
> >> This is borderline ... at minimum it expands what fflags means and how it
> >> is intended to be used and that needs to be documented because it reads:
> >>
> >> 	* @fflags:		flags to choose base and info files
> >>
> >> I am curious why you picked fflags instead of an explicit check against
> >> rid?
> >>
> >> Reinette
> > 
> > Is fflags already somewhat overloaded?  There seem to be a mix of things
> > that are independent Boolean flags, while other things seem mutually
> > exclusive or enum-like.
> > 
> > Do we expect RFTYPE_RES_CACHE | RFTYPE_RES_MB ever to make sense,
> > as David points out?
> > 
> > 
> > With MPAM, we could in theory have cache population control and egress
> > memory bandwidth controls on a single interconnect component.
> > 
> > If that would always be represented through resctrl as two components
> > with the MB controls considered one level out from the CACHE controls,
> > then I guess these control types remain mutually exclusive from
> > resctrl's point of view.
> > 
> > Allowing a single rdt_resource to sprout multiple control types looks
> > more invasive in the code, even if it logically makes sense in terms of
> > the hardware.
> > 
> > (I'm guessing that may have already been ruled out?  Apologies if I
> > seem to be questioning things that were decided already.  That's not
> > my intention, and James will already have thought about this in any
> > case...)
> > 
> > 
> > Anyway, for this patch, there seem to be a couple of assumptions:
> > 
> > a) get_parser() doesn't get called except for rdt_resources that
> > represent resource controls (so, fflags = RFTYPE_RES_foo for some "foo",
> > with no other flags set), and
> > 
> > b) there are exactly two kinds of "foo", so whatever isn't a CACHE is
> > a BW.
> > 
> > These assumptions seem to hold today (?)
> 
> (c) the parser for user provided data is based on the resource type.
> 
> As I understand (c) may not be true for MPAM that supports different
> partitioning controls for a single resource. For example, for a cache
> MPAM supports portion as well as maximum capacity controls that
> I expect would need different parsers (perhaps mapping to different
> schemata entries?) from user space but will be used to control the
> same resource.
> 
> I do now know if the goal is to support this MPAM capability via
> resctrl but do accomplish this I wonder if it may not be more appropriate
> to associate the parser with the schema entry that is presented to user space.
> 
> > But the semantics of fflags already look a bit complicated, so I can
> > see why it might be best to avoid anything that may add more
> > complexity.
> 
> ack.
> 
> > If the main aim is to avoid silly copy-paste errors when coding up
> > resources for a new arch, would it make sense to go for a more low-
> > tech approach and just bundle up related fields in a macro?
> 
> I understand this as more than avoiding copy-paste errors. I understand
> the goal is to prevent architectures from having architecture specific
> parsers.
> 
> > 
> > E.g., something like:
> > 
> > #define RDT_RESOURCE_MB_DEFAULTS		\
> > 	.format_str	= "%d=%*u",		\
> > 	.fflags		= RFTYPE_RES_MB,	\
> > 	.parse_ctrlval	= parse_bw
> > 
> > #define RDT_RESOURCE_CACHE_DEFAULTS		\
> > 	.format_str	= "%d=%0*x",		\
> > 	.fflags		= RFTYPE_RES_CACHE,	\
> > 	.parse_ctrlval	= parse_cbm
> > 
> > This isn't particularly pretty, but would at least help avoid accidents
> > and reduce the amount of explicit boilerplate in the resource
> > definitions.
> > 
> > Thoughts?
> 
> I understand the goal of this patch to make the parser something that
> the fs code owns. This is done in support of a consistent user interface.
> It is not clear how turning this into macros prevents arch code from
> still overriding the parser.
> 
> You do highlight another point though, shouldn't the fs code own the
> format_str also? I do not think we want arch code to control the
> print format, this is also something that should be consistent between
> all archs and owned by fs code, again perhaps more appropriate for
> a schema entry.
> 
> Reinette

Fair points, I guess.

For the print format, I was presuming that this ought to be consistent
with the parse format, so probably a core property too (until/unless
someone comes up with a convincing counterexample).


Would something like the following make sense, if you want a less
informal approach?  (Modulo minor details like naming conventions etc.)


/* In fs/resctrl.c */

struct struct resctrl_ctrl_traits {
	const char		*format_str;
	ctrlval_parser_t	*parse_ctrlval;
};

static const struct resctrl_ctrl_traits resource_traits[]  = {
	[RESTYPE_INVALID] = {},
	[RESTYPE_MB] = {
		.format_str	= "%d=%*u",
		.parse_ctrlval	= parse_bw,
	},
	[RESTYPE_CACHE] = {
		.format_str	= "%d=%0*x",
		.parse_ctrlval	= parse_cbm,
	},
};

static bool is_resource(const struct rdt_resource *r)
{
	return r->fflags & RFTYPE_RES;
}


/* In include/linux/resctrl_types.h */

+#define RFTYPE_RES			BIT(8)
-#define RFTYPE_RES_CACHE		BIT(8)
-#define RFTYPE_RES_MB			BIT(9)

/* For RFTYPE_RES: */
enum resctrl_resource_type {
	RRESTYPE_INVALID,
	RRESTYPE_MB,
	RRESTYPE_CACHE,
};

/* In include/linux/resctrl.h */

struct rdt_resource {
/* ... */

-	const char			*format_str;
+	enum resctrl_resource_type	res_type;

/* ... */
};


(RRESTYPE_INVALID would just be there to catch cases where .res_type is
not assigned.)


James might also have other thoughts about this when he gets back...

In any case, it might make sense to detach this change from this series
if we're making more significant changes in this area than just
splitting the code into core and arch parts.

(Note also, your suggestion about indexing using rid may also work;
I tend to assume that the mapping from rid to resource types may not be
fixed, but maybe I'm being too strongly influenced by MPAM...)

Cheers
---Dave

Hi Dave

On 4/16/2024 9:16 AM, Dave Martin wrote:
> On Mon, Apr 15, 2024 at 10:44:34AM -0700, Reinette Chatre wrote:
>> Hi Dave,
>>
>> On 4/12/2024 9:16 AM, Dave Martin wrote:
>>> On Mon, Apr 08, 2024 at 08:14:47PM -0700, Reinette Chatre wrote:
>>>> On 3/21/2024 9:50 AM, James Morse wrote:
>>
>>>>> @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
>>>>> +{
>>>>> +	if (res->fflags & RFTYPE_RES_CACHE)
>>>>> +		return &parse_cbm;
>>>>> +	else
>>>>> +		return &parse_bw;
>>>>> +}
>>>>
>>>> This is borderline ... at minimum it expands what fflags means and how it
>>>> is intended to be used and that needs to be documented because it reads:
>>>>
>>>> 	* @fflags:		flags to choose base and info files
>>>>
>>>> I am curious why you picked fflags instead of an explicit check against
>>>> rid?
>>>>
>>>> Reinette
>>>
>>> Is fflags already somewhat overloaded?  There seem to be a mix of things
>>> that are independent Boolean flags, while other things seem mutually
>>> exclusive or enum-like.
>>>
>>> Do we expect RFTYPE_RES_CACHE | RFTYPE_RES_MB ever to make sense,
>>> as David points out?
>>>
>>>
>>> With MPAM, we could in theory have cache population control and egress
>>> memory bandwidth controls on a single interconnect component.
>>>
>>> If that would always be represented through resctrl as two components
>>> with the MB controls considered one level out from the CACHE controls,
>>> then I guess these control types remain mutually exclusive from
>>> resctrl's point of view.
>>>
>>> Allowing a single rdt_resource to sprout multiple control types looks
>>> more invasive in the code, even if it logically makes sense in terms of
>>> the hardware.
>>>
>>> (I'm guessing that may have already been ruled out?  Apologies if I
>>> seem to be questioning things that were decided already.  That's not
>>> my intention, and James will already have thought about this in any
>>> case...)
>>>
>>>
>>> Anyway, for this patch, there seem to be a couple of assumptions:
>>>
>>> a) get_parser() doesn't get called except for rdt_resources that
>>> represent resource controls (so, fflags = RFTYPE_RES_foo for some "foo",
>>> with no other flags set), and
>>>
>>> b) there are exactly two kinds of "foo", so whatever isn't a CACHE is
>>> a BW.
>>>
>>> These assumptions seem to hold today (?)
>>
>> (c) the parser for user provided data is based on the resource type.
>>
>> As I understand (c) may not be true for MPAM that supports different
>> partitioning controls for a single resource. For example, for a cache
>> MPAM supports portion as well as maximum capacity controls that
>> I expect would need different parsers (perhaps mapping to different
>> schemata entries?) from user space but will be used to control the
>> same resource.
>>
>> I do now know if the goal is to support this MPAM capability via
>> resctrl but do accomplish this I wonder if it may not be more appropriate
>> to associate the parser with the schema entry that is presented to user space.
>>
>>> But the semantics of fflags already look a bit complicated, so I can
>>> see why it might be best to avoid anything that may add more
>>> complexity.
>>
>> ack.
>>
>>> If the main aim is to avoid silly copy-paste errors when coding up
>>> resources for a new arch, would it make sense to go for a more low-
>>> tech approach and just bundle up related fields in a macro?
>>
>> I understand this as more than avoiding copy-paste errors. I understand
>> the goal is to prevent architectures from having architecture specific
>> parsers.
>>
>>>
>>> E.g., something like:
>>>
>>> #define RDT_RESOURCE_MB_DEFAULTS		\
>>> 	.format_str	= "%d=%*u",		\
>>> 	.fflags		= RFTYPE_RES_MB,	\
>>> 	.parse_ctrlval	= parse_bw
>>>
>>> #define RDT_RESOURCE_CACHE_DEFAULTS		\
>>> 	.format_str	= "%d=%0*x",		\
>>> 	.fflags		= RFTYPE_RES_CACHE,	\
>>> 	.parse_ctrlval	= parse_cbm
>>>
>>> This isn't particularly pretty, but would at least help avoid accidents
>>> and reduce the amount of explicit boilerplate in the resource
>>> definitions.
>>>
>>> Thoughts?
>>
>> I understand the goal of this patch to make the parser something that
>> the fs code owns. This is done in support of a consistent user interface.
>> It is not clear how turning this into macros prevents arch code from
>> still overriding the parser.
>>
>> You do highlight another point though, shouldn't the fs code own the
>> format_str also? I do not think we want arch code to control the
>> print format, this is also something that should be consistent between
>> all archs and owned by fs code, again perhaps more appropriate for
>> a schema entry.
>>
>> Reinette
> 
> Fair points, I guess.
> 
> For the print format, I was presuming that this ought to be consistent
> with the parse format, so probably a core property too (until/unless
> someone comes up with a convincing counterexample).
> 
> 
> Would something like the following make sense, if you want a less
> informal approach?  (Modulo minor details like naming conventions etc.)
> 
> 
> /* In fs/resctrl.c */
> 
> struct struct resctrl_ctrl_traits {
> 	const char		*format_str;
> 	ctrlval_parser_t	*parse_ctrlval;
> };
> 
> static const struct resctrl_ctrl_traits resource_traits[]  = {
> 	[RESTYPE_INVALID] = {},
> 	[RESTYPE_MB] = {
> 		.format_str	= "%d=%*u",
> 		.parse_ctrlval	= parse_bw,
> 	},
> 	[RESTYPE_CACHE] = {
> 		.format_str	= "%d=%0*x",
> 		.parse_ctrlval	= parse_cbm,
> 	},
> };

It is not obvious to me that another layer is needed here. format_str
and parse_ctrlval can just be members of struct resctrl_schema?

> 
> static bool is_resource(const struct rdt_resource *r)
> {
> 	return r->fflags & RFTYPE_RES;
> }

I do not see the usage of is_resource().

(I think we are now discussing both this patch and patch #30 here)

Here is part relevant to #30:

What I was thinking about was something like below that uses the
enum you introduce later and lets the RF flags stay internal to fs code:

rdtgroup_create_info_dir()
{

	...
	list_for_each_entry(s, &resctrl_schema_all, list) {
		r = s->res;
		if (r->res_type == RRESTYPE_CACHE)
			fflags = RFTYPE_RES_CACHE;
		else if (r->res_type == RRESTYPE_MB)
			fflags = RFTYPE_RES_MB;
		else /* fail */
		
		fflags |= RFTYPE_CTRL_INFO;

		...
	}
	/* same idea for monitor info files */


For this patch the resource type can be used to initialize the schema
entry.

> 
> 
> /* In include/linux/resctrl_types.h */
> 
> +#define RFTYPE_RES			BIT(8)
> -#define RFTYPE_RES_CACHE		BIT(8)
> -#define RFTYPE_RES_MB			BIT(9)

The goal is to not have to expose any of the RFTYPE flags internals to
the architecture. RFTYPE_RES_CACHE and RFTYPE_RES_MB stays, but is
not exposed to arch code. I do not see need for RFTYPE_RES.
All the RFTYPE flags can be defined in fs/resctrl/internal.h

> 
> /* For RFTYPE_RES: */
> enum resctrl_resource_type {
> 	RRESTYPE_INVALID,
> 	RRESTYPE_MB,
> 	RRESTYPE_CACHE,
> };

(I find naming hard ... note the names changed from the beginning of
pseudo code to here where RESTYPE changing to RRESTYPE)

> 
> /* In include/linux/resctrl.h */
> 
> struct rdt_resource {
> /* ... */
> 
> -	const char			*format_str;
> +	enum resctrl_resource_type	res_type;
> 
> /* ... */
> };

Yes. With the above architecture code would only specify if it is
cache or memory via enum resctrl_resource_type and need not know
the individual file flags and can pick how to format and parse
data based on the resource type.

> 
> 
> (RRESTYPE_INVALID would just be there to catch cases where .res_type is
> not assigned.)
> 
> 
> James might also have other thoughts about this when he gets back...
> 
> In any case, it might make sense to detach this change from this series
> if we're making more significant changes in this area than just
> splitting the code into core and arch parts.
> 
> (Note also, your suggestion about indexing using rid may also work;
> I tend to assume that the mapping from rid to resource types may not be
> fixed, but maybe I'm being too strongly influenced by MPAM...)

Reinette

Hi Reinette, Dave,

On 18/04/2024 06:34, Reinette Chatre wrote:
> On 4/16/2024 9:16 AM, Dave Martin wrote:
>> On Mon, Apr 15, 2024 at 10:44:34AM -0700, Reinette Chatre wrote:
>>> On 4/12/2024 9:16 AM, Dave Martin wrote:
>>>> On Mon, Apr 08, 2024 at 08:14:47PM -0700, Reinette Chatre wrote:
>>>>> On 3/21/2024 9:50 AM, James Morse wrote:
>>>
>>>>>> @@ -195,6 +204,14 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>> +static ctrlval_parser_t *get_parser(struct rdt_resource *res)
>>>>>> +{
>>>>>> +	if (res->fflags & RFTYPE_RES_CACHE)
>>>>>> +		return &parse_cbm;
>>>>>> +	else
>>>>>> +		return &parse_bw;
>>>>>> +}
>>>>>
>>>>> This is borderline ... at minimum it expands what fflags means and how it
>>>>> is intended to be used and that needs to be documented because it reads:
>>>>>
>>>>> 	* @fflags:		flags to choose base and info files

Hmm, true this is used to day to select which groups of files appear.


>>>>> I am curious why you picked fflags instead of an explicit check against
>>>>> rid?

Simply because it would need to match both L2 and L3 to parse_cbm, I didn't think that
would scale if other cache resources get added. However, with an enum of types we can get
the compiler to bark if an entry is needed here, which is probably good enough.

more background: {
	In the arm world the cache hierarchy isn't something we can reason about. We
	have funny names for where different things converge, (Point of Coherency,
	Point of Unification etc), but its up to the platform designer if/where the
	L2/L3 or even L9 live. The cache topology is fed to the kernel via an ACPI
	table.

	I anticipate a 'System Cache' resource and schema eventually being added to
	resctrl as it looks to be a popular hardware design. These system-cache live
	after the L3 (if there is one).
}


>>>> Is fflags already somewhat overloaded?  There seem to be a mix of things
>>>> that are independent Boolean flags, while other things seem mutually
>>>> exclusive or enum-like.
>>>>
>>>> Do we expect RFTYPE_RES_CACHE | RFTYPE_RES_MB ever to make sense,
>>>> as David points out?
>>>>
>>>>
>>>> With MPAM, we could in theory have cache population control and egress
>>>> memory bandwidth controls on a single interconnect component.
>>>>
>>>> If that would always be represented through resctrl as two components
>>>> with the MB controls considered one level out from the CACHE controls,
>>>> then I guess these control types remain mutually exclusive from
>>>> resctrl's point of view.
>>>>
>>>> Allowing a single rdt_resource to sprout multiple control types looks
>>>> more invasive in the code, even if it logically makes sense in terms of
>>>> the hardware.

MPAM allows this, but it doesn't fit with resctrl. The MPAM drivers resctrl glue code has
a load of stuff to present these as separate resources to resctrl, even if they are the
same piece of hardware underneath.

So far it looks possible to hide this, I don't think its worth changing resctrl's
behaviour to try and cover this.

RFTYPE_RES_CACHE and RFTYPE_RES_MB would remain mutually-exclusive.


>>>> Anyway, for this patch, there seem to be a couple of assumptions:
>>>>
>>>> a) get_parser() doesn't get called except for rdt_resources that
>>>> represent resource controls (so, fflags = RFTYPE_RES_foo for some "foo",
>>>> with no other flags set), and
>>>>
>>>> b) there are exactly two kinds of "foo", so whatever isn't a CACHE is
>>>> a BW.
>>>>
>>>> These assumptions seem to hold today (?)
>>>
>>> (c) the parser for user provided data is based on the resource type.
>>>
>>> As I understand (c) may not be true for MPAM that supports different
>>> partitioning controls for a single resource. For example, for a cache
>>> MPAM supports portion as well as maximum capacity controls that
>>> I expect would need different parsers (perhaps mapping to different
>>> schemata entries?) from user space but will be used to control the
>>> same resource.

Exactly - to maintain compatibility with existing software the driver has to present it as
a totally new thing. I guess it will look something like this:
| L3:0=0xffff;1=0xffff;
| L3_CAP:0=1048576;1=;1048576

Where existing software knows about 'L3', and should ignore 'L3_CAP'.


>>> I do now know if the goal is to support this MPAM capability via
>>> resctrl but do accomplish this I wonder if it may not be more appropriate
>>> to associate the parser with the schema entry that is presented to user space.

Even better.

For Tony's resctrl2 I had mused on exposing to user-space whether the controls were a
bitmap/percentage/MBps-value/raw-number. As there is a parser for the first two (or three)
today I think keying these from something in the schemata makes the most sense.


>>>> But the semantics of fflags already look a bit complicated, so I can
>>>> see why it might be best to avoid anything that may add more
>>>> complexity.
>>>
>>> ack.
>>>
>>>> If the main aim is to avoid silly copy-paste errors when coding up
>>>> resources for a new arch, would it make sense to go for a more low-
>>>> tech approach and just bundle up related fields in a macro?
>>>
>>> I understand this as more than avoiding copy-paste errors. I understand
>>> the goal is to prevent architectures from having architecture specific
>>> parsers.

[...]

>>> You do highlight another point though, shouldn't the fs code own the
>>> format_str also? I do not think we want arch code to control the
>>> print format, this is also something that should be consistent between
>>> all archs and owned by fs code, again perhaps more appropriate for
>>> a schema entry.

Good point ... I've still got that as a "TODO: kill these properties off as they are
derivatives" in the MPAM code.

I agree they should live together. We can also pull in data_width too, as it is calculated
based on the format used here.

Moving default_ctrl is tricky as on AMD platforms the {S,}MBA default value is discovered
from cpuid. But it only makes sense for an architecture to provides this for MBps controls
- bitmaps and percentages have an obvious maximum/default value. Putting that in  struct
resctrl_membw as 'max_bw' makes bw_validate()s use of it clearer.

bw_validate() has always caught me out as it doesn't just parse percentages, but AMDs MBps
values. I don't think this needs changing, but having MBps as a control type will make
this less surprising.

Finally, core.c will end up keeping default_ctrl as an arch-specific thing as its
convenient for the init and reset code.

[...]

> What I was thinking about was something like below that uses the
> enum you introduce later and lets the RF flags stay internal to fs code:
> 
> rdtgroup_create_info_dir()
> {
> 
> 	...
> 	list_for_each_entry(s, &resctrl_schema_all, list) {
> 		r = s->res;
> 		if (r->res_type == RRESTYPE_CACHE)
> 			fflags = RFTYPE_RES_CACHE;
> 		else if (r->res_type == RRESTYPE_MB)
> 			fflags = RFTYPE_RES_MB;
> 		else /* fail */
> 		
> 		fflags |= RFTYPE_CTRL_INFO;
> 
> 		...
> 	}
> 	/* same idea for monitor info files */

Good point, that would let us remove fflags from the arch code too.


> For this patch the resource type can be used to initialize the schema
> entry.
> 
>>
>>
>> /* In include/linux/resctrl_types.h */
>>
>> +#define RFTYPE_RES			BIT(8)
>> -#define RFTYPE_RES_CACHE		BIT(8)
>> -#define RFTYPE_RES_MB			BIT(9)
> 
> The goal is to not have to expose any of the RFTYPE flags internals to
> the architecture. RFTYPE_RES_CACHE and RFTYPE_RES_MB stays, but is
> not exposed to arch code. I do not see need for RFTYPE_RES.
> All the RFTYPE flags can be defined in fs/resctrl/internal.h

Yup, these should stay in internal.h - they got swept up as there are #defines either side
that are needed for MPAM to build.


>> /* For RFTYPE_RES: */
>> enum resctrl_resource_type {
>> 	RRESTYPE_INVALID,
>> 	RRESTYPE_MB,
>> 	RRESTYPE_CACHE,
>> };
> 
> (I find naming hard ... note the names changed from the beginning of
> pseudo code to here where RESTYPE changing to RRESTYPE)

Before I saw this my attempt has:
| enum resctrl_schema_fmt {
| 	RESCTRL_SCHEMA_BITMAP,
| 	RESCTRL_SCHEMA_PERCENTAGE,
| 	RESCTRL_SCHEMA_MBPS,
| };

Invalid as value '0' would catch the arch code missing this - but means any switch over
this enum has to handle it... I'd prefer to leave that out so the compiler can bark about
any place that needs updating when a new control scheme is added.



Thanks,

James