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 <[email protected]>
Signed-off-by: James Morse <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 2 +-
arch/x86/kernel/cpu/resctrl/monitor.c | 44 ++++++++++++++++++-----
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 2 +-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 2 +-
4 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index af71401c57e2..013c8fc9fd28 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -510,7 +510,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);
void mon_event_count(void *info);
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index dbae380e3d1c..347be3767241 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -301,25 +301,51 @@ bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d)
return find_first_bit(d->rmid_busy_llc, idx_limit) != idx_limit;
}
+static struct rmid_entry *resctrl_find_free_rmid(u32 closid)
+{
+ struct rmid_entry *itr;
+ u32 itr_idx, cmp_idx;
+
+ if (list_empty(&rmid_free_lru))
+ 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. Otherwise they will only match if this
+ * RMID can be used with this CLOSID.
+ */
+ 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);
+}
+
/*
- * As of now the RMIDs allocation is global.
+ * As of now the RMIDs allocation is the same in each domain.
* However we keep track of which packages the RMIDs
* are used to optimize the limbo list management.
+ * The closid is ignored on x86.
*/
-int alloc_rmid(void)
+int alloc_rmid(u32 closid)
{
struct rmid_entry *entry;
lockdep_assert_held(&rdtgroup_mutex);
- if (list_empty(&rmid_free_lru))
- return rmid_limbo_count ? -EBUSY : -ENOSPC;
+ entry = resctrl_find_free_rmid(closid);
+ if (!IS_ERR(entry)) {
+ list_del(&entry->list);
+ return entry->rmid;
+ }
- entry = list_first_entry(&rmid_free_lru,
- struct rmid_entry, list);
- list_del(&entry->list);
-
- return entry->rmid;
+ return PTR_ERR(entry);
}
static void add_rmid_to_limbo(struct rmid_entry *entry)
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index c51932516965..3b724a40d3a2 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 c67083a8a5f5..ac88610a6946 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2875,7 +2875,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;
--
2.30.2
Hi, James,
> 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 <[email protected]>
> Signed-off-by: James Morse <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 2 +-
> arch/x86/kernel/cpu/resctrl/monitor.c | 44 ++++++++++++++++++-----
> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 2 +-
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 2 +-
> 4 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index af71401c57e2..013c8fc9fd28 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -510,7 +510,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); void mon_event_count(void
> *info); diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
> b/arch/x86/kernel/cpu/resctrl/monitor.c
> index dbae380e3d1c..347be3767241 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -301,25 +301,51 @@ bool has_busy_rmid(struct rdt_resource *r, struct
> rdt_domain *d)
> return find_first_bit(d->rmid_busy_llc, idx_limit) != idx_limit; }
>
> +static struct rmid_entry *resctrl_find_free_rmid(u32 closid) {
> + struct rmid_entry *itr;
> + u32 itr_idx, cmp_idx;
> +
> + if (list_empty(&rmid_free_lru))
> + 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. Otherwise they will only match if this
> + * RMID can be used with this CLOSID.
> + */
> + 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;
Finding free rmid may be called frequently depending on usage.
It would be better to have a simpler and faster arch helper that finds the itr on x86.
Something like:
struct rmid_entry *resctrl_arch_rmid_matchd(u32 ignored, u32 ignored)
{
return list_entry_first(resctrl_free_lru, itr, list);
}
Arm64 implements the complex case going through the rmid_free_lru list in the patch.
> + }
> +
> + return ERR_PTR(-ENOSPC);
> +}
> +
> /*
> - * As of now the RMIDs allocation is global.
> + * As of now the RMIDs allocation is the same in each domain.
> * However we keep track of which packages the RMIDs
> * are used to optimize the limbo list management.
> + * The closid is ignored on x86.
> */
> -int alloc_rmid(void)
> +int alloc_rmid(u32 closid)
> {
> struct rmid_entry *entry;
>
> lockdep_assert_held(&rdtgroup_mutex);
>
> - if (list_empty(&rmid_free_lru))
> - return rmid_limbo_count ? -EBUSY : -ENOSPC;
> + entry = resctrl_find_free_rmid(closid);
> + if (!IS_ERR(entry)) {
> + list_del(&entry->list);
> + return entry->rmid;
> + }
>
> - entry = list_first_entry(&rmid_free_lru,
> - struct rmid_entry, list);
> - list_del(&entry->list);
> -
> - return entry->rmid;
> + return PTR_ERR(entry);
> }
>
> static void add_rmid_to_limbo(struct rmid_entry *entry) diff --git
> a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index c51932516965..3b724a40d3a2 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 c67083a8a5f5..ac88610a6946 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2875,7 +2875,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;
> --
> 2.30.2
Thanks.
-Fenghua
Hi James,
On 1/13/2023 9:54 AM, 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 <[email protected]>
> Signed-off-by: James Morse <[email protected]>
...
> /*
> - * As of now the RMIDs allocation is global.
> + * As of now the RMIDs allocation is the same in each domain.
Could you please elaborate what is meant/intended with this change
(global vs per domain)? From the changelog a comment that RMID
allocation is the same in each resource group for MPAM may be
expected but per domain is not clear to me.
Reinette
Hi Fenghua,
On 17/01/2023 18:53, Yu, Fenghua 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.
>> b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index dbae380e3d1c..347be3767241 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -301,25 +301,51 @@ bool has_busy_rmid(struct rdt_resource *r, struct
>> rdt_domain *d)
>> return find_first_bit(d->rmid_busy_llc, idx_limit) != idx_limit; }
>>
>> +static struct rmid_entry *resctrl_find_free_rmid(u32 closid) {
>> + struct rmid_entry *itr;
>> + u32 itr_idx, cmp_idx;
>> +
>> + if (list_empty(&rmid_free_lru))
>> + 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. Otherwise they will only match if this
>> + * RMID can be used with this CLOSID.
>> + */
>> + 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;
>
> Finding free rmid may be called frequently depending on usage.
>
> It would be better to have a simpler and faster arch helper that finds the itr on x86.
> Something like:
> struct rmid_entry *resctrl_arch_rmid_matchd(u32 ignored, u32 ignored)
> {
> return list_entry_first(resctrl_free_lru, itr, list);
> }
>
> Arm64 implements the complex case going through the rmid_free_lru list in the patch.
The trick here is that one degenerates into the other:
>> + list_for_each_entry(itr, &rmid_free_lru, list) {
The first time round the loop, this is equivalent to:
| itr = list_entry_first(&rmid_free_lru, itr, 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. Otherwise they will only match if this
>> + * RMID can be used with this CLOSID.
>> + */
>> + itr_idx = resctrl_arch_rmid_idx_encode(itr->closid, itr->rmid);
On x86, after inline-ing this is:
| itr_idx = itr->rmid
>> + cmp_idx = resctrl_arch_rmid_idx_encode(closid, itr->rmid);
and this is:
| cmp_idx = itr->rmid
>> + if (itr_idx == cmp_idx)
>> + return itr;
So now any half decent compiler can spot that this condition is always true and the loop
only ever runs once, and the whole thing reduces to what you wanted it to be.
This saves exposing things that should be private to the filesystem code and having
per-arch helpers to mess with it.
The commit message described this, I'll expand the comment in the loop to be:
| * 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.
Thanks,
James
Hi Reinette,
On 02/02/2023 23:45, Reinette Chatre wrote:
> On 1/13/2023 9:54 AM, 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 <[email protected]>
>> Signed-off-by: James Morse <[email protected]>
>
> ...
>
>> /*
>> - * As of now the RMIDs allocation is global.
>> + * As of now the RMIDs allocation is the same in each domain.
> Could you please elaborate what is meant/intended with this change
> (global vs per domain)? From the changelog a comment that RMID
> allocation is the same in each resource group for MPAM may be
> expected but per domain is not clear to me.
This is badly worded. It's referring to the limbo list management, while RMID=7 isn't
unique on MPAM, the struct rmid_entry used in two domains will be the same because the
CLOSID doesn't change. This means its still sufficient to move around the struct
rmid_entry to manage the limbo list.
I think this had me confused because 'as of now' implies the RMID won't always be globally
allocated, and MPAM has non-unique RMID/PMG values which are a different kind of global.
I'll change this to read:
/*
* 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.
*/
(seeing as the function doesn't touch rmid_budy_llc, or refer to it by name)
Thanks,
James
Hi James,
On 3/3/2023 10:34 AM, James Morse wrote:
> On 02/02/2023 23:45, Reinette Chatre wrote:
>> On 1/13/2023 9:54 AM, James Morse wrote:
...
>>> /*
>>> - * As of now the RMIDs allocation is global.
>>> + * As of now the RMIDs allocation is the same in each domain.
>
>> Could you please elaborate what is meant/intended with this change
>> (global vs per domain)? From the changelog a comment that RMID
>> allocation is the same in each resource group for MPAM may be
>> expected but per domain is not clear to me.
>
> This is badly worded. It's referring to the limbo list management, while RMID=7 isn't
> unique on MPAM, the struct rmid_entry used in two domains will be the same because the
> CLOSID doesn't change. This means its still sufficient to move around the struct
> rmid_entry to manage the limbo list.
>
> I think this had me confused because 'as of now' implies the RMID won't always be globally
> allocated, and MPAM has non-unique RMID/PMG values which are a different kind of global.
>
>
> I'll change this to read:
> /*
> * 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.
> */
>
> (seeing as the function doesn't touch rmid_budy_llc, or refer to it by name)
>
Thank you. This is easier to understand.
Reinette