Re: [PATCH v5 06/24] x86/resctrl: Track the number of dirty RMID a CLOSID has

From: Fenghua Yu
Date: Mon Aug 14 2023 - 22:38:31 EST


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@xxxxxxx>
---
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