On umount(), resctrl resets each resource back to its default
configuration. It only ever does this for all resources in one go.

reset_all_ctrls() is architecture specific as it works with struct
rdt_hw_resource.

Add an architecture helper to reset all resources.

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>
---
Changes since v1:
 * Rename the for_each_capable_rdt_resource() introduced in the new
   function resctrl_arch_reset_resources(), back to
   for_each_alloc_capable_rdt_resource() as it was in the original code.

   The change looked unintentional; and presumably a resource that does
   not support resource allocation doesn't have any properties to
   reset...
---
 arch/x86/include/asm/resctrl.h         |  2 ++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 +++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
index 52f2326e2b1e..5622943f6354 100644
--- a/arch/x86/include/asm/resctrl.h
+++ b/arch/x86/include/asm/resctrl.h
@@ -16,6 +16,8 @@
  */
 #define X86_RESCTRL_EMPTY_CLOSID         ((u32)~0)
 
+void resctrl_arch_reset_resources(void);
+
 /**
  * struct resctrl_pqr_state - State cache for the PQR MSR
  * @cur_rmid:		The cached Resource Monitoring ID
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 61c8add103fe..a15198f90b29 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2883,6 +2883,14 @@ static int reset_all_ctrls(struct rdt_resource *r)
 	return 0;
 }
 
+void resctrl_arch_reset_resources(void)
+{
+	struct rdt_resource *r;
+
+	for_each_alloc_capable_rdt_resource(r)
+		reset_all_ctrls(r);
+}
+
 /*
  * Move tasks from one to the other group. If @from is NULL, then all tasks
  * in the systems are moved unconditionally (used for teardown).
@@ -2992,16 +3000,14 @@ static void rmdir_all_sub(void)
 
 static void rdt_kill_sb(struct super_block *sb)
 {
-	struct rdt_resource *r;
-
 	cpus_read_lock();
 	mutex_lock(&rdtgroup_mutex);
 
 	rdt_disable_ctx();
 
-	/*Put everything back to default values. */
-	for_each_alloc_capable_rdt_resource(r)
-		reset_all_ctrls(r);
+	/* Put everything back to default values. */
+	resctrl_arch_reset_resources();
+
 	rmdir_all_sub();
 	rdt_pseudo_lock_release();
 	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
-- 
2.39.2

Hi James,

On 10/4/24 11:03 AM, James Morse wrote:
> On umount(), resctrl resets each resource back to its default
> configuration. It only ever does this for all resources in one go.
> 
> reset_all_ctrls() is architecture specific as it works with struct
> rdt_hw_resource.
> 
> Add an architecture helper to reset all resources.
> 
> 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>
> ---
> Changes since v1:
>  * Rename the for_each_capable_rdt_resource() introduced in the new
>    function resctrl_arch_reset_resources(), back to
>    for_each_alloc_capable_rdt_resource() as it was in the original code.
> 
>    The change looked unintentional; and presumably a resource that does
>    not support resource allocation doesn't have any properties to
>    reset...
> ---
>  arch/x86/include/asm/resctrl.h         |  2 ++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 +++++++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 52f2326e2b1e..5622943f6354 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -16,6 +16,8 @@
>   */
>  #define X86_RESCTRL_EMPTY_CLOSID         ((u32)~0)
>  
> +void resctrl_arch_reset_resources(void);
> +
>  /**
>   * struct resctrl_pqr_state - State cache for the PQR MSR
>   * @cur_rmid:		The cached Resource Monitoring ID
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 61c8add103fe..a15198f90b29 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2883,6 +2883,14 @@ static int reset_all_ctrls(struct rdt_resource *r)
>  	return 0;
>  }
>  
> +void resctrl_arch_reset_resources(void)
> +{
> +	struct rdt_resource *r;
> +
> +	for_each_alloc_capable_rdt_resource(r)
> +		reset_all_ctrls(r);
> +}

Wouldn't this require all archs to have a duplicate helper as above with
only the resctrl_all_ctrls() actually being arch specific? 
What if it is instead:
	resctrl_reset_alloc_resources() or reset_alloc_resources() or ...
	{
		struct rdt_resource *r;
	
		for_each_alloc_capable_rdt_resource(r)
			resctrl_arch_reset_all_ctrls(r);
	}

With above archs only need to implement the actual reset code.


> +
>  /*
>   * Move tasks from one to the other group. If @from is NULL, then all tasks
>   * in the systems are moved unconditionally (used for teardown).
> @@ -2992,16 +3000,14 @@ static void rmdir_all_sub(void)
>  
>  static void rdt_kill_sb(struct super_block *sb)
>  {
> -	struct rdt_resource *r;
> -
>  	cpus_read_lock();
>  	mutex_lock(&rdtgroup_mutex);
>  
>  	rdt_disable_ctx();
>  
> -	/*Put everything back to default values. */
> -	for_each_alloc_capable_rdt_resource(r)
> -		reset_all_ctrls(r);
> +	/* Put everything back to default values. */
> +	resctrl_arch_reset_resources();
> +
>  	rmdir_all_sub();
>  	rdt_pseudo_lock_release();
>  	rdtgroup_default.mode = RDT_MODE_SHAREABLE;

Reinette

Hi Reinette,

On 23/10/2024 22:32, Reinette Chatre wrote:
> On 10/4/24 11:03 AM, James Morse wrote:
>> On umount(), resctrl resets each resource back to its default
>> configuration. It only ever does this for all resources in one go.
>>
>> reset_all_ctrls() is architecture specific as it works with struct
>> rdt_hw_resource.
>>
>> Add an architecture helper to reset all resources.

>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 61c8add103fe..a15198f90b29 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2883,6 +2883,14 @@ static int reset_all_ctrls(struct rdt_resource *r)
>>  	return 0;
>>  }
>>  
>> +void resctrl_arch_reset_resources(void)
>> +{
>> +	struct rdt_resource *r;
>> +
>> +	for_each_alloc_capable_rdt_resource(r)
>> +		reset_all_ctrls(r);
>> +}

> Wouldn't this require all archs to have a duplicate helper as above with
> only the resctrl_all_ctrls() actually being arch specific? 

I was hoping to be able to save a few IPI by doing the per-core work once, instead of
per-resource-per-core ... but its only done on umount, so I doubt anyone will complain!


> What if it is instead:
> 	resctrl_reset_alloc_resources() or reset_alloc_resources() or ...
> 	{
> 		struct rdt_resource *r;
> 	
> 		for_each_alloc_capable_rdt_resource(r)
> 			resctrl_arch_reset_all_ctrls(r);
> 	}
> 
> With above archs only need to implement the actual reset code.

I opted for one helper that does everything as that is the only example today.
This has the advantage that the filesystem code can now reset a specific resource.

Sure, lets do that.


Thanks,

James

[0] https://lore.kernel.org/all/20230419111111.477118-8-dfustini@baylibre.com/

Hi James,

On 2/7/25 7:43 AM, James Morse wrote:
> Hi Reinette,
> 
> On 23/10/2024 22:32, Reinette Chatre wrote:
>> On 10/4/24 11:03 AM, James Morse wrote:
>>> On umount(), resctrl resets each resource back to its default
>>> configuration. It only ever does this for all resources in one go.
>>>
>>> reset_all_ctrls() is architecture specific as it works with struct
>>> rdt_hw_resource.
>>>
>>> Add an architecture helper to reset all resources.
> 
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 61c8add103fe..a15198f90b29 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -2883,6 +2883,14 @@ static int reset_all_ctrls(struct rdt_resource *r)
>>>  	return 0;
>>>  }
>>>  
>>> +void resctrl_arch_reset_resources(void)
>>> +{
>>> +	struct rdt_resource *r;
>>> +
>>> +	for_each_alloc_capable_rdt_resource(r)
>>> +		reset_all_ctrls(r);
>>> +}
> 
>> Wouldn't this require all archs to have a duplicate helper as above with
>> only the resctrl_all_ctrls() actually being arch specific? 
> 
> I was hoping to be able to save a few IPI by doing the per-core work once, instead of
> per-resource-per-core ... but its only done on umount, so I doubt anyone will complain!

This is very reasonable but that is not what the code does today and the
helper is added to today's code without providing insight into future optimizations. It
sounds as though MPAM was planning something differently (better) for which the helper in this
version would be appropriate and I expect that x86 could also benefit from that. I
understand that this is not a "fast" path but it raises the question of how optimizations
across archs should be handled. Ideally we should look across archs for ideal helpers
and not force one arch to adapt to what works for another. I am not advocating for a
change to what you submitted in v6 but instead would like to share that I think by not
having a discussion before switching to new helper there was a missed opportunity for
both archs.

Reinette