Messages in this thread Patch in this message | | | Date | Thu, 5 Oct 2023 16:46:10 -0500 | Subject | Re: [PATCH v6 15/24] x86/resctrl: Allow arch to allocate memory needed in resctrl_arch_rmid_read() | From | "Moger, Babu" <> |
| |
Hi James,
On 9/14/2023 12:21 PM, James Morse wrote: > Depending on the number of monitors available, Arm's MPAM may need to > allocate a monitor prior to reading the counter value. Allocating a > contended resource may involve sleeping. > > add_rmid_to_limbo() calls resctrl_arch_rmid_read() for multiple domains, > the allocation should be valid for all domains. > > __check_limbo() and mon_event_count() each make multiple calls to > resctrl_arch_rmid_read(), to avoid extra work on contended systems, > the allocation should be valid for multiple invocations of > resctrl_arch_rmid_read(). > > Add arch hooks for this allocation, which need calling before > resctrl_arch_rmid_read(). The allocated monitor is passed to > resctrl_arch_rmid_read(), then freed again afterwards. The helper > can be called on any CPU, and can sleep. > > Reviewed-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Tested-By: Peter Newman <peternewman@google.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v3: > * Expanded comment. > * Removed stray header include. > * Reworded commit message. > * Made ctx a void * instead of an int. > > Changes since v4: > * Used IS_ERR() in more places. > > Changes since v5: > * Pass the error back from mon_event_read() as -EINVAL/Unavailable. > * Add some ratelimited warnings when failing to allocate a mon context > --- > arch/x86/include/asm/resctrl.h | 11 ++++++++ > arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 7 +++++ > arch/x86/kernel/cpu/resctrl/internal.h | 1 + > arch/x86/kernel/cpu/resctrl/monitor.c | 34 +++++++++++++++++++++-- > include/linux/resctrl.h | 5 +++- > 5 files changed, 54 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h > index 1d274dbabc44..29c4cc343787 100644 > --- a/arch/x86/include/asm/resctrl.h > +++ b/arch/x86/include/asm/resctrl.h > @@ -136,6 +136,17 @@ static inline u32 resctrl_arch_rmid_idx_encode(u32 ignored, u32 rmid) > return rmid; > } > > +/* x86 can always read an rmid, nothing needs allocating */ > +struct rdt_resource; > +static inline void *resctrl_arch_mon_ctx_alloc(struct rdt_resource *r, int evtid) > +{ > + might_sleep(); > + return NULL; > +}; > + > +static inline void resctrl_arch_mon_ctx_free(struct rdt_resource *r, int evtid, > + void *ctx) { }; > + > void resctrl_cpu_detect(struct cpuinfo_x86 *c); > > #else > diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > index bd263b9a0abd..ce4821ea111b 100644 > --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > @@ -546,6 +546,11 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r, > rr->d = d; > rr->val = 0; > rr->first = first; > + rr->arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, evtid); > + if (IS_ERR(rr->arch_mon_ctx)) { > + rr->err = -EINVAL; > + return; > + } > > cpu = cpumask_any_housekeeping(&d->cpu_mask); > > @@ -559,6 +564,8 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r, > smp_call_function_any(&d->cpu_mask, mon_event_count, rr, 1); > else > smp_call_on_cpu(cpu, smp_mon_event_count, rr, false); > + > + resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx); > } > > int rdtgroup_mondata_show(struct seq_file *m, void *arg) > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index 37bb3de37a4a..66d9ebb5e03a 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -136,6 +136,7 @@ struct rmid_read { > bool first; > int err; > u64 val; > + void *arch_mon_ctx; > }; > > extern bool rdt_alloc_capable; > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index 05d949ec94f1..28a2c8765faf 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -270,7 +270,7 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width) > > int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > u32 unused, u32 rmid, enum resctrl_event_id eventid, > - u64 *val) > + u64 *val, void *ignored) > { > struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); > struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d); > @@ -325,9 +325,17 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > u32 idx_limit = resctrl_arch_system_num_rmid_idx(); > struct rmid_entry *entry; > u32 idx, cur_idx = 1; > + void *arch_mon_ctx; > bool rmid_dirty; > u64 val = 0; > > + arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, QOS_L3_OCCUP_EVENT_ID); > + if (IS_ERR(arch_mon_ctx)) { > + pr_warn_ratelimited("Failed to allocate monitor context: %ld", > + PTR_ERR(arch_mon_ctx)); > + return; > + } > + > /* > * Skip RMID 0 and start from RMID 1 and check all the RMIDs that > * are marked as busy for occupancy < threshold. If the occupancy > @@ -341,7 +349,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > > entry = __rmid_entry(idx); > if (resctrl_arch_rmid_read(r, d, entry->closid, entry->rmid, > - QOS_L3_OCCUP_EVENT_ID, &val)) { > + QOS_L3_OCCUP_EVENT_ID, &val, > + arch_mon_ctx)) { > rmid_dirty = true; > } else { > rmid_dirty = (val >= resctrl_rmid_realloc_threshold); > @@ -354,6 +363,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > } > cur_idx = idx + 1; > } > + > + resctrl_arch_mon_ctx_free(r, QOS_L3_OCCUP_EVENT_ID, arch_mon_ctx); > } > > bool has_busy_rmid(struct rdt_domain *d) > @@ -532,7 +543,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr) > } > > rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid, rr->evtid, > - &tval); > + &tval, rr->arch_mon_ctx); > if (rr->err) > return rr->err; > > @@ -743,11 +754,27 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, > if (is_mbm_total_enabled()) { > rr.evtid = QOS_L3_MBM_TOTAL_EVENT_ID; > rr.val = 0; > + rr.arch_mon_ctx = resctrl_arch_mon_ctx_alloc(rr.r, rr.evtid); > + if (IS_ERR(rr.arch_mon_ctx)) { > + pr_warn_ratelimited("Failed to allocate monitor context: %ld", > + PTR_ERR(rr.arch_mon_ctx)); > + return; > + } > + > __mon_event_count(closid, rmid, &rr); > + > + resctrl_arch_mon_ctx_free(rr.r, rr.evtid, rr.arch_mon_ctx); > } > if (is_mbm_local_enabled()) { > rr.evtid = QOS_L3_MBM_LOCAL_EVENT_ID; > rr.val = 0; > + rr.arch_mon_ctx = resctrl_arch_mon_ctx_alloc(rr.r, rr.evtid); > + if (IS_ERR(rr.arch_mon_ctx)) { > + pr_warn_ratelimited("Failed to allocate monitor context: %ld", > + PTR_ERR(rr.arch_mon_ctx)); > + return; > + } > + > __mon_event_count(closid, rmid, &rr); > > /* > @@ -757,6 +784,7 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, > */ > if (is_mba_sc(NULL)) > mbm_bw_count(closid, rmid, &rr); > + resctrl_arch_mon_ctx_free(rr.r, rr.evtid, rr.arch_mon_ctx);
Space before the last line would be cleaner.
if (is_mba_sc(NULL)) mbm_bw_count(closid, rmid, &rr); + + resctrl_arch_mon_ctx_free(rr.r, rr.evtid, rr.arch_mon_ctx);
> } > } > > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h > index f7311102e94c..5e4b4df9610b 100644 > --- a/include/linux/resctrl.h > +++ b/include/linux/resctrl.h > @@ -235,6 +235,9 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d); > * @rmid: rmid of the counter to read. > * @eventid: eventid to read, e.g. L3 occupancy. > * @val: result of the counter read in bytes. > + * @arch_mon_ctx: An architecture specific value from > + * resctrl_arch_mon_ctx_alloc(), for MPAM this identifies > + * the hardware monitor allocated for this read request. > * > * Some architectures need to sleep when first programming some of the counters. > * (specifically: arm64's MPAM cache occupancy counters can return 'not ready' > @@ -248,7 +251,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d); > */ > int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > u32 closid, u32 rmid, enum resctrl_event_id eventid, > - u64 *val); > + u64 *val, void *arch_mon_ctx);
Just wondering.. Have you thought about passing rmid_read structure to this function?
Because most of the information is already inside the rmid_read structure. We can avoid passing 7 parameters.
Thanks
Babu
> > /** > * resctrl_arch_rmid_read_context_check() - warn about invalid contexts
| |