Re: [PATCH v5 19/40] x86/resctrl: Add resctrl_arch_is_evt_configurable() to abstract BMEC

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

Hi Reinette,

On 23/10/2024 23:04, Reinette Chatre wrote:
> On 10/4/24 11:03 AM, James Morse wrote:
>> When BMEC is supported the resctrl event can be configured in a number
>> of ways. This depends on architecture support. rdt_get_mon_l3_config()
>> modifies the struct mon_evt and calls mbm_config_rftype_init() to create
>> the files that allow the configuration.
>>
>> Splitting this into separate architecture and filesystem parts would
>> require the struct mon_evt and mbm_config_rftype_init() to be exposed.
>>
>> Instead, add resctrl_arch_is_evt_configurable(), and use this from
>> resctrl_mon_resource_init() to initialise struct mon_evt and call
>> mbm_config_rftype_init().
>> resctrl_arch_is_evt_configurable() calls rdt_cpu_has() so it doesn't
>> obviously benefit from being inlined. Putting it in core.c will allow
>> rdt_cpu_has() to eventually become static.
> 
> Why bother with rdt_cpu_has() when there are all those helpers available
> from previous patch?

It's what the existing code does... The helpers in the previous patch are about support
for an event/counter-type, e.g. whether mbm-total exists or not?
resctrl_arch_is_evt_configurable() is to check properties of a particular
event/counter-type, e.g. now you know mbm-total exists - can it be configured?
rdt_cpu_has() is how x86 determines this today.

The rdt_cpu_has() angle in the commit messages is that its a good thing if its all
contained in one file, and as its searching an array of of architecture specific
command-line options, its not going to be easy to inline resctrl_arch_is_evt_configurable().


Yes we could abuse rdt_mon_features to move these existing calls to something exposed as
bits in an unsigned long (and I'll be very glad its hidden behind helpers!) - but this is
done once when the filesystem is mounted, so it doesn't seem worth the churn.


Thanks,

James