Re: [PATCH 06/24] x86/resctrl: Store the effective num_closid in the schema

[James Morse <james.morse@arm.com>]

Hi Reinette,

On 17/11/2020 22:04, Reinette Chatre wrote:
> On 10/30/2020 9:11 AM, James Morse wrote:
>> resctrl_schema holds properties that vary with the style of configuration
>> that resctrl applies to a resource.
>>
>> Once the arch code has a single resource per cache that can be configured,
>> resctrl will need to keep track of the num_closid itself.
>>
>> Add num_closid to resctrl_schema. Change callers like
>> rdtgroup_schemata_show() to walk the schema instead.

> This is a significant patch in that it introduces a second num_closid available for code
> to use. Even so, the commit message is treating it quite nonchalantly ... essentially
> stating that "here is a new closid and change some code to use it".

The difference already exists, the number of closid that resctrl exposes to user-space may
be different from what the hardware supports. Currently the arch code does a
bait-and-switch with the L3CODE/L3DATA resources, but this is specific to x86, it
shouldn't be required for another architecture to copy it if its not necessary to support CDP.

I'll expand the commit message with this.

An earlier version tried to use different types for the 'hw' number of closid, and the
version used by resctrl, but I figured it was too noisy.


> Could you please elaborate how the callers needing to "walk the schema instead" were chosen?

These all want the num_closid value that is exposed to user-space:
rdtgroup_parse_resource()
rdtgroup_schemata_show()
rdt_num_closids_show()
closid_init()

If resctrl is in control of that, it should come from the schema instead of being pulled
straight out of the architecture code.


> This seems almost a revert of the earlier patch that introduced the helper and I wonder if
> it may not make this easier to understand if these areas do not receive the temporary
> change to use that helper.

Its a trade-off between churn for a self contained change (i.e. all the 'fs' bits,
regardless of if they are around later), and keeping the patches that are to do with the
schema together.

I don't think its a good idea to have "these are left alone as they will be changed
differently later" as that is liable to break (or just get weird) as the series is
restructured to fix it.

It will probably be fewer lines of change to do it the other way round. v2 does this,
hopefully that is easier on the eye!


Thanks,

James