diff mbox series

[v4,06/24] x86/resctrl: Track the number of dirty RMID a CLOSID has

Message ID 20230525180209.19497-7-james.morse@arm.com
State New
Headers
Series x86/resctrl: monitored closid+rmid together, separate arch/fs locking |

Commit Message

James Morse May 25, 2023, 6:01 p.m. UTC 
  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>
---
 arch/x86/kernel/cpu/resctrl/monitor.c | 43 +++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 5 deletions(-)

Comments

Reinette Chatre June 15, 2023, 10:08 p.m. UTC | #1 
Hi James,

On 5/25/2023 11:01 AM, James Morse wrote:

...

> @@ -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;
> @@ -420,6 +438,9 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>  		rmid_limbo_count++;
>  	else
>  		list_add_tail(&entry->list, &rmid_free_lru);
> +
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
> +		closid_num_dirty_rmid[entry->closid]++;

Wouldn't this always increment the counter, whether the entry is
dirty or not? (Although ... the later change where entries are
always dirty may make this correct ... although I would still
expect the if statement that precedes it to change).

Reinette
James Morse July 28, 2023, 4:35 p.m. UTC | #2 
Hi Reinette,

On 6/15/23 23:08, Reinette Chatre wrote:
> On 5/25/2023 11:01 AM, James Morse wrote:
>> @@ -420,6 +438,9 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>>   		rmid_limbo_count++;
>>   	else
>>   		list_add_tail(&entry->list, &rmid_free_lru);
>> +
>> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>> +		closid_num_dirty_rmid[entry->closid]++;
   
> Wouldn't this always increment the counter, whether the entry is
> dirty or not? (Although ... the later change where entries are
> always dirty may make this correct ... although I would still
> expect the if statement that precedes it to change).

True, I was expecting add_rmid_to_limbo() to always transiently add CLOSID to limbo,
hence this is unconditional, but you're right its optional - and this could cause everything
to pile up in the list when the limbo handler isn't running.
I'll add a check on entry->busy, and move this into the above if case.


Thanks,

James
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 27e731c7de72..1e7fa40ee471 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);
 
+/**
+ * @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;
+
 /**
  * @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]--;
+}
+
 /*
  * 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;
@@ -420,6 +438,9 @@  static void add_rmid_to_limbo(struct rmid_entry *entry)
 		rmid_limbo_count++;
 	else
 		list_add_tail(&entry->list, &rmid_free_lru);
+
+	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
+		closid_num_dirty_rmid[entry->closid]++;
 }
 
 void free_rmid(u32 closid, u32 rmid)
@@ -781,13 +802,25 @@  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;
 
+	lockdep_assert_held(&rdtgroup_mutex);
+
+	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
+		closid_num_dirty_rmid = kcalloc(num_closid, sizeof(int),
+						GFP_KERNEL);
+		if (!closid_num_dirty_rmid)
+			return -ENOMEM;
+	}
+
 	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];