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.
Reviewed-by: Shaopeng Tan <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Tested-By: Peter Newman <[email protected]>
Signed-off-by: James Morse <[email protected]>
---
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.
Changes since v5:
* Added braces after an else.
* Made closid_num_dirty_rmid an unsigned int.
* Moved mutex_lock() in dom_data_init() to cover the whole function.
---
arch/x86/kernel/cpu/resctrl/monitor.c | 66 +++++++++++++++++++++++----
1 file changed, 56 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index d286aba1ee63..0c783301d106 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -51,6 +51,13 @@ struct rmid_entry {
*/
static LIST_HEAD(rmid_free_lru);
+/**
+ * @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 unsigned int *closid_num_dirty_rmid;
+
/**
* @rmid_limbo_count count of currently unused but (potentially)
* dirty RMIDs.
@@ -293,6 +300,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]--;
+}
+
/*
* 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
@@ -329,10 +347,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;
}
@@ -400,6 +416,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;
@@ -425,10 +443,13 @@ 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]++;
+ } else {
list_add_tail(&entry->list, &rmid_free_lru);
+ }
}
void free_rmid(u32 closid, u32 rmid)
@@ -796,13 +817,30 @@ 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;
+ int err = 0, i;
u32 idx;
- int i;
+
+ mutex_lock(&rdtgroup_mutex);
+ if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
+ int *tmp;
+
+ tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL);
+ if (!tmp) {
+ err = -ENOMEM;
+ goto out_unlock;
+ }
+
+ closid_num_dirty_rmid = tmp;
+ }
rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL);
- if (!rmid_ptrs)
- return -ENOMEM;
+ if (!rmid_ptrs) {
+ kfree(closid_num_dirty_rmid);
+ err = -ENOMEM;
+ goto out_unlock;
+ }
for (i = 0; i < idx_limit; i++) {
entry = &rmid_ptrs[i];
@@ -822,13 +860,21 @@ static int dom_data_init(struct rdt_resource *r)
entry = __rmid_entry(idx);
list_del(&entry->list);
- return 0;
+out_unlock:
+ mutex_unlock(&rdtgroup_mutex);
+
+ return err;
}
void resctrl_exit_mon_l3_config(struct rdt_resource *r)
{
mutex_lock(&rdtgroup_mutex);
+ if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
+ kfree(closid_num_dirty_rmid);
+ closid_num_dirty_rmid = NULL;
+ }
+
kfree(rmid_ptrs);
rmid_ptrs = NULL;
--
2.39.2
Hi James,
On 9/14/2023 10:21 AM, James Morse wrote:
...
> @@ -796,13 +817,30 @@ 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;
> + int err = 0, i;
> u32 idx;
> - int i;
> +
> + mutex_lock(&rdtgroup_mutex);
> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> + int *tmp;
> +
> + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL);
Shouldn't this rather be sizeof(unsigned int) to match the type it will store?
> + if (!tmp) {
> + err = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + closid_num_dirty_rmid = tmp;
> + }
>
> rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL);
> - if (!rmid_ptrs)
> - return -ENOMEM;
> + if (!rmid_ptrs) {
> + kfree(closid_num_dirty_rmid);
> + err = -ENOMEM;
> + goto out_unlock;
> + }
>
> for (i = 0; i < idx_limit; i++) {
> entry = &rmid_ptrs[i];
> @@ -822,13 +860,21 @@ static int dom_data_init(struct rdt_resource *r)
> entry = __rmid_entry(idx);
> list_del(&entry->list);
>
> - return 0;
> +out_unlock:
> + mutex_unlock(&rdtgroup_mutex);
> +
> + return err;
> }
>
> void resctrl_exit_mon_l3_config(struct rdt_resource *r)
> {
> mutex_lock(&rdtgroup_mutex);
>
> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> + kfree(closid_num_dirty_rmid);
> + closid_num_dirty_rmid = NULL;
> + }
> +
> kfree(rmid_ptrs);
> rmid_ptrs = NULL;
>
Awaiting response on patch #2 related to above hunk.
Reinette
Hi Reinette,
On 03/10/2023 22:13, Reinette Chatre wrote:
> On 9/14/2023 10:21 AM, James Morse wrote:
>> @@ -796,13 +817,30 @@ 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;
>> + int err = 0, i;
>> u32 idx;
>> - int i;
>> +
>> + mutex_lock(&rdtgroup_mutex);
>> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>> + int *tmp;
>> +
>> + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL);
>
> Shouldn't this rather be sizeof(unsigned int) to match the type it will store?
It matches the type of tmp... I'll change both closid_num_dirty_rmid and tmp to a u32 *,
and this sizeof() to be sizeof(*tmp).
>> + if (!tmp) {
>> + err = -ENOMEM;
>> + goto out_unlock;
>> + }
>> +
>> + closid_num_dirty_rmid = tmp;
>> + }
>>
>> rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL);
>> - if (!rmid_ptrs)
>> - return -ENOMEM;
>> + if (!rmid_ptrs) {
>> + kfree(closid_num_dirty_rmid);
>> + err = -ENOMEM;
>> + goto out_unlock;
>> + }
>>
>> for (i = 0; i < idx_limit; i++) {
>> entry = &rmid_ptrs[i];
>> @@ -822,13 +860,21 @@ static int dom_data_init(struct rdt_resource *r)
>> entry = __rmid_entry(idx);
>> list_del(&entry->list);
>>
>> - return 0;
>> +out_unlock:
>> + mutex_unlock(&rdtgroup_mutex);
>> +
>> + return err;
>> }
>>
>> void resctrl_exit_mon_l3_config(struct rdt_resource *r)
>> {
>> mutex_lock(&rdtgroup_mutex);
>>
>> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>> + kfree(closid_num_dirty_rmid);
>> + closid_num_dirty_rmid = NULL;
>> + }
>> +
>> kfree(rmid_ptrs);
>> rmid_ptrs = NULL;
>>
>
> Awaiting response on patch #2 related to above hunk.
It's the same story here. CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID makes this behaviour
visible to the filesystem code, which means the filesystem code can do the alloc/free of
this array. All this eventually moves out to /fs/.
This is all because the RMID allocation is dependent on the limbo list that resctrl
manages, and for MPAM the CLOSID is too. I'm sure its simpler to expose this MPAM
behaviour to resctrl - and in a way that the compiler can remove if its not needed. The
alternative would be to duplicate the allocators on each architecture. I don't think MPAM
is different enough to justify this.
Thanks,
James