linux-kernel - Re: [PATCH v2 05/16] x86/resctrl: Add and initialize rdt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z-sHjRxLDwGDP68i@agluck-desk3>
Date: Mon, 31 Mar 2025 14:22:21 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: Fenghua Yu <fenghuay@...dia.com>,
	Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
	Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>,
	Drew Fustini <dfustini@...libre.com>,
	Dave Martin <Dave.Martin@....com>, linux-kernel@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH v2 05/16] x86/resctrl: Add and initialize rdt_resource
 for package scope core monitor

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@...el.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?

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

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