Re: [PATCH v2 05/16] x86/resctrl: Add and initialize rdt_resource for package scope core monitor

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

Hi Tony,

On 3/31/25 2:22 PM, Luck, Tony wrote:
> On Mon, Mar 31, 2025 at 09:18:12AM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 3/21/25 4:15 PM, Tony Luck wrote:
>>> New resource for monitoring core events reported at package level.
>>
>> Could you please add proper (per Documentation/process/maintainer-tip.rst)
>> changelogs to all patches in the series?
> 
> Will do.
> 
>>>
>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>> ---
>>>  include/linux/resctrl.h            |  1 +
>>>  include/linux/resctrl_types.h      |  1 +
>>>  fs/resctrl/internal.h              |  2 ++
>>>  arch/x86/kernel/cpu/resctrl/core.c | 11 +++++++++++
>>>  fs/resctrl/rdtgroup.c              |  2 ++
>>>  5 files changed, 17 insertions(+)
>>>
>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index 25c3ee78de3d..6c895ec220fe 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -233,6 +233,7 @@ enum resctrl_scope {
>>>  	RESCTRL_L2_CACHE = 2,
>>>  	RESCTRL_L3_CACHE = 3,
>>>  	RESCTRL_L3_NODE,
>>> +	RESCTRL_PACKAGE,
>>>  };
>>>  
>>>  /**
>>> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
>>> index a7faf2cd5406..8f967e03af5a 100644
>>> --- a/include/linux/resctrl_types.h
>>> +++ b/include/linux/resctrl_types.h
>>> @@ -39,6 +39,7 @@ enum resctrl_res_level {
>>>  	RDT_RESOURCE_L2,
>>>  	RDT_RESOURCE_MBA,
>>>  	RDT_RESOURCE_SMBA,
>>> +	RDT_RESOURCE_INTEL_AET,
>>
>> This is fs code so architecture specific code needs to be avoided. Any other
>> architecture that may need to report similar data would be forced to use this
>> resource name.
> 
> Maybe  RDT_RESOURCE_PERF?
> 

Would be ideal if it matches the name of the resource (below).

>>>  
>>>  	/* Must be the last */
>>>  	RDT_NUM_RESOURCES,
>>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>>> index ec3863d18f68..3a100007301d 100644
>>> --- a/fs/resctrl/internal.h
>>> +++ b/fs/resctrl/internal.h
>>> @@ -240,6 +240,8 @@ struct rdtgroup {
>>>  
>>>  #define RFTYPE_DEBUG			BIT(10)
>>>  
>>> +#define RFTYPE_RES_PKG			BIT(11)
>>
>> "PKG" is not the resource but instead the scope, no?
> 
> Correct. I missed this one when doing some renaming. It should
> get a "perf" name (to match what we pick for the RDT_RESOURCE)
> 
>>> +
>>>  #define RFTYPE_CTRL_INFO		(RFTYPE_INFO | RFTYPE_CTRL)
>>>  
>>>  #define RFTYPE_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index c316268b4442..c8cc3104f56c 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -99,6 +99,15 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
>>>  			.schema_fmt		= RESCTRL_SCHEMA_RANGE,
>>>  		},
>>>  	},
>>> +	[RDT_RESOURCE_INTEL_AET] =
>>> +	{
>>> +		.r_resctrl = {
>>> +			.rid			= RDT_RESOURCE_INTEL_AET,
>>> +			.name			= "PKG",
>>
>> The resource name should not be the scope. It should be a name that reflects the
>> resource being monitored. In this case it is "core"/"cpu"? I understand that this may
>> create confusion since the data is provided at package scope so perhaps the "resource"
>> can be "perf" and then all the events can include "core" in their name? If the intention
>> is to guide user space in how to interpret the domain IDs then we could consider
>> something like "perf_pkg" or even "phys_pkg_perf" that prepares resctrl for future perf
>> events that may need reporting at a different scope?
> 
> "perf_pkg" looks like a good option. Should it be "PERF_PKG". It appears
> to the user in the names of the "mon_data/mon_%s_%.2d" directories. Also
> in info/%s_MON.
> 
> Resctrl uses "L3" in captitals in those places.

I do not know where the capitalization custom originated but PERF_PKG does seem like a good start.


Reinette