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

James Morse posted 42 patches 1 year ago
There is a newer version of this series
[PATCH v6 29/42] x86/resctrl: Move get_config_index() to a header
Posted by James Morse 1 year ago
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.

Co-developed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: James Morse <james.morse@arm.com>
Tested-by: Carl Worth <carl@os.amperecomputing.com> # arm64
Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
Changes since v1:
 * Reindent resctrl_get_config_index() as per coding-style.rst rules.
---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 19 +++----------------
 include/linux/resctrl.h                   | 15 +++++++++++++++
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index a93b40ea0bad..032a585293af 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -287,25 +287,12 @@ static int parse_line(char *line, struct resctrl_schema *s,
 	return -EINVAL;
 }
 
-static u32 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;
-	}
-}
-
 int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 			    u32 closid, enum resctrl_conf_type t, u32 cfg_val)
 {
 	struct rdt_hw_ctrl_domain *hw_dom = resctrl_to_arch_ctrl_dom(d);
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
-	u32 idx = get_config_index(closid, t);
+	u32 idx = resctrl_get_config_index(closid, t);
 	struct msr_param msr_param;
 
 	if (!cpumask_test_cpu(smp_processor_id(), &d->hdr.cpu_mask))
@@ -342,7 +329,7 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
 			if (!cfg->have_new_ctrl)
 				continue;
 
-			idx = get_config_index(closid, t);
+			idx = resctrl_get_config_index(closid, t);
 			if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
 				continue;
 			hw_dom->ctrl_val[idx] = cfg->new_ctrl;
@@ -462,7 +449,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 			    u32 closid, enum resctrl_conf_type type)
 {
 	struct rdt_hw_ctrl_domain *hw_dom = resctrl_to_arch_ctrl_dom(d);
-	u32 idx = get_config_index(closid, type);
+	u32 idx = resctrl_get_config_index(closid, type);
 
 	return hw_dom->ctrl_val[idx];
 }
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 524f35b5532b..29415d023aab 100644
--- 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;
+	}
+}
+
 /*
  * Update the ctrl_val and apply this config right now.
  * Must be called on one of the domain's CPUs.
-- 
2.39.2
Re: [PATCH v6 29/42] x86/resctrl: Move get_config_index() to a header
Posted by Reinette Chatre 11 months, 3 weeks ago
Hi James,

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.
> 
> Co-developed-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> Tested-by: Carl Worth <carl@os.amperecomputing.com> # arm64
> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
> Changes since v1:
>  * Reindent resctrl_get_config_index() as per coding-style.rst rules.
> ---
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 19 +++----------------
>  include/linux/resctrl.h                   | 15 +++++++++++++++
>  2 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index a93b40ea0bad..032a585293af 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -287,25 +287,12 @@ static int parse_line(char *line, struct resctrl_schema *s,
>  	return -EINVAL;
>  }
>  
> -static u32 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;
> -	}
> -}
> -

...

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

Reinette
Re: [PATCH v6 29/42] x86/resctrl: Move get_config_index() to a header
Posted by James Morse 11 months, 2 weeks ago
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
Re: [PATCH v6 29/42] x86/resctrl: Move get_config_index() to a header
Posted by Reinette Chatre 11 months, 2 weeks ago
Hi James,

On 2/28/25 11:51 AM, James Morse wrote:
> 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

I am not following this. How would making this part of a .c file of fs/resctrl with just
the prototype in include/linux/resctrl.h result in this function being duplicated?

> 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

Don't we have this already with all the needed CPU and domain management (
resctrl_online_ctrl_domain(), resctrl_online_mon_domain(), resctrl_online_cpu(),
resctrl_offline_cpu(), etc.)?

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

Reinette
Re: [PATCH v6 29/42] x86/resctrl: Move get_config_index() to a header
Posted by James Morse 11 months, 1 week ago
Hi Reinette,

On 01/03/2025 02:28, Reinette Chatre wrote:
> On 2/28/25 11:51 AM, James Morse wrote:
>> 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
> 
> I am not following this. How would making this part of a .c file of fs/resctrl with just
> the prototype in include/linux/resctrl.h result in this function being duplicated?

Ah, I misread this as one of the functions marked resctrl_arch_.


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

> Don't we have this already with all the needed CPU and domain management (
> resctrl_online_ctrl_domain(), resctrl_online_mon_domain(), resctrl_online_cpu(),
> resctrl_offline_cpu(), etc.)?

And the realloc threshold, yes. These are the things that would need further abstraction
to allow the filesystem to be a module that isn't loaded. But these would all be changes
to the existing behaviour.
This one is just putting the definition in a header.


>> 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
Re: [PATCH v6 29/42] x86/resctrl: Move get_config_index() to a header
Posted by Reinette Chatre 11 months, 1 week ago
Hi James,

On 3/6/25 11:28 AM, James Morse wrote:
> Hi Reinette,
> 
> On 01/03/2025 02:28, Reinette Chatre wrote:
>> On 2/28/25 11:51 AM, James Morse wrote:
>>> 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
>>
>> I am not following this. How would making this part of a .c file of fs/resctrl with just
>> the prototype in include/linux/resctrl.h result in this function being duplicated?
> 
> Ah, I misread this as one of the functions marked resctrl_arch_.
> 
> 
>>> 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
> 
>> Don't we have this already with all the needed CPU and domain management (
>> resctrl_online_ctrl_domain(), resctrl_online_mon_domain(), resctrl_online_cpu(),
>> resctrl_offline_cpu(), etc.)?
> 
> And the realloc threshold, yes. These are the things that would need further abstraction
> to allow the filesystem to be a module that isn't loaded. But these would all be changes
> to the existing behaviour.
> This one is just putting the definition in a header.

hmmm ... this seems a stretch as an argument since filesystem is not currently a module
and cannot currently be a module. Adding a declaration to a header file that matches with
existing usage seems reasonable to me.

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

This is fair and seems to be only valid reason.

Reinette