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

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

>  
>  	/* 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?

> +
>  #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?

> +			.mon_scope		= RESCTRL_PACKAGE,
> +			.mon_domains		= mon_domain_init(RDT_RESOURCE_INTEL_AET),
> +		},
> +	},
>  };
>  
>  u32 resctrl_arch_system_num_rmid_idx(void)
> @@ -430,6 +439,8 @@ static int get_domain_id_from_scope(int cpu, enum resctrl_scope scope)
>  		return get_cpu_cacheinfo_id(cpu, scope);
>  	case RESCTRL_L3_NODE:
>  		return cpu_to_node(cpu);
> +	case RESCTRL_PACKAGE:
> +		return topology_physical_package_id(cpu);
>  	default:
>  		break;
>  	}
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index dbfb7d3bc3bc..a90291f57330 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2179,6 +2179,8 @@ static unsigned long fflags_from_resource(struct rdt_resource *r)
>  	case RDT_RESOURCE_MBA:
>  	case RDT_RESOURCE_SMBA:
>  		return RFTYPE_RES_MB;
> +	case RDT_RESOURCE_INTEL_AET:
> +		return RFTYPE_RES_PKG;
>  	}
>  
>  	return WARN_ON_ONCE(1);


Reinette