Re: [PATCH v2 00/24] x86/resctrl: Merge the CDP resources

[Babu Moger <babu.moger@amd.com>]

On 4/6/21 12:19 PM, James Morse wrote:
> Hi Babu,
> 
> On 30/03/2021 21:36, Babu Moger wrote:
>> On 3/12/21 11:58 AM, James Morse wrote:
>>> This series re-folds the resctrl code so the CDP resources (L3CODE et al)
>>> behaviour is all contained in the filesystem parts, with a minimum amount
>>> of arch specific code.
>>>
>>> Arm have some CPU support for dividing caches into portions, and
>>> applying bandwidth limits at various points in the SoC. The collective term
>>> for these features is MPAM: Memory Partitioning and Monitoring.
>>>
>>> MPAM is similar enough to Intel RDT, that it should use the defacto linux
>>> interface: resctrl. This filesystem currently lives under arch/x86, and is
>>> tightly coupled to the architecture.
>>> Ultimately, my plan is to split the existing resctrl code up to have an
>>> arch<->fs abstraction, then move all the bits out to fs/resctrl. From there
>>> MPAM can be wired up.
>>>
>>> x86 might have two resources with cache controls, (L2 and L3) but has
>>> extra copies for CDP: L{2,3}{CODE,DATA}, which are marked as enabled
>>> if CDP is enabled for the corresponding cache.
>>>
>>> MPAM has an equivalent feature to CDP, but its a property of the CPU,
>>> not the cache. Resctrl needs to have x86's odd/even behaviour, as that
>>> its the ABI, but this isn't how the MPAM hardware works. It is entirely
>>> possible that an in-kernel user of MPAM would not be using CDP, whereas
>>> resctrl is.
>>> Pretending L3CODE and L3DATA are entirely separate resources is a neat
>>> trick, but doing this is specific to x86.
>>> Doing this leaves the arch code in control of various parts of the
>>> filesystem ABI: the resources names, and the way the schemata are parsed.
>>> Allowing this stuff to vary between architectures is bad for user space.
>>>
>>> This series collapses the CODE/DATA resources, moving all the user-visible
>>> resctrl ABI into what becomes the filesystem code. CDP becomes the type of
>>> configuration being applied to a cache. This is done by adding a
>>> struct resctrl_schema to the parts of resctrl that will move to fs. This
>>> holds the arch-code resource that is in use for this schema, along with
>>> other properties like the name, and whether the configuration being applied
>>> is CODE/DATA/BOTH.
> 
> 
>> I applied your patches on my AMD box.
> 
> Great! Thanks for taking a look,
> 
> 
>> Seeing some difference in the behavior.
> 
> Ooer,
> 
> 
>> Before these patches.
>>
>> # dmesg |grep -i resctrl
>> [   13.076973] resctrl: L3 allocation detected
>> [   13.087835] resctrl: L3DATA allocation detected
>> [   13.092886] resctrl: L3CODE allocation detected
>> [   13.097936] resctrl: MB allocation detected
>> [   13.102599] resctrl: L3 monitoring detected
>>
>>
>> After the patches.
>>
>> # dmesg |grep -i resctrl
>> [   13.076973] resctrl: L3 allocation detected
>> [   13.097936] resctrl: MB allocation detected
>> [   13.102599] resctrl: L3 monitoring detected
>>
>> You can see that L3DATA and L3CODE disappeared. I think we should keep the
>> behavior same for x86(at least).
> 
> This is the kernel log ... what user-space software is parsing that for an expected value?
> What happens if the resctrl strings have been overwritten by more kernel log?
> 
> I don't think user-space should be relying on this. I'd argue any user-space doing this is
> already broken. Is it just the kernel selftest's filter_dmesg()? It doesn't seem to do
> anything useful
> 
> Whether resctrl is support can be read from /proc/filesystems. CDP is probably a
> try-it-and-see. User-space could parse /proc/cpuinfo, but its probably not a good idea.

Yes. Agree. Looking at the dmesg may no be right way to figure out all the
support details. As a normal practice, I searched for these texts and
noticed difference. That is why I felt it is best to keep those texts same
as before.
> 
> 
> Its easy to fix, but it seems odd that the kernel has to print things for user-space to
> try and parse. (I'd like to point at the user-space software that depends on this)

I dont think there is any software that parses the dmesg for these
details. These are info messages for the developers.

> 
> 
>> I am still not clear why we needed resctrl_conf_type
>>
>> enum resctrl_conf_type {
>>         CDP_BOTH,
>>         CDP_CODE,
>>         CDP_DATA,
>> };
>>
>> Right now, I see all the resources are initialized as CDP_BOTH.
>>
>>  [RDT_RESOURCE_L3] =
>>         {
>>                 .conf_type                      = CDP_BOTH,
>>  [RDT_RESOURCE_L2] =
>>         {
>>                 .conf_type                      = CDP_BOTH,
>>  [RDT_RESOURCE_MBA] =
>>         {
>>                 .conf_type                      = CDP_BOTH,
> 
> Ah, those should have been removed in patch 24. Once all the resources are the same, the
> resource doesn't need to describe what kind it is.
> 
> 
>> If all the resources are CDP_BOTH, then why we need separate CDP_CODE and
>> CDP_DATA?
> 
> The filesystem code for resctrl that will eventually move out of arch/x86 needs to be able
> to describe the type of configuration change being made back to the arch code. The enum
> gets used for that.
> 
> x86 needs this as it affects which MSRs the configuration value is written to.
> 
> 
>> Are these going to be different for ARM?
> 
> Nope. Arm's MPAM ends up emulating CDP with the closid values that get applied to
> transactions.
> 
> 
>> Also initializing RDT_RESOURCE_MBA as CDP_BOTH does not seem right. I dont
>> think there will CDP support in MBA in future.
> 
> Its not code or data, which makes it both. 'BOTH' is more of a 'do nothing special', there
> may be a better name, but I'm not very good at naming things. (any suggestions?)

Do you think CDP_NONE will make some sense?
> 
> 
> Thanks,
> 
> James
>