Messages in this thread | | | From | "Yu, Fenghua" <> | Subject | RE: [PATCH v2 08/18] x86/resctrl: Queue mon_event_read() instead of sending an IPI | Date | Tue, 17 Jan 2023 18:29:18 +0000 |
| |
Hi, James,
> x86 is blessed with an abundance of monitors, one per RMID, that can be read > from any CPU in the domain. MPAMs monitors reside in the MMIO MSC, the > number implemented is up to the manufacturer. This means when there are > fewer monitors than needed, they need to be allocated and freed. > > Worse, the domain may be broken up into slices, and the MMIO accesses for > each slice may need performing from different CPUs. > > These two details mean MPAMs monitor code needs to be able to sleep, and IPI > another CPU in the domain to read from a resource that has been sliced. > > mon_event_read() already invokes mon_event_count() via IPI, which means this > isn't possible. > > Change mon_event_read() to schedule mon_event_count() on a remote CPU > and wait, instead of sending an IPI. This function is only used in response to a > user-space filesystem request (not the timing sensitive overflow code).
But mkdir mon group needs mon_event_count() to reset RMID state. If mon_event_count() is called much later, the RMID state may be used before it's reset. E.g. prev_msr might be non-0 value. That will cause overflow code failure.
Seems this may happen on both x86 and arm64. At least need to make sure RMID state reset happens before it's used.
> > This allows MPAM to hide the slice behaviour from resctrl, and to keep the > monitor-allocation in monitor.c. > > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 7 +++++-- > arch/x86/kernel/cpu/resctrl/internal.h | 2 +- > arch/x86/kernel/cpu/resctrl/monitor.c | 6 ++++-- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > index 1df0e3262bca..4ee3da6dced7 100644 > --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > @@ -532,8 +532,11 @@ void mon_event_read(struct rmid_read *rr, struct > rdt_resource *r, > struct rdt_domain *d, struct rdtgroup *rdtgrp, > int evtid, int first) > { > + /* When picking a cpu from cpu_mask, ensure it can't race with cpuhp > */ > + lockdep_assert_held(&rdtgroup_mutex); > + > /* > - * setup the parameters to send to the IPI to read the data. > + * setup the parameters to pass to mon_event_count() to read the data. > */ > rr->rgrp = rdtgrp; > rr->evtid = evtid; > @@ -542,7 +545,7 @@ void mon_event_read(struct rmid_read *rr, struct > rdt_resource *r, > rr->val = 0; > rr->first = first; > > - smp_call_function_any(&d->cpu_mask, mon_event_count, rr, 1); > + smp_call_on_cpu(cpumask_any(&d->cpu_mask), mon_event_count, rr, > +false); > } > > 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 adae6231324f..1f90a10b75a1 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -514,7 +514,7 @@ void closid_free(int closid); int alloc_rmid(u32 closid); > void free_rmid(u32 closid, u32 rmid); int rdt_get_mon_l3_config(struct > rdt_resource *r); -void mon_event_count(void *info); > +int mon_event_count(void *info); > int rdtgroup_mondata_show(struct seq_file *m, void *arg); void > mon_event_read(struct rmid_read *rr, struct rdt_resource *r, > struct rdt_domain *d, struct rdtgroup *rdtgrp, diff --git > a/arch/x86/kernel/cpu/resctrl/monitor.c > b/arch/x86/kernel/cpu/resctrl/monitor.c > index 190ac183139e..d309b830aeb2 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -509,10 +509,10 @@ static void mbm_bw_count(u32 closid, u32 rmid, > struct rmid_read *rr) } > > /* > - * This is called via IPI to read the CQM/MBM counters > + * This is scheduled by mon_event_read() to read the CQM/MBM counters > * on a domain. > */ > -void mon_event_count(void *info) > +int mon_event_count(void *info) > { > struct rdtgroup *rdtgrp, *entry; > struct rmid_read *rr = info; > @@ -545,6 +545,8 @@ void mon_event_count(void *info) > */ > if (ret == 0) > rr->err = 0; > + > + return 0; > } > > /* > -- > 2.30.2
Thanks.
-Fenghua
| |