The for_each_*_rdt_resource() helpers walk the architecture's array
of structures, using the resctrl visible part as an iterator. These
became over-complex when the structures were split into a
filesystem and architecture-specific struct. This approach avoided
the need to touch every call site.

Once the filesystem parts of resctrl are moved to /fs/, both the
architecture's resource array, and the definition of those structures
is no longer accessible. To support resctrl, each architecture would
have to provide equally complex macros.

Change the resctrl code that uses these to walk through the resource_level
enum and check the mon/alloc capable flags instead. Instances in core.c,
and resctrl_arch_reset_resources() remain part of x86's architecture
specific code.

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: Peter Newman <peternewman@google.com>
Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
Changes since v1:
 * [Whitespace only] Fix bogus whitespace introduced in
   rdtgroup_create_info_dir().

 * [Commit message only] Typo fix:
   s/architectures/architecture's/g
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  7 +++++-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 28 +++++++++++++++++++----
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index aacf236dfe3b..ad20822bb64e 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -840,6 +840,7 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm
 bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
 {
 	cpumask_var_t cpu_with_psl;
+	enum resctrl_res_level i;
 	struct rdt_resource *r;
 	struct rdt_domain *d_i;
 	bool ret = false;
@@ -854,7 +855,11 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
 	 * First determine which cpus have pseudo-locked regions
 	 * associated with them.
 	 */
-	for_each_alloc_capable_rdt_resource(r) {
+	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
+		r = resctrl_arch_get_resource(i);
+		if (!r->alloc_capable)
+			continue;
+
 		list_for_each_entry(d_i, &r->domains, list) {
 			if (d_i->plr)
 				cpumask_or(cpu_with_psl, cpu_with_psl,
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 9c03e973a5f6..d9513d7b5157 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -98,12 +98,17 @@ void rdt_last_cmd_printf(const char *fmt, ...)
 
 void rdt_staged_configs_clear(void)
 {
+	enum resctrl_res_level i;
 	struct rdt_resource *r;
 	struct rdt_domain *dom;
 
 	lockdep_assert_held(&rdtgroup_mutex);
 
-	for_each_alloc_capable_rdt_resource(r) {
+	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
+		r = resctrl_arch_get_resource(i);
+		if (!r->alloc_capable)
+			continue;
+
 		list_for_each_entry(dom, &r->domains, list)
 			memset(dom->staged_config, 0, sizeof(dom->staged_config));
 	}
@@ -2192,6 +2197,7 @@ static u32 fflags_from_resource(struct rdt_resource *r)
 
 static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 {
+	enum resctrl_res_level i;
 	struct resctrl_schema *s;
 	struct rdt_resource *r;
 	unsigned long fflags;
@@ -2216,7 +2222,11 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 			goto out_destroy;
 	}
 
-	for_each_mon_capable_rdt_resource(r) {
+	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
+		r = resctrl_arch_get_resource(i);
+		if (!r->mon_capable)
+			continue;
+
 		fflags =  fflags_from_resource(r) | RFTYPE_MON_INFO;
 		sprintf(name, "%s_MON", r->name);
 		ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
@@ -2637,10 +2647,15 @@ static int schemata_list_add(struct rdt_resource *r, enum resctrl_conf_type type
 
 static int schemata_list_create(void)
 {
+	enum resctrl_res_level i;
 	struct rdt_resource *r;
 	int ret = 0;
 
-	for_each_alloc_capable_rdt_resource(r) {
+	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
+		r = resctrl_arch_get_resource(i);
+		if (!r->alloc_capable)
+			continue;
+
 		if (resctrl_arch_get_cdp_enabled(r->rid)) {
 			ret = schemata_list_add(r, CDP_CODE);
 			if (ret)
@@ -3181,6 +3196,7 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
 			     struct rdtgroup *prgrp,
 			     struct kernfs_node **dest_kn)
 {
+	enum resctrl_res_level i;
 	struct rdt_resource *r;
 	struct kernfs_node *kn;
 	int ret;
@@ -3199,7 +3215,11 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
 	 * Create the subdirectories for each domain. Note that all events
 	 * in a domain like L3 are grouped into a resource whose domain is L3
 	 */
-	for_each_mon_capable_rdt_resource(r) {
+	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
+		r = resctrl_arch_get_resource(i);
+		if (!r->mon_capable)
+			continue;
+
 		ret = mkdir_mondata_subdir_alldom(kn, r, prgrp);
 		if (ret)
 			goto out_destroy;
-- 
2.39.2

Hi James,

On 6/14/24 8:00 AM, James Morse wrote:
> The for_each_*_rdt_resource() helpers walk the architecture's array
> of structures, using the resctrl visible part as an iterator. These
> became over-complex when the structures were split into a
> filesystem and architecture-specific struct. This approach avoided
> the need to touch every call site.
> 
> Once the filesystem parts of resctrl are moved to /fs/, both the
> architecture's resource array, and the definition of those structures
> is no longer accessible. To support resctrl, each architecture would
> have to provide equally complex macros.
> 
> Change the resctrl code that uses these to walk through the resource_level
> enum and check the mon/alloc capable flags instead. Instances in core.c,
> and resctrl_arch_reset_resources() remain part of x86's architecture
> specific code.
> 
> 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: Peter Newman <peternewman@google.com>
> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
> Changes since v1:
>   * [Whitespace only] Fix bogus whitespace introduced in
>     rdtgroup_create_info_dir().
> 
>   * [Commit message only] Typo fix:
>     s/architectures/architecture's/g
> ---
>   arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  7 +++++-
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 28 +++++++++++++++++++----
>   2 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index aacf236dfe3b..ad20822bb64e 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -840,6 +840,7 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm
>   bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
>   {
>   	cpumask_var_t cpu_with_psl;
> +	enum resctrl_res_level i;
>   	struct rdt_resource *r;
>   	struct rdt_domain *d_i;
>   	bool ret = false;
> @@ -854,7 +855,11 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
>   	 * First determine which cpus have pseudo-locked regions
>   	 * associated with them.
>   	 */
> -	for_each_alloc_capable_rdt_resource(r) {
> +	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
> +		r = resctrl_arch_get_resource(i);
> +		if (!r->alloc_capable)
> +			continue;
> +

This looks like enough duplicate boilerplate for a new macro. For simplicity the
macro could require two arguments with enum resctrl_res_level also provided?

Reinette

Hi Reinette,

On 28/06/2024 17:48, Reinette Chatre wrote:
> On 6/14/24 8:00 AM, James Morse wrote:
>> The for_each_*_rdt_resource() helpers walk the architecture's array
>> of structures, using the resctrl visible part as an iterator. These
>> became over-complex when the structures were split into a
>> filesystem and architecture-specific struct. This approach avoided
>> the need to touch every call site.
>>
>> Once the filesystem parts of resctrl are moved to /fs/, both the
>> architecture's resource array, and the definition of those structures
>> is no longer accessible. To support resctrl, each architecture would
>> have to provide equally complex macros.
>>
>> Change the resctrl code that uses these to walk through the resource_level
>> enum and check the mon/alloc capable flags instead. Instances in core.c,
>> and resctrl_arch_reset_resources() remain part of x86's architecture
>> specific code.

>> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> index aacf236dfe3b..ad20822bb64e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> @@ -854,7 +855,11 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
>>        * First determine which cpus have pseudo-locked regions
>>        * associated with them.
>>        */
>> -    for_each_alloc_capable_rdt_resource(r) {
>> +    for (i = 0; i < RDT_NUM_RESOURCES; i++) {
>> +        r = resctrl_arch_get_resource(i);
>> +        if (!r->alloc_capable)
>> +            continue;
>> +
> 
> This looks like enough duplicate boilerplate for a new macro. For simplicity the
> macro could require two arguments with enum resctrl_res_level also provided?

I was hoping to escape from these clever macros! If you think this is too much:
- we'd need to come up with another name, as the arch code keeps the existing definition.
- to avoid touching every caller, it needs doing without an explicit iterator variable.

I guess the cleanest thing is to redefine the existing macros to use
resctrl_arch_get_resource(). Putting this in include/linxu/resctrl.h at least avoids each
architecture needing to define these, or forcing it to use an array.

The result is slightly more readable than the current version:
| #define for_each_rdt_resource(_r)                              \
|        for (_r = resctrl_arch_get_resource(0);                 \
|             _r->rid < RDT_NUM_RESOURCES;                       \
|             _r = resctrl_arch_get_resource(_r->rid + 1))

This leans heavily on resctrl_arch_get_resource() not being able to return NULL, and
having to return a dummy resource that is neither alloc nor mon capable. We may need to
revisit that if it becomes a burden for the arch code.


Thanks,

James

Hi James,

On 7/1/24 11:16 AM, James Morse wrote:
> Hi Reinette,
> 
> On 28/06/2024 17:48, Reinette Chatre wrote:
>> On 6/14/24 8:00 AM, James Morse wrote:
>>> The for_each_*_rdt_resource() helpers walk the architecture's array
>>> of structures, using the resctrl visible part as an iterator. These
>>> became over-complex when the structures were split into a
>>> filesystem and architecture-specific struct. This approach avoided
>>> the need to touch every call site.
>>>
>>> Once the filesystem parts of resctrl are moved to /fs/, both the
>>> architecture's resource array, and the definition of those structures
>>> is no longer accessible. To support resctrl, each architecture would
>>> have to provide equally complex macros.
>>>
>>> Change the resctrl code that uses these to walk through the resource_level
>>> enum and check the mon/alloc capable flags instead. Instances in core.c,
>>> and resctrl_arch_reset_resources() remain part of x86's architecture
>>> specific code.
> 
>>> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>> b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>> index aacf236dfe3b..ad20822bb64e 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>> @@ -854,7 +855,11 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
>>>         * First determine which cpus have pseudo-locked regions
>>>         * associated with them.
>>>         */
>>> -    for_each_alloc_capable_rdt_resource(r) {
>>> +    for (i = 0; i < RDT_NUM_RESOURCES; i++) {
>>> +        r = resctrl_arch_get_resource(i);
>>> +        if (!r->alloc_capable)
>>> +            continue;
>>> +
>>
>> This looks like enough duplicate boilerplate for a new macro. For simplicity the
>> macro could require two arguments with enum resctrl_res_level also provided?
> 
> I was hoping to escape from these clever macros! If you think this is too much:
> - we'd need to come up with another name, as the arch code keeps the existing definition.
> - to avoid touching every caller, it needs doing without an explicit iterator variable.
> 
> I guess the cleanest thing is to redefine the existing macros to use
> resctrl_arch_get_resource(). Putting this in include/linxu/resctrl.h at least avoids each
> architecture needing to define these, or forcing it to use an array.
> 
> The result is slightly more readable than the current version:
> | #define for_each_rdt_resource(_r)                              \
> |        for (_r = resctrl_arch_get_resource(0);                 \
> |             _r->rid < RDT_NUM_RESOURCES;                       \
> |             _r = resctrl_arch_get_resource(_r->rid + 1))
> 
> This leans heavily on resctrl_arch_get_resource() not being able to return NULL, and
> having to return a dummy resource that is neither alloc nor mon capable. We may need to
> revisit that if it becomes a burden for the arch code.

Replacing the repetitive four lines of code with a single line seems good to me.

resctrl_arch_get_resource() being able to return NULL is introduced in this series but
I am not seeing any handling of a possible NULL value. Not being able to return NULL thus
already seems a requirement?

Reinette

Hi Reinette,

On 01/07/2024 22:10, Reinette Chatre wrote:
> On 7/1/24 11:16 AM, James Morse wrote:
>> On 28/06/2024 17:48, Reinette Chatre wrote:
>>> On 6/14/24 8:00 AM, James Morse wrote:
>>>> The for_each_*_rdt_resource() helpers walk the architecture's array
>>>> of structures, using the resctrl visible part as an iterator. These
>>>> became over-complex when the structures were split into a
>>>> filesystem and architecture-specific struct. This approach avoided
>>>> the need to touch every call site.
>>>>
>>>> Once the filesystem parts of resctrl are moved to /fs/, both the
>>>> architecture's resource array, and the definition of those structures
>>>> is no longer accessible. To support resctrl, each architecture would
>>>> have to provide equally complex macros.
>>>>
>>>> Change the resctrl code that uses these to walk through the resource_level
>>>> enum and check the mon/alloc capable flags instead. Instances in core.c,
>>>> and resctrl_arch_reset_resources() remain part of x86's architecture
>>>> specific code.
>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>>> b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>>> index aacf236dfe3b..ad20822bb64e 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>>>> @@ -854,7 +855,11 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
>>>>         * First determine which cpus have pseudo-locked regions
>>>>         * associated with them.
>>>>         */
>>>> -    for_each_alloc_capable_rdt_resource(r) {
>>>> +    for (i = 0; i < RDT_NUM_RESOURCES; i++) {
>>>> +        r = resctrl_arch_get_resource(i);
>>>> +        if (!r->alloc_capable)
>>>> +            continue;
>>>> +
>>>
>>> This looks like enough duplicate boilerplate for a new macro. For simplicity the
>>> macro could require two arguments with enum resctrl_res_level also provided?
>>
>> I was hoping to escape from these clever macros! If you think this is too much:
>> - we'd need to come up with another name, as the arch code keeps the existing definition.
>> - to avoid touching every caller, it needs doing without an explicit iterator variable.
>>
>> I guess the cleanest thing is to redefine the existing macros to use
>> resctrl_arch_get_resource(). Putting this in include/linxu/resctrl.h at least avoids each
>> architecture needing to define these, or forcing it to use an array.
>>
>> The result is slightly more readable than the current version:
>> | #define for_each_rdt_resource(_r)                              \
>> |        for (_r = resctrl_arch_get_resource(0);                 \
>> |             _r->rid < RDT_NUM_RESOURCES;                       \
>> |             _r = resctrl_arch_get_resource(_r->rid + 1))
>>
>> This leans heavily on resctrl_arch_get_resource() not being able to return NULL, and
>> having to return a dummy resource that is neither alloc nor mon capable. We may need to
>> revisit that if it becomes a burden for the arch code.
> 
> Replacing the repetitive four lines of code with a single line seems good to me.

> resctrl_arch_get_resource() being able to return NULL is introduced in this series but
> I am not seeing any handling of a possible NULL value. Not being able to return NULL thus
> already seems a requirement?

It's currently implicit because until this point resctrl has just reached into the
rdt_resources_all[] array - and can never get a NULL pointer. Replacing that with a helper
needed to preserve the no-NULLs behaviour.
Changing this created too much churn so the resctrl idiom is to check
alloc_enabled/mon_enabled to see if the resource actually exists....

If we wanted to change this, that for_each_rdt_resource() would need an index variable as
_r could be NULL.


Thanks,

James