Message-ID: <Zh6kRMkqVpu0Km4l@e133380.arm.com>
Date: Tue, 16 Apr 2024 17:16:04 +0100
From: Dave Martin <Dave.Martin@....com>
To: Reinette Chatre <reinette.chatre@...el.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

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

Powered by blists - more mailing lists