Messages in this thread | | | Date | Mon, 14 Aug 2023 18:22:04 -0700 | Subject | Re: [PATCH v5 05/24] x86/resctrl: Allow RMID allocation to be scoped by CLOSID | From | Fenghua Yu <> |
| |
Hi, James,
On 7/28/23 09:42, James Morse wrote: > MPAMs RMID values are not unique unless the CLOSID is considered as well. > > alloc_rmid() expects the RMID to be an independent number. > > Pass the CLOSID in to alloc_rmid(). Use this to compare indexes when > allocating. If the CLOSID is not relevant to the index, this ends up > comparing the free RMID with itself, and the first free entry will be > used. With MPAM the CLOSID is included in the index, so this becomes a > walk of the free RMID entries, until one that matches the supplied > CLOSID is found. > > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v2; > * Rephrased comment in resctrl_find_free_rmid() to describe this in terms of > list_entry_first() > * Rephrased comment above alloc_rmid() > > Changes since v3: > * Flipped conditions in alloc_rmid() > > Changes since v4: > * Typo in comment > --- > arch/x86/kernel/cpu/resctrl/internal.h | 2 +- > arch/x86/kernel/cpu/resctrl/monitor.c | 51 +++++++++++++++++------ > arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 2 +- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 2 +- > 4 files changed, 41 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index b48715bb8762..94749ee950dd 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -535,7 +535,7 @@ void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp); > struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r); > int closids_supported(void); > void closid_free(int closid); > -int alloc_rmid(void); > +int alloc_rmid(u32 closid); > void free_rmid(u32 closid, u32 rmid); > int rdt_get_mon_l3_config(struct rdt_resource *r); > bool __init rdt_cpu_has(int flag); > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index bd234b66dddf..de91ca781d9f 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -337,24 +337,49 @@ bool has_busy_rmid(struct rdt_domain *d) > return find_first_bit(d->rmid_busy_llc, idx_limit) != idx_limit; > } > > -/* > - * As of now the RMIDs allocation is global. > - * However we keep track of which packages the RMIDs > - * are used to optimize the limbo list management. > - */ > -int alloc_rmid(void) > +static struct rmid_entry *resctrl_find_free_rmid(u32 closid) > { > - struct rmid_entry *entry; > - > - lockdep_assert_held(&rdtgroup_mutex); > + struct rmid_entry *itr; > + u32 itr_idx, cmp_idx; > > if (list_empty(&rmid_free_lru)) > - return rmid_limbo_count ? -EBUSY : -ENOSPC; > + return rmid_limbo_count ? ERR_PTR(-EBUSY) : ERR_PTR(-ENOSPC); > + > + list_for_each_entry(itr, &rmid_free_lru, list) { > + /* > + * Get the index of this free RMID, and the index it would need > + * to be if it were used with this CLOSID. > + * If the CLOSID is irrelevant on this architecture, these will > + * always be the same meaning the compiler can reduce this loop > + * to a single list_entry_first() call.
s/list_entry_first()/list_first_entry()/?
Seems the comment is not accurate because the loop is not really reduced to a single list_first_entry() call. Getting itr_idx and cmp_idx and comparing them are extra operations that list_first_entry() doesn't have.
Maybe change the second half comment to something like:
If the CLOSID is irrelevant on this architecture, the two index values are always same on every entry and thus the very first entry will be returned.
> + */ > + itr_idx = resctrl_arch_rmid_idx_encode(itr->closid, itr->rmid); > + cmp_idx = resctrl_arch_rmid_idx_encode(closid, itr->rmid); > + > + if (itr_idx == cmp_idx) > + return itr; > + } > + > + return ERR_PTR(-ENOSPC); > +} > + > +/* > + * For MPAM the RMID value is not unique, and has to be considered with > + * the CLOSID. The (CLOSID, RMID) pair is allocated on all domains, which > + * allows all domains to be managed by a single limbo list. > + * Each domain also has a rmid_busy_llc to reduce the work of the limbo handler. > + */ > +int alloc_rmid(u32 closid) > +{ > + struct rmid_entry *entry; > + > + lockdep_assert_held(&rdtgroup_mutex); > + > + entry = resctrl_find_free_rmid(closid); > + if (IS_ERR(entry)) > + return PTR_ERR(entry); > > - entry = list_first_entry(&rmid_free_lru, > - struct rmid_entry, list); > list_del(&entry->list); > - > return entry->rmid; > } > > diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c > index aeadaeb5df9a..5ebd6e54c7f2 100644 > --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c > +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c > @@ -763,7 +763,7 @@ int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp) > int ret; > > if (rdt_mon_capable) { > - ret = alloc_rmid(); > + ret = alloc_rmid(rdtgrp->closid); > if (ret < 0) { > rdt_last_cmd_puts("Out of RMIDs\n"); > return ret; > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 7c5cfb373d03..b97e119dbe46 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -3172,7 +3172,7 @@ static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp) > if (!rdt_mon_capable) > return 0; > > - ret = alloc_rmid(); > + ret = alloc_rmid(rdtgrp->closid); > if (ret < 0) { > rdt_last_cmd_puts("Out of RMIDs\n"); > return ret;
Thanks.
-Fenghua
| |