Re: [PATCH v1 7/9] x86/resctrl: Assign HW RMIDs to CPUs for soft RMID

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

Hi Peter,

On 4/21/2023 7:17 AM, Peter Newman wrote:
> To implement soft RMIDs, each CPU needs a HW RMID that is unique within
> its L3 cache domain. This is the minimum number of RMIDs needed to
> monitor all CPUs.
> 
> This is accomplished by determining the rank of each CPU's mask bit
> within its L3 shared_cpu_mask in resctrl_online_cpu().
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c | 39 +++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 47b1c37a81f8..b0d873231b1e 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -596,6 +596,38 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
>  	}
>  }
>  
> +/* Assign each CPU an RMID that is unique within its cache domain. */
> +static u32 determine_hw_rmid_for_cpu(int cpu)

This code tends to use the verb "get", something like "get_hw_rmid()"
could work.

> +{
> +	struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
> +	struct cacheinfo *l3ci = NULL;
> +	u32 rmid;
> +	int i;
> +
> +	/* Locate the cacheinfo for this CPU's L3 cache. */
> +	for (i = 0; i < ci->num_leaves; i++) {
> +		if (ci->info_list[i].level == 3 &&
> +		    (ci->info_list[i].attributes & CACHE_ID)) {
> +			l3ci = &ci->info_list[i];
> +			break;
> +		}
> +	}
> +	WARN_ON(!l3ci);
> +
> +	if (!l3ci)
> +		return 0;

You can use "if (WARN_ON(..))"

> +
> +	/* Use the position of cpu in its shared_cpu_mask as its RMID. */

(please use "CPU" instead of "cpu" in comments and changelogs)

> +	rmid = 0;
> +	for_each_cpu(i, &l3ci->shared_cpu_map) {
> +		if (i == cpu)
> +			break;
> +		rmid++;
> +	}
> +
> +	return rmid;
> +}

I do not see any impact to the (soft) RMIDs that can be assigned to monitor
groups, yet from what I understand a generic "RMID" is used as index to MBM state.
Is this correct? A hardware RMID and software RMID would thus share the
same MBM state. If this is correct I think we need to work on making
the boundaries between hard and soft RMID more clear.

> +
>  static void clear_closid_rmid(int cpu)
>  {
>  	struct resctrl_pqr_state *state = this_cpu_ptr(&pqr_state);
> @@ -604,7 +636,12 @@ static void clear_closid_rmid(int cpu)
>  	state->default_rmid = 0;
>  	state->cur_closid = 0;
>  	state->cur_rmid = 0;
> -	wrmsr(MSR_IA32_PQR_ASSOC, 0, 0);
> +	state->hw_rmid = 0;
> +
> +	if (static_branch_likely(&rdt_soft_rmid_enable_key))
> +		state->hw_rmid = determine_hw_rmid_for_cpu(cpu);
> +
> +	wrmsr(MSR_IA32_PQR_ASSOC, state->hw_rmid, 0);
>  }
>  
>  static int resctrl_online_cpu(unsigned int cpu)

Reinette