Re: [PATCH v1 03/31] x86/resctrl: Move ctrlval string parsing policy away from the arch code

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

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