Re: [PATCH v3 03/38] x86/resctrl: Add a schema format enum and use this for fflags

[James Morse]

Hi Reinette,

On 01/07/2024 22:09, Reinette Chatre wrote:
> On 7/1/24 11:17 AM, James Morse wrote:
>> On 28/06/2024 17:43, Reinette Chatre wrote:
>>> On 6/14/24 7:59 AM, James Morse wrote:
>>>> resctrl has three types of control, these emerge from the way the
>>>> architecture initialises a number of properties in struct rdt_resource.
>>>>
>>>> A group of these properties need to be set the same on all architectures,
>>>> it would be better to specify the format the schema entry should use, and
>>>> allow resctrl to generate all the other properties it needs. This avoids
>>>> architectures having divergant behaviour here.
>>>
>>> divergant -> divergent ?
>>>
>>>>
>>>> Add a schema format enum, and as a first use, replace the fflags member
>>>> of struct rdt_resource.
>>>>
>>>> The MBA schema has a different format between AMD and Intel systems.
>>>> The schema_fmt property is changed by __rdt_get_mem_config_amd() to
>>>> enable the MBPS format.
>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index e3edc41882dc..b12307d465bc 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -2162,6 +2162,19 @@ static int rdtgroup_mkdir_info_resdir(void *priv, char *name,
>>>>        return ret;
>>>>    }
>>>>    +static u32 fflags_from_resource(struct rdt_resource *r)
>>>> +{
>>>> +    switch (r->schema_fmt) {
>>>> +    case RESCTRL_SCHEMA_BITMAP:
>>>> +        return RFTYPE_RES_CACHE;
>>>> +    case RESCTRL_SCHEMA_PERCENTAGE:
>>>> +    case RESCTRL_SCHEMA_MBPS:
>>>> +        return RFTYPE_RES_MB;
>>>> +    }
>>>> +
>>>> +    return WARN_ON_ONCE(1);
>>>> +}
>>>> +
>>>
>>> The fflags returned specifies which files will be associated with the resource
>>> in the "info" directory. Basing this on a property of the schema does not look
>>> right to me. I understand that many of the info files relate to, for example,
>>> information related to the bitmap used by the cache,
>>
>> Do we agree that some of them are?
>>
>> One reason for doing this is it decouples the parsing and management of bitmaps from "this
>> is the L3 cache", which will make it much easier to support bitmaps on some other kind of
>> resource.

> The way I see it is that it changes the meaning of the RFTYPE_RES_CACHE flag from "this is a
> file related to the cache resource" to "this is a file containing a bitmap property".
> It prevents us from easily adding a file related to the cache resource, which
> the info directory is intended to contain.

I struggled to find something that is a property of a "cache control", but is neither a
property of the control (e.g. bitmap size) or the cache. I guess the 'bit_usage' stuff is
the best example.

Maybe we end up with two sets of flags - this will be for the distant future. Currently I
taking your 'base fflags on resource id'.


>> Ultimately I'd like to expose these to user-space, so that user-space can work out how to
>> configure resources it doesn't recognise. Today '100' could be a percentage, a bitmap, or
>> a value in MB/s. Today some knowledge of the control type is needed to work this out.
>>
>>
>>> but that is not the same for
>>> info files related to the MBA resource (all info files related to MBA resource
>>> are not about the schema property format).
>>
>> Hmmm, because the files min_bandwidth and bandwidth_gran both have bandwidth in their name?
>>
>> I agree 'delay_linear' and 'thread_throttle_mode' are a bit strange.
> 
> Right. This is not a clean association.
> 
>>
>>
>>> I do not think the type of values of a schema should dictate which files
>>> appear in the info directory.
>>
>> Longer term I think this will be a problem. We probably only have 3 types of control:
>> percentage, bitmap and MB/s... but if each resource on each architecture adds files here
>> the list will quickly grow. User-space won't be able to work out how to configure a
>> resource type it hadn't seen before.
> 
> That is fair. This makes the type of control a property of the resource as is done in this
> series. Perhaps this can be exposed to user space via the info directory?

Yes, that is something I intend to look at. I eventually need to get MPAM's "cache
capacity" controls working as there are a number of hardware platforms that have it. This
would probably be a percentage control for 'L2' or 'L3', exposing an "info/schema_format"
file makes the most sense. I can't convert the existing bitmap as it implies isolation,
which this control format can't do, so it does need to be separate.
But! - to prevent confusing existing software, I don't think the L2/L3 should be touched -
those will forever have to be implicitly a bitmap, so anything in this area would have to
be an additional schema.


> Possibly the files related to control can have new flags that that reflect the control type
> instead of the resource. For example, "bit_usage" currently has
> "RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE" and that could be (for lack of better
> term) "RFTYPE_CTRL_INFO | RFTYPE_CTRL_BITMAP" to disconnect the control type from the
> resource. Doing so may then map nicely to the fflags_from_resource() in this patch that
> connects the schema format to the _control_ type flag. As we have found there is not
> a clear mapping between the control type and the resource type so I expect RFTYPE_RES_CACHE
> and RFTYPE_RES_MB to remain and be associated with files that contain information
> specific to that resource. This enables future additions of files containing cache specific
> (non-bitmap) properties to still be added (with RFTYPE_RES_CACHE flag) without impacting
> everything that uses a bitmap.
> 
> What do you think?

Makes sense!


Thanks,

James