Messages in this thread | | | Date | Thu, 5 Oct 2023 16:33:00 -0500 | Subject | Re: [PATCH v6 14/24] x86/resctrl: Allow resctrl_arch_rmid_read() to sleep | From | "Moger, Babu" <> |
| |
Hi James,
On 9/14/2023 12:21 PM, James Morse wrote: > MPAM's cache occupancy counters can take a little while to settle once > the monitor has been configured. The maximum settling time is described > to the driver via a firmware table. The value could be large enough > that it makes sense to sleep. To avoid exposing this to resctrl, it > should be hidden behind MPAM's resctrl_arch_rmid_read(). > > resctrl_arch_rmid_read() may be called via IPI meaning it is unable > to sleep. In this case resctrl_arch_rmid_read() should return an error > if it needs to sleep. This will only affect MPAM platforms where > the cache occupancy counter isn't available immediately, nohz_full is > in use, and there are there are no housekeeping CPUs in the necessary
:%s/there are there are/there are/
Thanks
Babu
> domain. > > There are three callers of resctrl_arch_rmid_read(): > __mon_event_count() and __check_limbo() are both called from a > non-migrateable context. mon_event_read() invokes __mon_event_count() > using smp_call_on_cpu(), which adds work to the target CPUs workqueue. > rdtgroup_mutex() is held, meaning this cannot race with the resctrl > cpuhp callback. __check_limbo() is invoked via schedule_delayed_work_on() > also adds work to a per-cpu workqueue. > > The remaining call is add_rmid_to_limbo() which is called in response > to a user-space syscall that frees an RMID. This opportunistically > reads the LLC occupancy counter on the current domain to see if the > RMID is over the dirty threshold. This has to disable preemption to > avoid reading the wrong domain's value. Disabling pre-emption here > prevents resctrl_arch_rmid_read() from sleeping. > > add_rmid_to_limbo() walks each domain, but only reads the counter > on one domain. If the system has more than one domain, the RMID will > always be added to the limbo list. If the RMIDs usage was not over the > threshold, it will be removed from the list when __check_limbo() runs. > Make this the default behaviour. Free RMIDs are always added to the > limbo list for each domain. > > The user visible effect of this is that a clean RMID is not available > for re-allocation immediately after 'rmdir()' completes, this behaviour > was never portable as it never happened on a machine with multiple > domains. > > Removing this path allows resctrl_arch_rmid_read() to sleep if its called > with interrupts unmasked. Document this is the expected behaviour, and > add a might_sleep() annotation to catch changes that won't work on arm64. > > 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> > --- > The previous version allowed resctrl_arch_rmid_read() to be called on the > wrong CPUs, but now that this needs to take nohz_full and housekeeping into > account, its too complex. > > Changes since v3: > * Removed error handling for smp_call_function_any(), this can't race > with the cpuhp callbacks as both hold rdtgroup_mutex. > * Switched to the alternative of removing the counter read, this simplifies > things dramatically. > > Changes since v4: > * Messed with capitalisation. > * Removed some dead code now that entry->busy will never be zero in > add_rmid_to_limbo(). > * Rephrased the comment above resctrl_arch_rmid_read_context_check(). > --- > arch/x86/kernel/cpu/resctrl/monitor.c | 25 +++++-------------------- > include/linux/resctrl.h | 18 +++++++++++++++++- > 2 files changed, 22 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index 7749e6569a4a..05d949ec94f1 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -278,6 +278,8 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > u64 msr_val, chunks; > int ret; > > + resctrl_arch_rmid_read_context_check(); > + > if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask)) > return -EINVAL; > > @@ -454,8 +456,6 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > { > struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; > struct rdt_domain *d; > - int cpu, err; > - u64 val = 0; > u32 idx; > > lockdep_assert_held(&rdtgroup_mutex); > @@ -463,17 +463,7 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid); > > entry->busy = 0; > - cpu = get_cpu(); > list_for_each_entry(d, &r->domains, list) { > - if (cpumask_test_cpu(cpu, &d->cpu_mask)) { > - err = resctrl_arch_rmid_read(r, d, entry->closid, > - entry->rmid, > - QOS_L3_OCCUP_EVENT_ID, > - &val); > - if (err || val <= resctrl_rmid_realloc_threshold) > - continue; > - } > - > /* > * For the first limbo RMID in the domain, > * setup up the limbo worker. > @@ -483,15 +473,10 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > set_bit(idx, d->rmid_busy_llc); > entry->busy++; > } > - put_cpu(); > > - if (entry->busy) { > - rmid_limbo_count++; > - if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > - closid_num_dirty_rmid[entry->closid]++; > - } else { > - list_add_tail(&entry->list, &rmid_free_lru); > - } > + rmid_limbo_count++; > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + closid_num_dirty_rmid[entry->closid]++; > } > > void free_rmid(u32 closid, u32 rmid) > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h > index 660752406174..f7311102e94c 100644 > --- a/include/linux/resctrl.h > +++ b/include/linux/resctrl.h > @@ -236,7 +236,12 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d); > * @eventid: eventid to read, e.g. L3 occupancy. > * @val: result of the counter read in bytes. > * > - * Call from process context on a CPU that belongs to domain @d. > + * Some architectures need to sleep when first programming some of the counters. > + * (specifically: arm64's MPAM cache occupancy counters can return 'not ready' > + * for a short period of time). Call from a non-migrateable process context on > + * a CPU that belongs to domain @d. e.g. use smp_call_on_cpu() or > + * schedule_work_on(). This function can be called with interrupts masked, > + * e.g. using smp_call_function_any(), but may consistently return an error. > * > * Return: > * 0 on success, or -EIO, -EINVAL etc on error. > @@ -245,6 +250,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > u32 closid, u32 rmid, enum resctrl_event_id eventid, > u64 *val); > > +/** > + * resctrl_arch_rmid_read_context_check() - warn about invalid contexts > + * > + * When built with CONFIG_DEBUG_ATOMIC_SLEEP generate a warning when > + * resctrl_arch_rmid_read() is called with preemption disabled. > + */ > +static inline void resctrl_arch_rmid_read_context_check(void) > +{ > + if (!irqs_disabled()) > + might_sleep(); > +} > > /** > * resctrl_arch_reset_rmid() - Reset any private state associated with rmid
| |