Received-SPF: pass (google.com: domain of linux-kernel+bounces-197104-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <050c64b3-20b3-4db6-b782-f5124ebaab31@intel.com>
Date: Fri, 31 May 2024 09:01:04 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 08/20] x86/resctrl: Prepare for new Sub-NUMA Cluster
 (SNC) monitor files
To: Tony Luck <tony.luck@intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>, Maciej Wieczor-Retman
	<maciej.wieczor-retman@intel.com>, Peter Newman <peternewman@google.com>,
	James Morse <james.morse@arm.com>, Babu Moger <babu.moger@amd.com>, "Drew
 Fustini" <dfustini@baylibre.com>, Dave Martin <Dave.Martin@arm.com>,
	<x86@kernel.org>, <linux-kernel@vger.kernel.org>, <patches@lists.linux.dev>
References: <20240528222006.58283-1-tony.luck@intel.com>
 <20240528222006.58283-9-tony.luck@intel.com>
 <fe9a4cd5-f74d-4c89-8667-8b5b1841b84e@intel.com>
 <ZlkZG5bgsU6hY0Ok@agluck-desk3.sc.intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <ZlkZG5bgsU6hY0Ok@agluck-desk3.sc.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?b2xEZlNBbTBkTjduUWpuSTF0eHh2QUt5cFF4SGhTRktabWMzSmg1ZVBrN3VE?=
 =?utf-8?B?eDFNaWRkU2ptcEo1dUwxZmZyZ2w5TGVGMUlZS0YxUmhyeGN6RG81NzBqNFpH?=
 =?utf-8?B?TFQ5dlVTK29ubXh2MFJjckp0N2RDaGROcUtyRXlaV25JYklNM05QQUd0MVIz?=
 =?utf-8?B?dHlBWjBieStBWmpIUDV1UXNUblE2QXE3TGVrK05sYnNSS3lodkN1LytqeHFO?=
 =?utf-8?B?cjgweTVhdTNDVVVXSXFydnFlYmtZWXZmR2c5YjluQ25CN2NFTW1mUW12bUlm?=
 =?utf-8?B?VEpMdkZxbjBCZGhzUjlQeUNLakpaNVZSNCtMSjFMUGVBL0o0Y0ZUL3RFejZ4?=
 =?utf-8?B?aWZWSGZuRDlMUlpPVEpzWFhER0lySEFVaWgvbmtFQUloeDcxTjNPbUdtRGRt?=
 =?utf-8?B?dFlveFNDWkttejloYVl3RnZENHZTQk5NY2Q1bDBiY3hMeEY2SWNwQjlCU0xn?=
 =?utf-8?B?OGV3Y2o1RlhUWHkxcXdyR1ZlS3NpZW9DZFpnMlZJK2VZRDA2Nm04T2U4T25L?=
 =?utf-8?B?TC9zRDJ4NHlRVU9RalVheW9RdiswNDBIcVk0eUlTbmZIUENIbTNOK3c5VnpC?=
 =?utf-8?B?ZjJySVJlL3VZbGpvcHROSEZxNU8xZlA1K2xvelA1YVJBUXprL1M0MXhtM1Nq?=
 =?utf-8?B?ejZnVXRwWnkyVWhIQnpMY1E1MmxmUkFhczJJVm95T3BsUFJ4Tm9xMDNrL0tq?=
 =?utf-8?B?TllEa08rZ0MvYm1XQ3huS2tFc0lVeDNmdUc0SzFTdjBHZ0hpQ29ITDNtUHhi?=
 =?utf-8?B?YzNMay9LR0J5bEtzci9mM0VJNTNraUNoOTEwTC9oNUZuSFRPcGtnQkRjSzl6?=
 =?utf-8?B?eEVrMGZEQXczaWRQdGNLOHZJdjF1TWpBejlhVTZsRytBLzQ2VFpqRDlYOXQ5?=
 =?utf-8?B?UHVtM2h2dXBDQ2gxMDRLcnBSTlVOSU5hRVFzcjc3VHB6bkx1RE5WWmNpUERr?=
 =?utf-8?B?VTRqMCtDS1U4c1QyeFFMUnBnd1ljMklxTUlOMzI4T2EvdWNsY0xVcmhycE1m?=
 =?utf-8?B?ZGgrS295ZGNHK3BWVE01Ri9KQVdyVWNkZzlGb1hVZng3UTJGMEFLVk9heHkv?=
 =?utf-8?B?TUFDMDZpTmEvajFtV3lITkQ2M3pXQUhzd08xeHNRemRsRnArQmRNZm4wZXVD?=
 =?utf-8?B?SDF2RDIxazY0OFBNejFjQVRuQnJRTVpHeXo2NlVIbEQ1L1NzSStTYlcrL3gr?=
 =?utf-8?B?OTl5Q1h0anBiT0dTb0pxNVVJb2VOTGFCU1F4cW01KzNwWWpPdHVPUHY4V3N4?=
 =?utf-8?B?NW9NSmV0M1lWdXQyV1BsSnVvaG9DSDVNVDdFRXRsdGRzMFIzNnBEcVJmMk40?=
 =?utf-8?B?TmNhWnNidGxOWHBacDk0Tk9TZ0IxajF3MEd2aS9IUVNDRVI1Tlp2eHcxTGg2?=
 =?utf-8?B?d205ZlRTZVF1L0Nwa3VSQjhuMzJzK3AvOUlhdVNudFhNYVgycVI4ZktQU1lM?=
 =?utf-8?B?QWYyeVV2ZzQ5Q1Q4S2VpbFR6UWRGTU5uakNwZzc3anZJUXVNL0EzRGNmTVBW?=
 =?utf-8?B?Q2FVZWtBWUs3aFFxcjMvZFhScXEvMUpYWVJEeFVRSmhpK3BGdmtaZ1g4UGNK?=
 =?utf-8?B?cWRrc2dpYVJtWkZCY1AwNWZIa0pZc2ZZWGgwMDlwMkQrMlNyaGNyYlpWQVZr?=
 =?utf-8?B?c2JNQ0JDaUtIMFVYTlVNUGhKT2xwWWFJSWNCa3ZqQVZSMy95Q05vaVdCSnV1?=
 =?utf-8?B?M0x4b01TL3o2cmhXVkpYTlpuWU8rdWp4SDhQTUJjUzdBNFBIbENUbTU2Nngz?=
 =?utf-8?B?VUE0aXRSdFU1Yy9sOTF6bTdkTG9rSDg4SDh4ajVuK3h6OTh5R0RURVcybDRv?=
 =?utf-8?B?ejA0MEFRTiswNno2eVVrK0RwYk8xMDFwaE8zUGY0UXhtdmpUNWUrSlZibFdo?=
 =?utf-8?B?MEdvTjQ1UFRBUnFSY0trb0dzL0l6clR4Y3ZJbmNEZjhmZWxGNDVYcVBSZTJr?=
 =?utf-8?B?U2NaOFhLdUgzd2NGQWIyRCtaNGN6NlVGSWx1TEQ2ZWdrcWRWd1pleHdvSjVo?=
 =?utf-8?B?TVB5SGdwRjVSUzVMUWVDdFhpQWFheXhKMTlmNGhKdmVXa1ZYeFBSdmVrTHBk?=
 =?utf-8?B?UWkxMkRROVovZkpFNXVZTVRWZ3pPYXk4NENtMTdaUmVtNGtVY2g1ZlhHNlVn?=
 =?utf-8?B?dFhOY1NTQWc5SDlPNFVBbTlkT0NGUlNpS0NCd3ZDcjJLbkdnNVJjRnJBcUFK?=
 =?utf-8?B?N2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f3d2dca1-7724-425c-b474-08dc818ae19e
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2024 16:01:07.2117
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UkKo7yiHk50lF04I6r1TuBDg1tSASa0ZTswlGP0YIDYcl2fp1EhzT10aAT08FVoGn4AmkOhGwlbLZMFhCt9uPlpe92p31I+HiGv9RPXa1zk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB8039
X-OriginatorOrg: intel.com

Hi Tony,

On 5/30/24 5:26 PM, Tony Luck wrote:
> On Thu, May 30, 2024 at 01:21:01PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 5/28/24 3:19 PM, Tony Luck wrote:
>>> When SNC is enabled monitoring data is collected at the SNC node
>>> granularity, but must be reported at L3-cache granularity for
>>> backwards compatibility in addition to reporting at the node
>>> level.
>>>
>>> Add a "ci" field to the rdt_mon_domain structure to save the
>>> cache information about the enclosing L3 cache for the domain.
>>> This provides:
>>>
>>> 1) The cache id which is needed to compose the name of the legacy
>>> monitoring directory, and to determine which domains should be
>>> summed to provide L3-scoped data.
>>>
>>> 2) The shared_cpu_map which is needed to determine which CPUs can
>>> be used to read the RMID counters with the MSR interface.
>>>
>>> This is the first step to an eventual goal of monitor reporting files
>>> like this (for a system with two SNC nodes per L3):
>>>
>>> $ cd /sys/fs/resctrl/mon_data
>>> $ tree mon_L3_00
>>> mon_L3_00			<- 00 here is L3 cache id
>>> ├── llc_occupancy		\  These files provide legacy support
>>> ├── mbm_local_bytes		 > for non-SNC aware monitor apps
>>> ├── mbm_total_bytes		/  that expect data at L3 cache level
>>> ├── mon_sub_L3_00		<- 00 here is SNC node id
>>> │   ├── llc_occupancy		\  These files are finer grained
>>> │   ├── mbm_local_bytes		 > data from each SNC node
>>> │   └── mbm_total_bytes		/
>>> └── mon_sub_L3_01
>>>       ├── llc_occupancy		\
>>>       ├── mbm_local_bytes		 > As above, but for node 1.
>>>       └── mbm_total_bytes		/
>>>
>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>> ---
>>>    include/linux/resctrl.h                   |  2 ++
>>>    arch/x86/kernel/cpu/resctrl/internal.h    | 21 +++++++++++++++++++++
>>>    arch/x86/kernel/cpu/resctrl/core.c        |  7 ++++++-
>>>    arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  1 -
>>>    arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  1 -
>>>    5 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index 64b6ad1b22a1..d733e1f6485d 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -96,6 +96,7 @@ struct rdt_ctrl_domain {
>>>    /**
>>>     * struct rdt_mon_domain - group of CPUs sharing a resctrl monitor resource
>>>     * @hdr:		common header for different domain types
>>> + * @ci:			cache info for this domain
>>>     * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold
>>>     * @mbm_total:		saved state for MBM total bandwidth
>>>     * @mbm_local:		saved state for MBM local bandwidth
>>> @@ -106,6 +107,7 @@ struct rdt_ctrl_domain {
>>>     */
>>>    struct rdt_mon_domain {
>>>    	struct rdt_domain_hdr		hdr;
>>> +	struct cacheinfo		*ci;
>>>    	unsigned long			*rmid_busy_llc;
>>>    	struct mbm_state		*mbm_total;
>>>    	struct mbm_state		*mbm_local;
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 135190e0711c..eb70d3136ced 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -2,6 +2,7 @@
>>>    #ifndef _ASM_X86_RESCTRL_INTERNAL_H
>>>    #define _ASM_X86_RESCTRL_INTERNAL_H
>>> +#include <linux/cacheinfo.h>
>>>    #include <linux/resctrl.h>
>>>    #include <linux/sched.h>
>>>    #include <linux/kernfs.h>
>>> @@ -509,6 +510,26 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>>>    int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>>> +/*
>>> + * Get the cacheinfo structure of the cache associated with @cpu at level @level.
>>> + * cpuhp lock must be held.
>>> + */
>>> +static inline struct cacheinfo *get_cpu_cacheinfo_level(int cpu, int level)
>>> +{
>>> +	struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < ci->num_leaves; i++) {
>>> +		if (ci->info_list[i].level == level) {
>>> +			if (ci->info_list[i].attributes & CACHE_ID)
>>> +				return &ci->info_list[i];
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>
>> This does not belong in resctrl. It really looks to partner well with existing
>> cache helpers in include/linux/cacheinfo.h that already contains get_cpu_cacheinfo_id().
>> Considering the existing naming get_cpu_cacheinfo() may be more appropriate.
> 
> Reinette,
> 
> The name get_cpu_cacheinfo() already exists and does something different
> (returns a "struct cpu_cacheinfo *" rather than a "struct cacheinfo *").

Indeed, it is even used by get_cpu_cacheinfo_id() as well as a few
other places in resctrl.

> 
> How does this look for the change to <linux/cacheinfo.h> ... add a new
> function get_cpu_cacheinfo_level() and then use it as a helper for the
> existing get_cpu_cacheinfo_id()
> 
> -Tony
> 
> ---
> 
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index 2cb15fe4fe12..301b0b24f446 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -113,10 +113,10 @@ int acpi_get_cache_info(unsigned int cpu,
>   const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
>   
>   /*
> - * Get the id of the cache associated with @cpu at level @level.
> + * Get the cacheinfo structure for cache associated with @cpu at level @level.
>    * cpuhp lock must be held.
>    */
> -static inline int get_cpu_cacheinfo_id(int cpu, int level)
> +static inline struct cacheinfo *get_cpu_cacheinfo_level(int cpu, int level)
>   {
>   	struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
>   	int i;
> @@ -124,12 +124,23 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)
>   	for (i = 0; i < ci->num_leaves; i++) {
>   		if (ci->info_list[i].level == level) {
>   			if (ci->info_list[i].attributes & CACHE_ID)
> -				return ci->info_list[i].id;
> -			return -1;
> +				return &ci->info_list[i];
> +			return NULL;
>   		}
>   	}
>   
> -	return -1;
> +	return NULL;
> +}
> +
> +/*
> + * Get the id of the cache associated with @cpu at level @level.
> + * cpuhp lock must be held.
> + */
> +static inline int get_cpu_cacheinfo_id(int cpu, int level)
> +{
> +	struct cacheinfo *ci = get_cpu_cacheinfo_level(cpu, level);
> +
> +	return ci ? ci->id : -1;
>   }
>   
>   #ifdef CONFIG_ARM64

This looks useful to me from resctrl perspective. Having it can already
benefit resctrl by replacing the open coded ones in pseudo_lock_region_init()
and rdtgroup_cbm_to_size().
We have previously [1] been able to include a cacheinfo change in a resctrl
series by clearly identifying it as such.

Reinette

[1] https://lore.kernel.org/all/20200708163929.2783-1-james.morse@arm.com/