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

[Reinette Chatre <reinette.chatre@intel.com>]

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@arm.com>
> Tested-by: Peter Newman <peternewman@google.com>
> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> ---
> 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@intel.com>

Reinette