Re: [PATCH v5 17/40] x86/resctrl: Rewrite and move the for_each_*_rdt_resource() walkers

[Reinette Chatre]

Hi Tony,

On 10/7/24 5:00 PM, Tony Luck wrote:
> On Fri, Oct 04, 2024 at 06:03:24PM +0000, James Morse wrote:
>> The for_each_*_rdt_resource() helpers walk the architecture's array
>> of structures, using the resctrl visible part as an iterator. These
>> became over-complex when the structures were split into a
>> filesystem and architecture-specific struct. This approach avoided
>> the need to touch every call site, and was done before there was a
>> helper to retrieve a resource by rid.
>>
>> Once the filesystem parts of resctrl are moved to /fs/, both the
>> architecture's resource array, and the definition of those structures
>> is no longer accessible. To support resctrl, each architecture would
>> have to provide equally complex macros.
>>
>> Rewrite the macro to make use of resctrl_arch_get_resource(), and
>> move these to the core header so existing x86 arch code continues
>> to use them.
> 
> Apologies if this comment was suggested against earlier versions
> of this series.
> 
> Did you consider replacing rdt_resources_all[] a list (in the filesystem
> code) instead of an array (in the architecture code)?
> 
> List would start empty. Architecture init code would enumerate features
> and add entries to the list for those that exist and are to be enabled.
> 
> The "for_each" macros then walk the list (variants for all entries,
> for "alloc_capable" and for "mon_capable"). Note that only enabled
> entries appear on the lists.
> 
> There are currently a bunch of places in filesystem code that
> do:
> 	r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
> or
> 	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> 
> those could become:
> 
> 	r = resctrl_arch_get_mba_resource();
> 
> 	r = resctrl_arch_get_l3_resource();
> 
> Then the whole "enum resctrl_res_level" and ->rid field in
> struct rdt_resource could go away? Remaining uses look like
> distinguishing MBA from SMBA. Perhaps better done with a
> flags word?
> 
> Advantage of doing this would be to avoid the generic
> enum resctrl_res_level having to be a superset of all
> features across all architectures. E.g. ARM might want
> to add L4/L5 resources, X86 may have some that ARM will
> never need. RiscV may also follow some divergent path.

Ideally resctrl fs would remain as an interface that a user can use to interact
with all architectures without knowing architecture specific details. Platform
differences can be exposed by resctrl in a generic way to support this.
I am afraid that allowing architectures to diverge would require resctrl fs users
to additionally know which platform they are running on.

> If this v5 series is close to being applied then I don't
> want to derail with a re-write at this late stage.
> All of this could be done as a cleanup after this series
> has been applied.

Due to the already significant size of this work I think it would make it easier
if the number of functional changes are minimal. Specifically, only those functional
changes that are required to accomplish the goal of moving the code.

Considering that one goal of this proposal is to support architectural
flexibility I do think it would be easier to understand its impact if it
is implemented on top of the arch/fs split.

Reinette