Messages in this thread Patch in this message | | | Date | Mon, 14 Aug 2023 19:37:35 -0700 | Subject | Re: [PATCH v5 06/24] x86/resctrl: Track the number of dirty RMID a CLOSID has | From | Fenghua Yu <> |
| |
Hi, James,
On 7/28/23 09:42, James Morse wrote: > MPAM's PMG bits extend its PARTID space, meaning the same PMG value can be > used for different control groups. > > This means once a CLOSID is allocated, all its monitoring ids may still be > dirty, and held in limbo. > > Keep track of the number of RMID held in limbo each CLOSID has. This will > allow a future helper to find the 'cleanest' CLOSID when allocating. > > The array is only needed when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is > defined. This will never be the case on x86. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v4: > * Moved closid_num_dirty_rmid[] update under entry->busy check > * Take the mutex in dom_data_init() as the caller doesn't. > --- > arch/x86/kernel/cpu/resctrl/monitor.c | 49 +++++++++++++++++++++++---- > 1 file changed, 42 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index de91ca781d9f..44addc0126fc 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -43,6 +43,13 @@ struct rmid_entry { > */ > static LIST_HEAD(rmid_free_lru); >
Better to add:
#if CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID > +/** > + * @closid_num_dirty_rmid The number of dirty RMID each CLOSID has. > + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined. > + * Indexed by CLOSID. Protected by rdtgroup_mutex. > + */ > +static int *closid_num_dirty_rmid; #endif
Then the global variable won't exist on x86 to avoid confusion and space.
Some code related to the CONFIG also needs to be changed accordingly.
> + > /** > * @rmid_limbo_count count of currently unused but (potentially) > * dirty RMIDs. > @@ -285,6 +292,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > return 0; > } > > +static void limbo_release_entry(struct rmid_entry *entry) > +{ > + lockdep_assert_held(&rdtgroup_mutex); > + > + rmid_limbo_count--; > + list_add_tail(&entry->list, &rmid_free_lru); > + > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + closid_num_dirty_rmid[entry->closid]--;
Maybe define some helpers (along with other similar ones) in resctrl.h like this:
#ifdef CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID static inline void closid_num_dirty_rmid_dec(struct rmid_entry *entry) { closid_num_dirty_rmid[entry->closid]--; } ... #else static inline void closid_num_dirty_rmid_dec(struct rmid_entry *unused) { } ... #endif
Then directly call the helper here:
+ closid_num_dirty_rmid_dec(entry);
On x86 this is noop without occupy any space and cleaner code.
> +} > + > /* > * Check the RMIDs that are marked as busy for this domain. If the > * reported LLC occupancy is below the threshold clear the busy bit and > @@ -321,10 +339,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > > if (force_free || !rmid_dirty) { > clear_bit(idx, d->rmid_busy_llc); > - if (!--entry->busy) { > - rmid_limbo_count--; > - list_add_tail(&entry->list, &rmid_free_lru); > - } > + if (!--entry->busy) > + limbo_release_entry(entry); > } > cur_idx = idx + 1; > } > @@ -391,6 +407,8 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > u64 val = 0; > u32 idx; > > + lockdep_assert_held(&rdtgroup_mutex); > + > idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid); > > entry->busy = 0; > @@ -416,9 +434,11 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > } > put_cpu(); > > - if (entry->busy) > + if (entry->busy) { > rmid_limbo_count++; > - else > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + closid_num_dirty_rmid[entry->closid]++;
Ditto.
> + } else > list_add_tail(&entry->list, &rmid_free_lru); > } > > @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms) > static int dom_data_init(struct rdt_resource *r) > { > u32 idx_limit = resctrl_arch_system_num_rmid_idx(); > + u32 num_closid = resctrl_arch_get_num_closid(r); > struct rmid_entry *entry = NULL; > u32 idx; > int i; > > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { > + int *tmp; > + > + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL); > + if (!tmp) > + return -ENOMEM; > + > + mutex_lock(&rdtgroup_mutex);
data_init() is called in __init. No need to lock here, right?
> + closid_num_dirty_rmid = tmp; > + mutex_unlock(&rdtgroup_mutex); > + } > +
This code is also can be defined as a helper in resctrl.h.
> rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL); > - if (!rmid_ptrs) > + if (!rmid_ptrs) { > + kfree(closid_num_dirty_rmid); > return -ENOMEM; > + } > > for (i = 0; i < idx_limit; i++) { > entry = &rmid_ptrs[i];
Thanks.
-Fenghua
| |