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

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

Hi Babu,

On 06/04/2021 22:37, Babu Moger wrote:
> On 4/6/21 12:19 PM, James Morse wrote:
>> 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.

>>>> 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.

The kernel log changes all the time, messages at boot aren't something you can depend on
seeing later. Unless there is some user-space software broken by this, I'm afraid I don't
think its a good idea to add extra code to keep it the same.

Printing 'CDP supported by Lx' would be more useful to developers perusing the log. Even
more useful would be exposing feature attributes via sysfs to say what resctrl supports
without having to mount-it-and-see. This can then be used by user-space too.
e.g.:
| cat /sys/fs/ext4/features/fast_commit


>>> 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?

If you think that is clearer, sure.


Thanks,

James