Messages in this thread | | | Date | Wed, 17 Apr 2024 22:34:58 -0700 | Subject | Re: [PATCH v1 03/31] x86/resctrl: Move ctrlval string parsing policy away from the arch code | From | Reinette Chatre <> |
| |
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
| |