[PATCH v8 12/21] x86/resctrl: Move the filesystem bits to headers visible to fs/resctrl

James Morse posted 21 patches 8 months, 1 week ago
There is a newer version of this series
[PATCH v8 12/21] x86/resctrl: Move the filesystem bits to headers visible to fs/resctrl
Posted by James Morse 8 months, 1 week ago
Once the filesystem parts of resctrl move to fs/resctrl, it cannot rely
on definitions in x86's internal.h.

Move definitions in internal.h that need to be shared between the
filesystem and architecture code to header files that fs/resctrl can
include.

Doing this separately means the filesystem code only moves between files
of the same name, instead of having these changes mixed in too.

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>
Tested-by: Peter Newman <peternewman@google.com>
Tested-by: Amit Singh Tomar <amitsinght@marvell.com> # arm64
Tested-by: Shanker Donthineni <sdonthineni@nvidia.com> # arm64
Tested-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes since v6:
 * Moved resctrl_arch_set_cdp_enabled() to rdtgroup.c instead of core.c

Changes since v3:
 * Changed the number of hyphens at the end of the commit message.

Changes since v2:
 * Dropped the rfflags and some other defines from being moved.

Changes since v1:
 * Revert apparently unintentional duplication of a couple of variable
   declarations in <linux/resctrl.h>.

   No functional change.
---
 arch/x86/include/asm/resctrl.h         | 3 +++
 arch/x86/kernel/cpu/resctrl/internal.h | 9 ---------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 5 +++++
 include/linux/resctrl_types.h          | 3 +++
 4 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
index 7a39728b0743..6eb7d5c94c7a 100644
--- a/arch/x86/include/asm/resctrl.h
+++ b/arch/x86/include/asm/resctrl.h
@@ -210,6 +210,9 @@ int resctrl_arch_measure_l2_residency(void *_plr);
 int resctrl_arch_measure_l3_residency(void *_plr);
 void resctrl_cpu_detect(struct cpuinfo_x86 *c);
 
+bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l);
+int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
+
 #else
 
 static inline void resctrl_arch_sched_in(struct task_struct *tsk) {}
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 365d5697a6ae..866b9fec5a11 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -16,8 +16,6 @@
 #define CQM_LIMBOCHECK_INTERVAL	1000
 
 #define MBM_CNTR_WIDTH_BASE		24
-#define MBM_OVERFLOW_INTERVAL		1000
-#define MAX_MBA_BW			100u
 #define MBA_IS_LINEAR			0x4
 #define MBM_CNTR_WIDTH_OFFSET_AMD	20
 
@@ -410,13 +408,6 @@ extern struct rdtgroup rdtgroup_default;
 extern struct dentry *debugfs_resctrl;
 extern enum resctrl_event_id mba_mbps_default_event;
 
-static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
-{
-	return rdt_resources_all[l].cdp_enabled;
-}
-
-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);
 
 /* CPUID.(EAX=10H, ECX=ResID=1).EAX */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 23fd92cc0c56..25dc94cb8e88 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2536,6 +2536,11 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
 	return 0;
 }
 
+bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
+{
+	return rdt_resources_all[l].cdp_enabled;
+}
+
 /*
  * We don't allow rdtgroup directories to be created anywhere
  * except the root directory. Thus when looking for the rdtgroup
diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
index 654323066174..a7faf2cd5406 100644
--- a/include/linux/resctrl_types.h
+++ b/include/linux/resctrl_types.h
@@ -7,6 +7,9 @@
 #ifndef __LINUX_RESCTRL_TYPES_H
 #define __LINUX_RESCTRL_TYPES_H
 
+#define MAX_MBA_BW			100u
+#define MBM_OVERFLOW_INTERVAL		1000
+
 /* Reads to Local DRAM Memory */
 #define READS_TO_LOCAL_MEM		BIT(0)
 
-- 
2.20.1
Re: [PATCH v8 12/21] x86/resctrl: Move the filesystem bits to headers visible to fs/resctrl
Posted by Reinette Chatre 8 months ago
Hi James,

> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 7a39728b0743..6eb7d5c94c7a 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -210,6 +210,9 @@ int resctrl_arch_measure_l2_residency(void *_plr);
>  int resctrl_arch_measure_l3_residency(void *_plr);
>  void resctrl_cpu_detect(struct cpuinfo_x86 *c);
>  
> +bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l);
> +int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
> +
>  #else


I noticed this while reading through the telemetry work ... the custom
appears to be for all arch helpers to be declared in include/linux/resctrl.h
making these two stand out. What motivated their inclusion into
arch/x86/include/asm/resctrl.h?

If they move to include/linux/resctrl.h it looks like enum resctrl_res_level is
no longer required to be in include/linux/resctrl_types.h.

Looking further, from commit f16adbaf9272 ("x86/resctrl: Move resctrl types to a separate header")
the motivation for including enum resctrl_event_id is to support 
resctrl_arch_mon_ctx_alloc() and resctrl_arch_mon_ctx_free(), but looking
at their definitions in arch/x86/include/asm/resctrl.h they are using "int evtid"
instead of enum resctrl_event_id. Looks like their definitions need to
change?

I assume the pseudo-locking arch helpers are placed in arch/x86/include/asm/resctrl.h
with the implicit knowledge that only x86 will set CONFIG_RESCTRL_FS_PSEUDO_LOCK so
avoiding that extra handling in include/linux/resctrl.h?

Reinette
Re: [PATCH v8 12/21] x86/resctrl: Move the filesystem bits to headers visible to fs/resctrl
Posted by James Morse 7 months, 4 weeks ago
Hi Reinette,

On 17/04/2025 23:46, Reinette Chatre wrote:
>> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
>> index 7a39728b0743..6eb7d5c94c7a 100644
>> --- a/arch/x86/include/asm/resctrl.h
>> +++ b/arch/x86/include/asm/resctrl.h
>> @@ -210,6 +210,9 @@ int resctrl_arch_measure_l2_residency(void *_plr);
>>  int resctrl_arch_measure_l3_residency(void *_plr);
>>  void resctrl_cpu_detect(struct cpuinfo_x86 *c);
>>  
>> +bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l);
>> +int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);

> I noticed this while reading through the telemetry work ... the custom
> appears to be for all arch helpers to be declared in include/linux/resctrl.h
> making these two stand out. What motivated their inclusion into
> arch/x86/include/asm/resctrl.h?

Doing some archaeology ... it looks like this dates from the era when MPAM had stub
versions of these because it didn't support/emulate CDP. Having these in the arch code's
header meant the static-inline stubs could be inlined into resctrl. x86 has an inline
version before this patch, but I moved it out of line to avoid exposing the array of all
resources. (which in turn would suggest changing the name, which isn't worth the churn)

I was assuming only x86 would ever support CDP, but since then, emulating CDP using the
separate I/D PARTID values MPAM has proved feasible.

I'll move them.


> If they move to include/linux/resctrl.h it looks like enum resctrl_res_level is
> no longer required to be in include/linux/resctrl_types.h.

Yes. I'll add a patch move it, but it feels like churn.


> Looking further, from commit f16adbaf9272 ("x86/resctrl: Move resctrl types to a separate header")
> the motivation for including enum resctrl_event_id is to support 
> resctrl_arch_mon_ctx_alloc() and resctrl_arch_mon_ctx_free(), but looking
> at their definitions in arch/x86/include/asm/resctrl.h they are using "int evtid"
> instead of enum resctrl_event_id. Looks like their definitions need to
> change?

Bother. The MPAM definition of those has the enum type, I'd missed that the x86 stub
swallowed the type. I'll fix that.


> I assume the pseudo-locking arch helpers are placed in arch/x86/include/asm/resctrl.h
> with the implicit knowledge that only x86 will set CONFIG_RESCTRL_FS_PSEUDO_LOCK so
> avoiding that extra handling in include/linux/resctrl.h?

Given how specific that is to x86, I think it will only ever be enabled there.
For arm, things like the prefetcher can't be disabled by linux. I would assume risc-v is
structurally the same.

Those definitions went into the arch header so that some architectures could inline the
stubs to let the compiler remove code that will never run.

I guess these could be defined both ways round in linux/resctrl.h depending on the Kconfig
symbol. At this point its churn, but this saves a third architecture having to define the
same list of stubs. (I reckon that is likely for risc-v).


Thanks,

James