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

Reinette Chatre
2024-04-18T05:35:10Z

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