Re: [PATCH v12 15/25] x86,fs/resctrl: Handle systems where AET is the only resource

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

Hi Tony,

On 9/16/26 4:13 PM, Tony Luck wrote:
> On a system with no L3 monitor resources it should still be possible to
> run with just AET.

This needs more context.

> 
> Update get_rdt_mon_resources() to return true based on whether the

"Update get_rdt_mon_resources() to return true" - this is obvious from the
patch. Please replace verbatim code descriptions with high level
descriptions and include _why_ the change is made.

> system supports monitoring rather than on the presence of any of the
> L3 monitor features.
> 
> Refuse to mount the resctrl file system if there are no monitoring or
> control features.  Add invalfc() to rdt_get_tree() so the console log will
> let the user know why the mount failed.

This is obvious from the patch.

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e46eb9a4c725..d70c8cb1cb81 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -226,7 +226,7 @@ union l3_qos_abmc_cfg {
>  
>  void rdt_ctrl_update(void *arg);
>  
> -int rdt_get_l3_mon_config(struct rdt_resource *r);
> +void rdt_get_l3_mon_config(struct rdt_resource *r);
>  
>  bool rdt_cpu_has(int flag);
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index e633da548959..addcb2a19c98 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -991,7 +991,7 @@ static __init bool get_rdt_alloc_resources(void)
>  static __init bool get_rdt_mon_resources(void)
>  {
>  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> -	bool ret = false;
> +	bool l3_mon_found = false;
>  
>  	/* Any monitoring at all? */
>  	if (!cpu_feature_enabled(X86_FEATURE_CQM))
> @@ -1001,27 +1001,29 @@ static __init bool get_rdt_mon_resources(void)
>  
>  	/* Any of the L3 monitoring features? */
>  	if (!cpu_feature_enabled(X86_FEATURE_CQM_LLC))
> -		return false;
> +		goto skip_l3_feature_checks;

This goto looks unnecessary. Why not just "return true"? Even so, this 
also changes behavior from the previous version in a way that is not clear
to me. I was expecting this to consider the number of RMIDs supported by the system,
now that this function added:

	pqr_assoc_num_rmid = cpuid_ebx(0xf) + 1;

Should this be:

	if (!cpu_feature_enabled(X86_FEATURE_CQM_LLC))
		return pqr_assoc_num_rmid > 1;

Although ... looking at this closer it does look strange for pqr_assoc_num_rmid
to be 1, thus reflecting that the system supports one RMID, when zero may be more
accurate?

>  
>  	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
>  		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0, NULL);
> -		ret = true;
> +		l3_mon_found = true;
>  	}
>  	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
>  		resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID, false, 0, NULL);
> -		ret = true;
> +		l3_mon_found = true;
>  	}
>  	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
>  		resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID, false, 0, NULL);
> -		ret = true;
> +		l3_mon_found = true;
>  	}
>  	if (rdt_cpu_has(X86_FEATURE_ABMC))
> -		ret = true;
> +		l3_mon_found = true;
>  
> -	if (!ret)
> -		return false;
> +	if (l3_mon_found)
> +		rdt_get_l3_mon_config(r);
>  
> -	return !rdt_get_l3_mon_config(r);
> +skip_l3_feature_checks:
> +	/* Even if there are no L3 monitor events, AET might be supported */
> +	return true;

Is it guaranteed that system can support AET at this point? Previous version used 
supported RMIDs (return resctrl_arch_system_max_rmid_idx() > 0) as return value and
it is not clear to my why that changed in this version. Although, when looking at this
check it looks more like this series blurs "maximum index" vs "maximum number of RMIDs".
For example, resctrl_arch_system_max_rmid_idx() appears to reflect the highest RMID
index but it returns pqr_assoc_num_rmid that reflects the highest number of RMIDs.
This series should take care to make the distinction clear to avoid (now or future)
issues introduced because of different interpretations.

Reinette