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, and was done before there was a
helper to retrieve a resource by rid.

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.

Rewrite the macro to make use of resctrl_arch_get_resource(), and
move these to the core header so existing x86 arch code continues
to use them.

Signed-off-by: James Morse <james.morse@arm.com>
Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
Changes since v3:
 * Restructure the existing macros instead of open-coding the for loop.

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/internal.h | 29 --------------------------
 include/linux/resctrl.h                | 18 ++++++++++++++++
 2 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 4e2ec1fed2ff..a434daa6dba4 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -472,14 +472,6 @@ extern struct rdt_hw_resource rdt_resources_all[];
 extern struct rdtgroup rdtgroup_default;
 extern struct dentry *debugfs_resctrl;
 
-static inline struct rdt_resource *resctrl_inc(struct rdt_resource *res)
-{
-	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(res);
-
-	hw_res++;
-	return &hw_res->r_resctrl;
-}
-
 static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
 {
 	return rdt_resources_all[l].cdp_enabled;
@@ -489,27 +481,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
 
 void arch_mon_domain_online(struct rdt_resource *r, struct rdt_mon_domain *d);
 
-/*
- * To return the common struct rdt_resource, which is contained in struct
- * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
- */
-#define for_each_rdt_resource(r)					      \
-	for (r = &rdt_resources_all[0].r_resctrl;			      \
-	     r <= &rdt_resources_all[RDT_NUM_RESOURCES - 1].r_resctrl;	      \
-	     r = resctrl_inc(r))
-
-#define for_each_capable_rdt_resource(r)				      \
-	for_each_rdt_resource(r)					      \
-		if (r->alloc_capable || r->mon_capable)
-
-#define for_each_alloc_capable_rdt_resource(r)				      \
-	for_each_rdt_resource(r)					      \
-		if (r->alloc_capable)
-
-#define for_each_mon_capable_rdt_resource(r)				      \
-	for_each_rdt_resource(r)					      \
-		if (r->mon_capable)
-
 /* CPUID.(EAX=10H, ECX=ResID=1).EAX */
 union cpuid_0x10_1_eax {
 	struct {
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 8894aed3c593..f75f0409ae09 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -26,6 +26,24 @@ int proc_resctrl_show(struct seq_file *m,
 /* max value for struct rdt_domain's mbps_val */
 #define MBA_MAX_MBPS   U32_MAX
 
+/* Walk all possible resources, with variants for only controls or monitors. */
+#define for_each_rdt_resource(_r)						\
+	for ((_r) = resctrl_arch_get_resource(0);				\
+	     (_r)->rid < RDT_NUM_RESOURCES - 1;					\
+	     (_r) = resctrl_arch_get_resource((_r)->rid + 1))
+
+#define for_each_capable_rdt_resource(r)				      \
+	for_each_rdt_resource((r))					      \
+		if ((r)->alloc_capable || (r)->mon_capable)
+
+#define for_each_alloc_capable_rdt_resource(r)				      \
+	for_each_rdt_resource((r))					      \
+		if ((r)->alloc_capable)
+
+#define for_each_mon_capable_rdt_resource(r)				      \
+	for_each_rdt_resource((r))					      \
+		if ((r)->mon_capable)
+
 /**
  * enum resctrl_conf_type - The type of configuration.
  * @CDP_NONE:	No prioritisation, both code and data are controlled or monitored.
-- 
2.39.2

Hi James,

On 10/4/24 11:03 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, and was done before there was a
> helper to retrieve a resource by rid.
> 
> 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.
> 
> Rewrite the macro to make use of resctrl_arch_get_resource(), and
> move these to the core header so existing x86 arch code continues
> to use them.

The last part is not clear, why does it need to be moved to core
header for x86 to use it?


...

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 8894aed3c593..f75f0409ae09 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -26,6 +26,24 @@ int proc_resctrl_show(struct seq_file *m,
>  /* max value for struct rdt_domain's mbps_val */
>  #define MBA_MAX_MBPS   U32_MAX
>  
> +/* Walk all possible resources, with variants for only controls or monitors. */
> +#define for_each_rdt_resource(_r)						\
> +	for ((_r) = resctrl_arch_get_resource(0);				\
> +	     (_r)->rid < RDT_NUM_RESOURCES - 1;					\

I do not think this reaches all resources ... should this perhaps be:
	(_r) && (_r)->rid < RDT_NUM_RESOURCES

> +	     (_r) = resctrl_arch_get_resource((_r)->rid + 1))
> +
> +#define for_each_capable_rdt_resource(r)				      \
> +	for_each_rdt_resource((r))					      \
> +		if ((r)->alloc_capable || (r)->mon_capable)
> +
> +#define for_each_alloc_capable_rdt_resource(r)				      \
> +	for_each_rdt_resource((r))					      \
> +		if ((r)->alloc_capable)
> +
> +#define for_each_mon_capable_rdt_resource(r)				      \
> +	for_each_rdt_resource((r))					      \
> +		if ((r)->mon_capable)
> +
>  /**
>   * enum resctrl_conf_type - The type of configuration.
>   * @CDP_NONE:	No prioritisation, both code and data are controlled or monitored.

Reinette

On Fri, Oct 04, 2024 at 06:03:24PM +0000, 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, and was done before there was a
> helper to retrieve a resource by rid.
> 
> 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.
> 
> Rewrite the macro to make use of resctrl_arch_get_resource(), and
> move these to the core header so existing x86 arch code continues
> to use them.

Apologies if this comment was suggested against earlier versions
of this series.

Did you consider replacing rdt_resources_all[] a list (in the filesystem
code) instead of an array (in the architecture code)?

List would start empty. Architecture init code would enumerate features
and add entries to the list for those that exist and are to be enabled.

The "for_each" macros then walk the list (variants for all entries,
for "alloc_capable" and for "mon_capable"). Note that only enabled
entries appear on the lists.

There are currently a bunch of places in filesystem code that
do:
	r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
or
	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);

those could become:

	r = resctrl_arch_get_mba_resource();

	r = resctrl_arch_get_l3_resource();

Then the whole "enum resctrl_res_level" and ->rid field in
struct rdt_resource could go away? Remaining uses look like
distinguishing MBA from SMBA. Perhaps better done with a
flags word?

Advantage of doing this would be to avoid the generic
enum resctrl_res_level having to be a superset of all
features across all architectures. E.g. ARM might want
to add L4/L5 resources, X86 may have some that ARM will
never need. RiscV may also follow some divergent path.

If this v5 series is close to being applied then I don't
want to derail with a re-write at this late stage.
All of this could be done as a cleanup after this series
has been applied.

-Tony

Hi Tony,

On 10/7/24 5:00 PM, Tony Luck wrote:
> On Fri, Oct 04, 2024 at 06:03:24PM +0000, 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, and was done before there was a
>> helper to retrieve a resource by rid.
>>
>> 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.
>>
>> Rewrite the macro to make use of resctrl_arch_get_resource(), and
>> move these to the core header so existing x86 arch code continues
>> to use them.
> 
> Apologies if this comment was suggested against earlier versions
> of this series.
> 
> Did you consider replacing rdt_resources_all[] a list (in the filesystem
> code) instead of an array (in the architecture code)?
> 
> List would start empty. Architecture init code would enumerate features
> and add entries to the list for those that exist and are to be enabled.
> 
> The "for_each" macros then walk the list (variants for all entries,
> for "alloc_capable" and for "mon_capable"). Note that only enabled
> entries appear on the lists.
> 
> There are currently a bunch of places in filesystem code that
> do:
> 	r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
> or
> 	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> 
> those could become:
> 
> 	r = resctrl_arch_get_mba_resource();
> 
> 	r = resctrl_arch_get_l3_resource();
> 
> Then the whole "enum resctrl_res_level" and ->rid field in
> struct rdt_resource could go away? Remaining uses look like
> distinguishing MBA from SMBA. Perhaps better done with a
> flags word?
> 
> Advantage of doing this would be to avoid the generic
> enum resctrl_res_level having to be a superset of all
> features across all architectures. E.g. ARM might want
> to add L4/L5 resources, X86 may have some that ARM will
> never need. RiscV may also follow some divergent path.

Ideally resctrl fs would remain as an interface that a user can use to interact
with all architectures without knowing architecture specific details. Platform
differences can be exposed by resctrl in a generic way to support this.
I am afraid that allowing architectures to diverge would require resctrl fs users
to additionally know which platform they are running on.

> If this v5 series is close to being applied then I don't
> want to derail with a re-write at this late stage.
> All of this could be done as a cleanup after this series
> has been applied.

Due to the already significant size of this work I think it would make it easier
if the number of functional changes are minimal. Specifically, only those functional
changes that are required to accomplish the goal of moving the code.

Considering that one goal of this proposal is to support architectural
flexibility I do think it would be easier to understand its impact if it
is implemented on top of the arch/fs split.

Reinette

Hi Tony, Reinette,

On 08/10/2024 17:40, Reinette Chatre wrote:
> On 10/7/24 5:00 PM, Tony Luck wrote:
>> On Fri, Oct 04, 2024 at 06:03:24PM +0000, 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, and was done before there was a
>>> helper to retrieve a resource by rid.
>>>
>>> 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.
>>>
>>> Rewrite the macro to make use of resctrl_arch_get_resource(), and
>>> move these to the core header so existing x86 arch code continues
>>> to use them.

>> Apologies if this comment was suggested against earlier versions
>> of this series.
>>
>> Did you consider replacing rdt_resources_all[] a list (in the filesystem
>> code) instead of an array (in the architecture code)?

I didn't consider this, but it would be a more natural fit for the secret for loops that
are all over the resctrl code.


>> List would start empty. Architecture init code would enumerate features
>> and add entries to the list for those that exist and are to be enabled.

That saves the 'can't return NULL' wart - but that was intended to be temporary - and only
a headache for !x86 architectures.


>> The "for_each" macros then walk the list (variants for all entries,
>> for "alloc_capable" and for "mon_capable"). Note that only enabled
>> entries appear on the lists.
>>
>> There are currently a bunch of places in filesystem code that
>> do:
>> 	r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
>> or
>> 	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>>
>> those could become:
>>
>> 	r = resctrl_arch_get_mba_resource();
>>
>> 	r = resctrl_arch_get_l3_resource();

Where these walk this list instead of 'knowing' the offset.
(just in case I'm missing a trick here)


>> Then the whole "enum resctrl_res_level" and ->rid field in
>> struct rdt_resource could go away?

I think level is still going to be useful for cache resources - that is something we
expose via the sysfs cpu cache/indexX stuff too. I'd like resctrl to generate the names of
resources - just to ensure they are the same on every architecture.

The rid is an existing field just to make the array searching work.


>> Remaining uses look like
>> distinguishing MBA from SMBA. Perhaps better done with a
>> flags word?
>>
>> Advantage of doing this would be to avoid the generic
>> enum resctrl_res_level having to be a superset of all
>> features across all architectures.

Ah, I see this as an advantage - its much harder for an architecture to add a new type of
control or resource than it is to provide compatibility with one that is already there.
This in turn is better for user-space.

MPAM's bandwidth controls don't have the same control format as Intel RDT - but its much
better for everyone if I convert the values to hide the differences instead of trying to
shoehorn in ARM_MB as a new resource, only to find another architecture grows something
similar.

The difficult bit is making sure new resources/controls are as generic as possible,
meaning other architectures can adopt them. (L3's bitmap is a good example).

(and I agree there will always be platform specific things each camp has)

>> E.g. ARM might want to add L4/L5 resources,

/me shudders.

I've seen folk wanting to add the 'system cache' - which sits where the L3 should be, but
behaves differently. And ACPI's "Memory Side Caches" which gives me a hilarious TLA
collision to navigate).
I've argued neither of these are L<n> caches because they aren't visible to user-space in
/sys/devices/system/cpu/cpu0/cache ...

[..]

> Ideally resctrl fs would remain as an interface that a user can use to interact
> with all architectures without knowing architecture specific details. Platform
> differences can be exposed by resctrl in a generic way to support this.
> I am afraid that allowing architectures to diverge would require resctrl fs users
> to additionally know which platform they are running on.
> 
>> If this v5 series is close to being applied then I don't
>> want to derail with a re-write at this late stage.
>> All of this could be done as a cleanup after this series
>> has been applied.
> 
> Due to the already significant size of this work I think it would make it easier
> if the number of functional changes are minimal. Specifically, only those functional
> changes that are required to accomplish the goal of moving the code.

Yup - hence the need for !alloc_capable && !mon_capable resources behind
resctrl_arch_get_resource() - this is keeping the behaviour of the existing code.


> Considering that one goal of this proposal is to support architectural
> flexibility I do think it would be easier to understand its impact if it
> is implemented on top of the arch/fs split.

Make sense to me,


Thanks,

James