Re: [PATCH v3 01/38] x86/resctrl: Fix allocation of cleanest CLOSID on platforms with no monitors

From: Reinette Chatre
Date: Fri Jun 28 2024 - 12:42:04 EST


Hi James,

On 6/14/24 7:59 AM, James Morse wrote:
commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
searching closid_num_dirty_rmid") added logic that causes resctrl to

There is a custom in x86 on how messages containing references to commits
are formatted. For reference you can see how the recent fix
739c9765793e ("x86/resctrl: Don't try to free nonexistent RMIDs") was
reformatted before merge. Applied to this patch, it may look something
like:

Commit

6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by searching closid_num_dirty_rmid")

added logic ...

Looking at, for example, commit e3ca96e479c9 ("x86/resctrl: Pass domain to
target CPU"), it does seem ok to split the name of the commit.


search for the CLOSID with the fewest dirty cache lines when creating a
new control group, if requested by the arch code. This depends on the
values read from the llc_occupancy counters. The logic is applicable to
architectures where the CLOSID effectively forms part of the monitoring
identifier and so do not allow complete freedom to choose an unused
monitoring identifier for a given CLOSID.

This support missed that some platforms may not have these counters.
This causes a NULL pointer dereference when creating a new control
group as the array was not allocated by dom_data_init().

As this feature isn't necessary on platforms that don't have cache
occupancy monitors, add this to the check that occurs when a new
control group is allocated.


The snippet below does not belong in changelog and can be moved to
maintainer notes.

The existing code is not selected by any upstream platform, it makes
no sense to backport this patch to stable.

Fixes: 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by searching closid_num_dirty_rmid")
Signed-off-by: James Morse <james.morse@xxxxxxx>
Tested-by: Peter Newman <peternewman@xxxxxxxxxx>
Tested-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>

---
Changes since v1:
* [Commit message only] Reword the first paragraph to make it clear
that the issue being fixed wasn't directly associated with addition
of a Kconfig option. (Actually, the option is not in Kconfig yet,
and gets added later in this series.)
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 02f213f1c51c..d02f4c97e40f 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -149,7 +149,8 @@ static int closid_alloc(void)
lockdep_assert_held(&rdtgroup_mutex);
- if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
+ if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
+ is_llc_occupancy_enabled()) {
cleanest_closid = resctrl_find_cleanest_closid();
if (cleanest_closid < 0)
return cleanest_closid;

With changelog updated:

| Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>

Reinette