Re: [PATCH v6 29/42] x86/resctrl: Move get_config_index() to a header

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

Hi Reinette,

On 20/02/2025 01:27, Reinette Chatre wrote:
> On 2/7/25 10:18 AM, James Morse wrote:
>> get_config_index() is used by the architecture specific code to map a
>> CLOSID+type pair to an index in the configuration arrays.
>>
>> MPAM needs to do this too to preserve the ABI to user-space, there is
>> no reason to do it differently.
>>
>> Move the helper to a header file.

>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -384,6 +384,21 @@ void resctrl_arch_mon_event_config_write(void *config_info);
>>   */
>>  void resctrl_arch_mon_event_config_read(void *config_info);
>>  
>> +/* For use by arch code to remap resctrl's smaller CDP CLOSID range */
>> +static inline u32 resctrl_get_config_index(u32 closid,
>> +					   enum resctrl_conf_type type)
>> +{
>> +	switch (type) {
>> +	default:
>> +	case CDP_NONE:
>> +		return closid;
>> +	case CDP_CODE:
>> +		return closid * 2 + 1;
>> +	case CDP_DATA:
>> +		return closid * 2;
>> +	}
>> +}
>> +

> Could you please add the motivation for the use of an inline function?

Putting this in the header file means it isn't duplicated, so its behaviour can't become
different. If its in a header file, it has to be marked inline otherwise every C file that
includes it gets a copy that probably isn't used, and upsets the linker.

Calling from the arch code into the filesystem prevents the arch code from being
standalone. This is a useful direction of travel because it allows fs/resctrl to one
day become a module

Today, the compiler is choosing to inline this:
| x86_64-linux-objdump -d ctrlmondata.o | grep resctrl_get_config_index | wc -l
| 0

This kind of arithmetic for an array lookup is the kind of thing its good to give the
compiler full visibility of as its good fodder for constant folding.

For so few call sites, I don't think this is really worth thinking about.
Forcing this call out of line makes the kernel text bigger, but only by 32 bytes.


I've expanded the last paragraph of the commit message to read:
| Move the helper to a header file to allow all architectures that either
| use or emulate CDP to use the same pattern of CLOSID values. Moving
| this to a header file means it must be marked inline, which matches
| the existing compiler choice for this static function.


Thanks,

James