Received-SPF: pass (google.com: domain of linux-kernel+bounces-143041-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 12 Apr 2024 17:16:39 +0100
From: Dave Martin <Dave.Martin@arm.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: James Morse <james.morse@arm.com>, x86@kernel.org,
	linux-kernel@vger.kernel.org, Fenghua Yu <fenghua.yu@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	H Peter Anvin <hpa@zytor.com>, Babu Moger <Babu.Moger@amd.com>,
	shameerali.kolothum.thodi@huawei.com,
	D Scott Phillips OS <scott@os.amperecomputing.com>,
	carl@os.amperecomputing.com, lcherian@marvell.com,
	bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com,
	baolin.wang@linux.alibaba.com, Jamie Iles <quic_jiles@quicinc.com>,
	Xin Hao <xhao@linux.alibaba.com>, peternewman@google.com,
	dfustini@baylibre.com, amitsinght@marvell.com,
	David Hildenbrand <david@redhat.com>,
	Rex Nie <rex.nie@jaguarmicro.com>
Subject: Re: [PATCH v1 03/31] x86/resctrl: Move ctrlval string parsing policy
 away from the arch code
Message-ID: <ZhleZ2q60ysIRapv@e133380.arm.com>
References: <20240321165106.31602-1-james.morse@arm.com>
 <20240321165106.31602-4-james.morse@arm.com>
 <e85e7786-7995-42d5-a5ac-1e08a84492fe@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e85e7786-7995-42d5-a5ac-1e08a84492fe@intel.com>

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