Messages in this thread | | | Date | Fri, 31 Mar 2023 16:27:51 -0700 | Subject | Re: [PATCH v3 11/19] x86/resctrl: Allow arch to allocate memory needed in resctrl_arch_rmid_read() | From | Reinette Chatre <> |
| |
Hi James,
On 3/20/2023 10:26 AM, 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. > > All callers of resctrl_arch_rmid_read() read the counter on more than > one domain. If the monitor is allocated globally, there is no need to
This does not seem accurate considering the __check_limbo() call that is called for a single domain.
> allocate and free it for each call to 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. > > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/x86/include/asm/resctrl.h | 11 +++++++ > arch/x86/kernel/cpu/resctrl/internal.h | 1 + > arch/x86/kernel/cpu/resctrl/monitor.c | 40 +++++++++++++++++++++++--- > include/linux/resctrl.h | 4 +-- > 4 files changed, 50 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h > index 752123b0ce40..1c87f1626456 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 int resctrl_arch_mon_ctx_alloc(struct rdt_resource *r, int evtid) > +{ > + might_sleep(); > + return 0; > +}; > + > +static inline void resctrl_arch_mon_ctx_free(struct rdt_resource *r, int evtid, > + int ctx) { }; > + > void resctrl_cpu_detect(struct cpuinfo_x86 *c); > > #else > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index a07557390895..7262b355e128 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -135,6 +135,7 @@ struct rmid_read { > bool first; > int err; > u64 val; > + int 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 de72df06b37b..f38cd2f12285 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -15,6 +15,7 @@ > * Software Developer Manual June 2016, volume 3, section 17.17. > */ > > +#include <linux/cpu.h>
Why is this needed?
> #include <linux/module.h> > #include <linux/sizes.h> > #include <linux/slab.h> > @@ -271,7 +272,7 @@ static void smp_call_rmid_read(void *_arg) > > 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, int ignored) > { > struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); > struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d); > @@ -317,9 +318,14 @@ 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; > + int arch_mon_ctx; > bool rmid_dirty; > u64 val = 0; > > + arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, QOS_L3_OCCUP_EVENT_ID); > + if (arch_mon_ctx < 0) > + return; > +
The vision for this is not clear to me. When I read that context needs to be allocated I expect it to return a pointer to some new context, not an int. What would the "context" consist of?
...
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h > index ff7452f644e4..03e4f41cd336 100644 > --- a/include/linux/resctrl.h > +++ b/include/linux/resctrl.h > @@ -233,6 +233,7 @@ 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 allocated context from resctrl_arch_mon_ctx_alloc(). > *
Could this description be expanded to indicate what this context is used for?
> * Call from process context on a CPU that belongs to domain @d. > *
Reinette
| |