Re: [PATCH v11 01/23] x86/resctrl: Give better names to X86_FEATURE flags for monitoring

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

Hi Tony,

On 9/10/26 5:06 PM, Luck, Tony wrote:
> On Wed, Sep 09, 2026 at 08:48:01PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> Please switch the subject prefix to "x86/cpufeatures:" to highlight the
>> subsystem changed. Considering that, the short description could mention
>> resctrl instead, for example:
>> 	x86/cpufeatures: Give better names to flags used by resctrl
>>
>> Although, I do have a question about one rename and depending on that
>> outcome the subject could be made more specific.
>>
>> On 8/31/26 10:43 AM, Tony Luck wrote:
>>> The feature flags for enumeration of Resource Director Technology (RDT)
>>> capabilities were chosen when the only feature was LLC cache occupancy
>>> monitoring and they were given names using the abbreviation CQM for
>>> Cache Quality of Service Monitoring.
>>>
>>> Additional monitoring features have been added to CPUs and the names
>>> are now more likely to confuse than inform the purpose of these flags.
>>>
>>> Rename X86_FEATURE_CQM to X86_FEATURE_RDT_M (to match the Intel Software
>>> Developer's Manual, and for symmetry with X86_FEATURE_RDT_A).
>>>
>>> Rename X86_FEATURE_CQM_LLC to X86_FEATURE_L3_MON since it enumerates
>>> that some L3 monitoring features may be present.
>>
>> Considering LLC as synonym for L3 it is not obvious why this rename is needed
>> (more below). 
> 
> I'm concerned about the "CQM" string in the name. This stands for "Cache
> QoS Monitoring" according to the original commit that added it:

ok, but the new name does not seem different in this regard.
Current name: X86_FEATURE_CQM_LLC, longer meaning: "Cache QoS Monitoring LLC"
Suggested name: X86_FEATURE_L3_MON, longer meaning: "L3 Monitoring"

As I see it, the "L3" in the new suggested name implies the L3 cache, the LLC.
So the only term in X86_FEATURE_CQM_LLC that does not appear in X86_FEATURE_L3_MON
is "quality" ... but looking at the patch this term (via "LLC QoS") is kept in the
description:
:

	-#define X86_FEATURE_CQM_LLC		(11*32+ 0) /* "cqm_llc" LLC QoS if 1 */
	+#define X86_FEATURE_L3_MON		(11*32+ 0) /* "cqm_llc" LLC QoS if 1 */


> 
>  cbc82b172638 ("x86: Add support for Intel Cache QoS Monitoring (CQM) detection")
> 
> This made perfect sense when the only feature was LLC cache occupancy.

oh, hmmm ... I do not see how CQM implies "occupancy".

> That's very clearly a useful metric for anyone interested in cache
> quality of service.
> 
> But the next two monitoring features added to CPUID leaf 0xF subleaf 0x1
> were memory bandwidth monitoring of local & total traffic. These are
> only peripherally connected to cache quality of service.

This does not seem peripherally, but instead explicitly, since these features
enumerated as events of the "L3" resource type. 

> 
> I realize that this argument is somewhat undercut by the Linux naming of
> those features with CQM substrings:
> 
> #define X86_FEATURE_CQM_MBM_TOTAL       (11*32+ 2) /* "cqm_mbm_total" LLC Total MBM monitoring */
> #define X86_FEATURE_CQM_MBM_LOCAL       (11*32+ 3) /* "cqm_mbm_local" LLC Local MBM monitoring */
> 
> But those seem wrong too.

These features look to match how they are enumerated from hardware via the, quoting the
SDM: "L3 Cache Monitoring Capability Enumeration Event Type Bit Vector (CPUID.0FH.01H )"

> 
>>>
>>> Add missing dependency to cpuid_deps[].
>>
>> nit: dependency -> dependencies
>>
>>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>>> index f70ee74b5f92..6a7f0adb123e 100644
>>> --- a/arch/x86/include/asm/cpufeatures.h
>>> +++ b/arch/x86/include/asm/cpufeatures.h
>>
>> ...
>>
>>> @@ -285,7 +285,7 @@
>>>   *
>>>   * Reuse free bits when adding new feature flags!
>>>   */
>>> -#define X86_FEATURE_CQM_LLC		(11*32+ 0) /* "cqm_llc" LLC QoS if 1 */
>>> +#define X86_FEATURE_L3_MON		(11*32+ 0) /* "cqm_llc" LLC QoS if 1 */
>>
>> The original name matched the description and since the description needed no changing it
>> is not clear why the feature name needed to change? I do see some redundancy in the name with
>> "LLC" as well "cache" making an appearance, but none of that is inaccurate, is it? The changelog
>> claims that the name confuses the purpose. How does X86_FEATURE_CQM_LLC confuse the purpose of
>> the flag?
> 
> See above ... 

I'd like to highlight again that the description, "LLC QoS", does not change and since it so closely resembles
the current name the new name looks unnecessary,  

>>
>>>  #define X86_FEATURE_CQM_OCCUP_LLC	(11*32+ 1) /* "cqm_occup_llc" LLC occupancy monitoring */
>>>  #define X86_FEATURE_CQM_MBM_TOTAL	(11*32+ 2) /* "cqm_mbm_total" LLC Total MBM monitoring */
>>>  #define X86_FEATURE_CQM_MBM_LOCAL	(11*32+ 3) /* "cqm_mbm_local" LLC Local MBM monitoring */
>> Reinette
>>
> 
> -Tony

Reinette