Messages in this thread | | | Date | Thu, 9 Nov 2023 09:47:32 -0800 | Subject | Re: [PATCH v7 14/24] x86/resctrl: Allow resctrl_arch_rmid_read() to sleep | From | Reinette Chatre <> |
| |
Hi James,
On 10/25/2023 11:03 AM, 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 no housekeeping CPUs in the necessary 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. > > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Tested-by: Peter Newman <peternewman@google.com> > Reviewed-by: Shaopeng Tan <tan.shaopeng@fujitsu.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(). > > Changes since v5: > * Really rephrased the comment above resctrl_arch_rmid_read_context_check(). > > No changes since v6
If I trusted this I would not have taken the time to review this patch.
> ---
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Reinette
| |