Re: [PATCH v5 18/40] x86/resctrl: Export the is_mbm_*_enabled() helpers to asm/resctrl.h

[James Morse]

On 23/10/2024 23:00, Reinette Chatre wrote:
> Hi James,
> 
> On 10/4/24 11:03 AM, James Morse wrote:
>> The architecture specific parts of resctrl have helpers to hide accesses
>> to the rdt_mon_features bitmap.
> 
> hmmmm ... no ... this patch creates those helpers.

is_mbm_total_enabled() and is_mbm_local_enabled() from the subject were added
by commit 9f52425ba303 ("x86/intel_rdt/mbm: Basic counting of MBM events (total and
local"), way back in 2017.

I'll add some of the helper names to this paragraph, but I think a list impedes readability.


>> Once the filesystem parts of resctrl are moved, these can no longer live
>> in internal.h. Once these are exposed to the wider kernel, they should
>> have a 'resctrl_arch_' prefix, to fit the rest of the arch<->fs interface.
>>
>> Move and rename the helpers that touch rdt_mon_features directly.
>> is_mbm_event() and is_mbm_enabled() are only called from rdtgroup.c,
>> so can be moved into that file.
> 
> There seems to be a contradiction here ... earlier patch moved the
> event IDs to common header so this makes these events shared between
> resctrl and all archs.

Unique identifiers were needed for the events that are shared by all architectures - using
the x86 hardware values is simple enough, and benefits the x86 architecture code. It was
an easy choice because today they are 1,2,3 ...


> rdt_mon_features bitmap positions are
> the common event IDs. Why should rdt_mon_features thus be considered arch
> specific if bits that can be set are not?

The values are passed into the helper, its up to the architecture code what it does with
them. For example, MPAM currently uses these to check pointers in an array, but once it
exposes events that resctrl doesn't offer to user-space, it will need to do more pointer
chasing.

I don't think its a good idea to require data values to be exposed between the
architecture and filesystem code. It's simple today, but having to maintain a shared
bitmap of event types across architectures sounds like a headache.
Helpers like this have a much clearer and closely defined behaviour, and are much harder
to abuse. When one architecture needs something different, its free to do so. If one
architecture wants to expose something like rdt_mon_features and test the bits - all that
can be inlined in to the caller.

(Currently the realloc-threshold is the only data value exposed because it would have been
more churn to abstract it)


> The patch may be ok if MPAM wants to do something different here but
> motivating it as "this is arch specific and needs to be hidden by helpers"
> is a stretch since there is nothing arch specific about it.

My view will be coloured because at one point I did have helpers to remap 'resctrl event
enum' numbers back to x86's hardware counters. The cunning plan was for the compiler to
optimise it out - unless it proved impossible - which the compiler could work out.
But I figured it would be simpler to get rid of it and use the enum values directly. (the
actual values don't matter to MPAM - as long as the enum isn't too big).

I'll reword this to cover why exposing helpers instead of an unsigned-long is preferable.


Thanks,

James