Date: Mon, 15 Apr 2024 10:44:34 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Dave Martin <Dave.Martin@....com>
CC: James Morse <james.morse@....com>, <x86@...nel.org>,
	<linux-kernel@...r.kernel.org>, 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>
Subject: Re: [PATCH v1 03/31] x86/resctrl: Move ctrlval string parsing policy
 away from the arch code

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

Powered by blists - more mailing lists