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

["Luck, Tony" <tony.luck@intel.com>]

Hi Reinette,

On Thu, Sep 24, 2026 at 01:41:48PM -0700, Reinette Chatre wrote:
> 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.

I'dd add some more details.

> 
> > 
> > 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.

OK.

> 
> > 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.

OK. I'll rewrite the whole commit message.

> 
> > 
> > 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 X86_FEATURE_CQM_LLC is set, then monitoring is supported and the system
must support at least one RMID. I don't see a need to insist on more than one
RMID as a precondition for using resctrl. It wouldn't be super interesting
as all activity would be captured by that single RMID.  But it would be
keeping counts of events.

So you first instinct above to "just return true" seems reasonable.

> 
> >  
> >  	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 

Yes. At this point it has been established that the system supports
monitoring ... so AET support is possible. No way to know for sure until
mount queries the pmt_telemetry driver to see if AET is actually available.

> 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

-Tony