Messages in this thread | | | Date | Fri, 31 Mar 2023 16:19:48 -0700 | Subject | Re: [PATCH v3 02/19] x86/resctrl: Access per-rmid structures by index | From | Reinette Chatre <> |
| |
Hi James,
On 3/20/2023 10:26 AM, James Morse wrote: > Because of the differences between Intel RDT/AMD QoS and Arm's MPAM > monitors, RMID values on arm64 are not unique unless the CLOSID is > also included. Bitmaps like rmid_busy_llc need to be sized by the > number of unique entries for this resource. > > Add helpers to encode/decode the CLOSID and RMID to an index. The > domain's rmid_busy__llc and the rmid_ptrs[] array are then sized by
rmid_busy__llc -> rmid_busy_llc
Not a big deal but since you are using [] for rmid_ptrs[] it should not be necessary to say that it is an array.
> index, as are the domain mbm_local and mbm_total arrays. > On x86, the index is always just the RMID, so all these structures > remain the same size. > > The index gives resctrl a unique value it can use to store monitor > values, and allows MPAM to decode the closid when reading the hardware > counters.
When you are switching between CLOSID and closid in the same context it is less obvious that it means the same thing.
> > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v1: > * Added X86_BAD_CLOSID macro to make it clear what this value means > * Added second WARN_ON() for closid checking, and made both _ONCE() > > Changes since v2: > * Added RESCTRL_RESERVED_CLOSID > * Removed a newline > * Repharsed some comments > * Renamed a variable 'ignore'd > * Moved X86_RESCTRL_BAD_CLOSID to a previous patch > --- > arch/x86/include/asm/resctrl.h | 17 ++++++ > arch/x86/kernel/cpu/resctrl/core.c | 2 +- > arch/x86/kernel/cpu/resctrl/internal.h | 1 + > arch/x86/kernel/cpu/resctrl/monitor.c | 83 +++++++++++++++++--------- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 7 ++- > include/linux/resctrl.h | 3 + > 6 files changed, 82 insertions(+), 31 deletions(-) > > diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h > index cbe986d23df6..3ca40be41a0a 100644 > --- a/arch/x86/include/asm/resctrl.h > +++ b/arch/x86/include/asm/resctrl.h > @@ -101,6 +101,23 @@ static inline void resctrl_sched_in(void) > __resctrl_sched_in(); > } > > +static inline u32 resctrl_arch_system_num_rmid_idx(void) > +{ > + /* RMID are independent numbers for x86. num_rmid_idx==num_rmid */
Could you add spaces around the "=="?
> + return boot_cpu_data.x86_cache_max_rmid + 1; > +} > + > +static inline void resctrl_arch_rmid_idx_decode(u32 idx, u32 *closid, u32 *rmid) > +{ > + *rmid = idx; > + *closid = X86_RESCTRL_BAD_CLOSID; > +} > + > +static inline u32 resctrl_arch_rmid_idx_encode(u32 ignored, u32 rmid) > +{ > + return rmid; > +} > + > void resctrl_cpu_detect(struct cpuinfo_x86 *c); > > #else > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c > index 030d3b409768..351319403f84 100644 > --- a/arch/x86/kernel/cpu/resctrl/core.c > +++ b/arch/x86/kernel/cpu/resctrl/core.c > @@ -600,7 +600,7 @@ static void clear_closid_rmid(int cpu) > state->default_rmid = 0; > state->cur_closid = 0; > state->cur_rmid = 0; > - wrmsr(MSR_IA32_PQR_ASSOC, 0, 0); > + wrmsr(MSR_IA32_PQR_ASSOC, RESCTRL_RESERVED_CLOSID, 0); > } > > static int resctrl_online_cpu(unsigned int cpu) > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index c64097947994..47506e2afd59 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -7,6 +7,7 @@ > #include <linux/kernfs.h> > #include <linux/fs_context.h> > #include <linux/jump_label.h> > +#include <asm/resctrl.h> > > #define L3_QOS_CDP_ENABLE 0x01ULL > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index 18c37d364030..03a7d13dd653 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -142,12 +142,29 @@ static inline u64 get_corrected_mbm_count(u32 rmid, unsigned long val) > return val; > } > > -static inline struct rmid_entry *__rmid_entry(u32 closid, u32 rmid) > +/* > + * x86 and arm64 differ in their handling of monitoring. > + * x86's RMID are an independent number, there is only one source of traffic > + * an RMID value of '1'.
"source of traffic an RMID" -> "source of traffic with an RMID" ?
> + * arm64's PMG extend the PARTID/CLOSID space, there are multiple sources of > + * traffic with a PMG value of '1', one for each CLOSID, meaining the RMID
meaining -> meaning
> + * value is no longer unique.
Reinette
| |