x86 has an array, rdt_resources_all[], of all possible resources.
The for-each-resource walkers depend on the rid field of all
resources being initialised.

If the array ever grows due to another architecture adding a resource
type that is not defined on x86, the for-each-resources walkers will
loop forever.

Initialise all the rid values in resctrl_arch_late_init() before
any for-each-resource walker can be called.

Signed-off-by: James Morse <james.morse@arm.com>

---
Changes since v7:
 * Split out of a previous patch due to a botched merged conflict.
---
 arch/x86/kernel/cpu/resctrl/core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 58d7c6accdf2..ce684da600bc 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -60,7 +60,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
 	[RDT_RESOURCE_L3] =
 	{
 		.r_resctrl = {
-			.rid			= RDT_RESOURCE_L3,
 			.name			= "L3",
 			.ctrl_scope		= RESCTRL_L3_CACHE,
 			.mon_scope		= RESCTRL_L3_CACHE,
@@ -74,7 +73,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
 	[RDT_RESOURCE_L2] =
 	{
 		.r_resctrl = {
-			.rid			= RDT_RESOURCE_L2,
 			.name			= "L2",
 			.ctrl_scope		= RESCTRL_L2_CACHE,
 			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_L2),
@@ -86,7 +84,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
 	[RDT_RESOURCE_MBA] =
 	{
 		.r_resctrl = {
-			.rid			= RDT_RESOURCE_MBA,
 			.name			= "MB",
 			.ctrl_scope		= RESCTRL_L3_CACHE,
 			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_MBA),
@@ -96,7 +93,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
 	[RDT_RESOURCE_SMBA] =
 	{
 		.r_resctrl = {
-			.rid			= RDT_RESOURCE_SMBA,
 			.name			= "SMBA",
 			.ctrl_scope		= RESCTRL_L3_CACHE,
 			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_SMBA),
@@ -996,7 +992,11 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)
 static int __init resctrl_arch_late_init(void)
 {
 	struct rdt_resource *r;
-	int state, ret;
+	int state, ret, i;
+
+	/* Initialise all rid values for_each_rdt_resource() */
+	for (i = 0; i < RDT_NUM_RESOURCES; i++)
+		rdt_resources_all[i].r_resctrl.rid = i;
 
 	/*
 	 * Initialize functions(or definitions) that are different
-- 
2.20.1

Hi James,

On 4/11/25 9:42 AM, James Morse wrote:

> @@ -996,7 +992,11 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)
>  static int __init resctrl_arch_late_init(void)
>  {
>  	struct rdt_resource *r;
> -	int state, ret;
> +	int state, ret, i;
> +
> +	/* Initialise all rid values for_each_rdt_resource() */

I find the above difficult to parse. How about:
	/* for_each_rdt_resource() requires all rid to be initialised. */

> +	for (i = 0; i < RDT_NUM_RESOURCES; i++)
> +		rdt_resources_all[i].r_resctrl.rid = i;
>  
>  	/*
>  	 * Initialize functions(or definitions) that are different

Reinette

Hi Reinette,

On 16/04/2025 03:14, Reinette Chatre wrote:
> On 4/11/25 9:42 AM, James Morse wrote:
> 
>> @@ -996,7 +992,11 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)
>>  static int __init resctrl_arch_late_init(void)
>>  {
>>  	struct rdt_resource *r;
>> -	int state, ret;
>> +	int state, ret, i;
>> +
>> +	/* Initialise all rid values for_each_rdt_resource() */
> 
> I find the above difficult to parse. How about:
> 	/* for_each_rdt_resource() requires all rid to be initialised. */
> 
>> +	for (i = 0; i < RDT_NUM_RESOURCES; i++)
>> +		rdt_resources_all[i].r_resctrl.rid = i;
>>  
>>  	/*
>>  	 * Initialize functions(or definitions) that are different

Thanks that's better. I think I removed a 'for' as 'for for_each...' looked like
a duplicate word...


Thanks,

James

On Fri, Apr 11, 2025 at 04:42:24PM +0000, James Morse wrote:
> x86 has an array, rdt_resources_all[], of all possible resources.
> The for-each-resource walkers depend on the rid field of all
> resources being initialised.
> 
> If the array ever grows due to another architecture adding a resource
> type that is not defined on x86, the for-each-resources walkers will
> loop forever.

This feels a bit weird. Having rdt_resources_all[] be a "swiss cheese"
array full of holes where other architectures defined events that aren't
supported by x86.

But it does work, so it can go in like this. But someday I may revisit
some experimental patches I did a while back that:
1) Split the rdt_resource structure into separate "ctrl" and "mon"
pieces.
2) Replaced this array with a pair of lists, one each for enabled
ctrl and mon resources.
3) Changed the resource walkers to use list_for_each*() macros.

> 
> Initialise all the rid values in resctrl_arch_late_init() before
> any for-each-resource walker can be called.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> 
> ---
> Changes since v7:
>  * Split out of a previous patch due to a botched merged conflict.
> ---
>  arch/x86/kernel/cpu/resctrl/core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 58d7c6accdf2..ce684da600bc 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -60,7 +60,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
>  	[RDT_RESOURCE_L3] =
>  	{
>  		.r_resctrl = {
> -			.rid			= RDT_RESOURCE_L3,
>  			.name			= "L3",
>  			.ctrl_scope		= RESCTRL_L3_CACHE,
>  			.mon_scope		= RESCTRL_L3_CACHE,
> @@ -74,7 +73,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
>  	[RDT_RESOURCE_L2] =
>  	{
>  		.r_resctrl = {
> -			.rid			= RDT_RESOURCE_L2,
>  			.name			= "L2",
>  			.ctrl_scope		= RESCTRL_L2_CACHE,
>  			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_L2),
> @@ -86,7 +84,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
>  	[RDT_RESOURCE_MBA] =
>  	{
>  		.r_resctrl = {
> -			.rid			= RDT_RESOURCE_MBA,
>  			.name			= "MB",
>  			.ctrl_scope		= RESCTRL_L3_CACHE,
>  			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_MBA),
> @@ -96,7 +93,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
>  	[RDT_RESOURCE_SMBA] =
>  	{
>  		.r_resctrl = {
> -			.rid			= RDT_RESOURCE_SMBA,
>  			.name			= "SMBA",
>  			.ctrl_scope		= RESCTRL_L3_CACHE,
>  			.ctrl_domains		= ctrl_domain_init(RDT_RESOURCE_SMBA),
> @@ -996,7 +992,11 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)
>  static int __init resctrl_arch_late_init(void)
>  {
>  	struct rdt_resource *r;
> -	int state, ret;
> +	int state, ret, i;
> +
> +	/* Initialise all rid values for_each_rdt_resource() */
> +	for (i = 0; i < RDT_NUM_RESOURCES; i++)
> +		rdt_resources_all[i].r_resctrl.rid = i;
>  
>  	/*
>  	 * Initialize functions(or definitions) that are different
> -- 
> 2.20.1

-Tony

Hi Tony,

On 15/04/2025 20:08, Luck, Tony wrote:
> On Fri, Apr 11, 2025 at 04:42:24PM +0000, James Morse wrote:
>> x86 has an array, rdt_resources_all[], of all possible resources.
>> The for-each-resource walkers depend on the rid field of all
>> resources being initialised.
>>
>> If the array ever grows due to another architecture adding a resource
>> type that is not defined on x86, the for-each-resources walkers will
>> loop forever.

> This feels a bit weird. Having rdt_resources_all[] be a "swiss cheese"
> array full of holes where other architectures defined events that aren't
> supported by x86.

Today, there are none of those for x86. The MPAM driver has to do this, but so far the
array is small. I agree if the array becomes large, and no architecture implements
everything then some other structure would be better.


> But it does work, so it can go in like this. But someday I may revisit
> some experimental patches I did a while back that:
> 1) Split the rdt_resource structure into separate "ctrl" and "mon"
> pieces.
> 2) Replaced this array with a pair of lists, one each for enabled
> ctrl and mon resources.
> 3) Changed the resource walkers to use list_for_each*() macros.

Sounds good - this matches what the schema list has become.
The only oddity is the newly proposed "resctrl_online_domains_exist()" that would
sanity-check a resctrl_exit() call - but I don't see a problem walking one list after the
other there.


Thanks,

James