Received-SPF: pass (google.com: domain of linux-kernel+bounces-193210-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <08ca8fe7-9da6-41da-aad2-0081b789326c@intel.com>
Date: Tue, 28 May 2024 15:55:29 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 00/20] Add support for Sub-NUMA cluster (SNC) systems
To: Tony Luck <tony.luck@intel.com>, 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>
CC: <x86@kernel.org>, <linux-kernel@vger.kernel.org>,
	<patches@lists.linux.dev>
References: <20240528222006.58283-1-tony.luck@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <20240528222006.58283-1-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T0JvK25vTFBsUFNGMXRxZTdtTUJtNnE5d3dHdTBYaTZDZU5EQWtWelowQ3ZH?=
 =?utf-8?B?NU00b1IzdXpDd3VkOVhEWi9XVHlwa1BVK3E0dk9NcjQvcEVzbWY2dkVWcFAy?=
 =?utf-8?B?NUlKTlJCWmpDaXBYNzRhck9pdTIwaUVjVXVXK0R1NE1yaHRtamkybUo0VXBZ?=
 =?utf-8?B?YUZDS1cxMnZxZElrcGk5TUVmSEFpNTRhd0w2a3NqVlBPU0krMytUdnFsVVJr?=
 =?utf-8?B?M0h0WlE3YzI3M1VoVFBSNmIreEFTc0o0S1pFSGxHMlhXS25FaFJPN1hlTTlS?=
 =?utf-8?B?Z3lBMzV1YWhMNEVRNHl4K3NoVStibnE5Q1d1bEN1L1gyQUk5M211bFZEM2RY?=
 =?utf-8?B?VzFOdXNBRnBIRjN5R0VkUTFyWTVQTmFXZXRUd25wSE5lVWpteFRZVE9aYmdm?=
 =?utf-8?B?ZlpFbEdOWVFuWitUUyt0RzV6M1FNWTIydG1jNXVhZUtQY2p5UkR2b2RDcTVW?=
 =?utf-8?B?N2ZMeGhOeVMrekFna0RjZGQ1WVNKODdLVHVadDA5MXZCdlpLR09QUm0rY2NX?=
 =?utf-8?B?cHBLRmZuUlRZTUdTVCtHQitSaHV5aGdPcE9LOXpaU1FxVTcrd2w3bi9wZlBL?=
 =?utf-8?B?Rmc4MWtiM2xRbVhyS0FVbVlSL09ILzIvS1hjUy95dHV5Q1E4ckluVWVMRGsw?=
 =?utf-8?B?L0hDcnNYblFDcTd4SloweWttdWFrdzQ5U0Y5eldQYWRpVTNobFpxTloxdlQx?=
 =?utf-8?B?NzlYSlZUVkhlMS81bStoNHJkSXNyMmp1UFA3R1hNMmtULzREdndSYldHdWpD?=
 =?utf-8?B?RVh0MlBtT3k3MjNYblM1RTdlbnU3TmFZNldzTHltblBqN1R3eVFLM0tPcmo0?=
 =?utf-8?B?N0xjUHpyNzN3elNGUlM1NHNzU2hYY3c1MUIxUS9jaEd6NVZBWWNRV3AvZUUy?=
 =?utf-8?B?eWJUU25tdzFaSTBEd01PWWczeGpTdGtlV1pjejhocGVQZzVERytRVlMvbzRH?=
 =?utf-8?B?cXdKV3l0TGVYSlNQZ3orZURXaTNIc2puM1R0MmRpRi90TDU2VDdSUnhYYzRy?=
 =?utf-8?B?ekZTZnc3MlBPYS9YbjUvRytOTVhGcTZINDZLc21TQ0xFZm4vVkV4dUlWYnNG?=
 =?utf-8?B?UEFFdVZsWVpOb2xia2tWUi9TY29Tand2RTBUcExIU3I3SGxJYUdLcnFKMjVB?=
 =?utf-8?B?eU1iZFJia0kvYUlhU2ZwclZzZS9HNXNyYlBIeW41VUlBTHdyeG5wekR0SmdW?=
 =?utf-8?B?bVZnTEtKdDE2dUFTT3VoeDk1b3hwdGF2ZktNUWYwUlpyU3Z5SmVROXdCSWVK?=
 =?utf-8?B?Z2Z1aVJVeDRrN0lVcTBRY2trTzFPaXFJT2xFVU5CZ2xCWXI1ZnNJa2lWZnZO?=
 =?utf-8?B?MGhsWG16V000eGVTeDk4NjYxKzJKbjJsOFhzblU3ODdaQWxqNnNOUlBFSm5S?=
 =?utf-8?B?UGMrT1lQVjJ0RUhLajN1Z1hqaFJuYTlQdWpCdVhna1NVU1dVeVYyUU5DWTJ5?=
 =?utf-8?B?eGZoQmoySE1FaWk2QjgxUk5nbEVKdXdzQlhSYk5wL2RQKzdnVmJkU1RIa0w5?=
 =?utf-8?B?ZlJnRzlyMitKQkwyamZ1eGREaW42Q3M1Uk82ak82VXNUbTBacnVPeks3N3c4?=
 =?utf-8?B?VTZuK080aG5ZTUNtd1VzWk1sbDJnMHRid2drek5qL204TWFYT0NqQlNsZktD?=
 =?utf-8?B?OU40aVMvMFltYlA0Nktza3FCUGxoL1hiMDVWdnpSMUhQZk5wZmNrWTBJZ2hZ?=
 =?utf-8?B?aG1FZXY3K2YvUklaRnplVkl6b2xUUW5GcnpjNFZxcU9Ja291aTJMYmZ0cENr?=
 =?utf-8?B?Q1lnMWh3TXNVZkluYmsvbFVOUVZlS0NzL09vMVhDOTZmYW4yZkR6WTRRZzQ5?=
 =?utf-8?B?d1gyL01kY1ZJZjFuT1NIVS9lUmNOWVNOMU1QUnJ3aFhpZktvTVBzeWNUSkRB?=
 =?utf-8?B?bm1WZjltb3dOYjhjQUxQMXN0bk5uZnoxcjF6NXV2djIwNm90dzlrQ3I4SVJL?=
 =?utf-8?B?VDBvaGd6d0FtL1ZKQkNwM0pBVlBhaFh6eE1tMExid1UyUjRJN0tWTVRBaFZn?=
 =?utf-8?B?SmRnT0xBTW1pemNDOHNycHpzd0ZWVlloNE93QW1CRVF5MlN3RjloK2pKZGJ5?=
 =?utf-8?B?TS9VS1NtVDNJS3YvLzFqbTlLVjlKUnhoR2JrVytldnhYODdWL2JtVzZ1TElh?=
 =?utf-8?B?RTVRWFkxemNnZ00wWkJDZm84Tzk0L24xK2ExQ2J6U0RjbEE5a2RQTkRCazdY?=
 =?utf-8?B?b0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 45b5d1fc-0238-4e22-01be-08dc7f69469e
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 22:55:31.4063
 (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: A2TUk2tk/mPZNwpav4can4Uwl9AVCCwGvMJleZF7ptW68O0sD+HzYvDCc8OBwvbnKtgL0qbjLSYV2ITSztzkHaFE8u3bU5MxYLBRP/7gWgw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6993
X-OriginatorOrg: intel.com

Hi Tony,

On 5/28/24 3:19 PM, Tony Luck wrote:
> This series based on top of Linus upstream commit 33e02dc69afb ("Merge
> tag 'sound-6.10-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound")
> 
> The Sub-NUMA cluster feature on some Intel processors partitions the CPUs
> that share an L3 cache into two or more sets. This plays havoc with the
> Resource Director Technology (RDT) monitoring features.  Prior to this
> patch Intel has advised that SNC and RDT are incompatible.
> 
> Some of these CPUs support an MSR that can partition the RMID counters
> in the same way. This allows monitoring features to be used. Legacy
> monitoring files provide the sum of counters from each SNC node for
> backwards compatibility. Additional  files per SNC node provide details
> per node.
> 
> Cache and memory bandwidth allocation features continue to operate at
> the scope of the L3 cache.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> 
> ---
> Changes since v18: https://lore.kernel.org/all/20240515222326.74166-1-tony.luck@intel.com/
> 
> Global: Consistent use of "Sub-NUMA Cluster (SNC)"

(briefly scanning changes)

> 
> 1-4:	No change
> 
> 5:	Rename RESCTRL_NODE as RESCTRL_L3_NODE to make it clear that
> 	these "nodes" are each subsets of L3 cache instances.
> 
> 6:	Changes for snc_nodes_per_l3_cache are localized to monitor.c
> 	Don't use it in decision block use of mba_MBps option.
> 	Moved the old get_node_rmid() function here, but renamed it to
> 	logical_rmid_to_physical_rmid() with a block comment explaining
> 	how RMIDs are distributed when SNC is enabled. Function now
> 	checks if snc_nodes_per_l3_cache == 1 for fast return.
> 
> 7:	New patch. Only allow mba_MBps option if scope of MBM matches MBA
> 
> 8:	Replaces old patch 8. "display_id" field is no more. Add and
> 	initialize the @ci (struct cachinfo *) to rdt_mon_domain.
> 	Note that the new get_cpu_cacheinfo_level() helper function is
> 	added to internal.h as it will also be needed by patch 19.
> 
> 9:	Instead of display_id, add pointer to cacheinfo structure to
> 	struct rmid_read. Add kerneldoc description of existing and
> 	new fields.
> 
> 10:	Added to commit comment describing why mkdir_mondata_subdir()
> 	needs to be refactored.
> 
> 11:	Dropped Intel specific description of fields in the mon_evt
> 	structure. Say that choice of bit to steal was arbitrary, but
> 	can be changed in the future.
> 
> 12:	Fixed typo s/and file/and files/ in commit message. Now using
> 	the cacheinfo structure (specifically "id" field) instead of
> 	display_id.
> 
> 13:	Wordsmith commit into imperative.
> 	I looked at using kobject_has_children() to check for empty
> 	directory, but it needs a "struct kobject *" and all I have
> 	is "struct kernfs_node *". I'm now checking how many CPUs

Consider how kobject_has_children() uses that struct kobject *.
Specifically:
	return kobj->sd && kobj->sd->dir.subdirs

It operates on kobj->sd, which is exactly what you have: struct kernfs_node.

> 	remain in ci->shared_cpu_map to detect whether this is the
> 	last SNC node.

hmmm, ok, will take a look ... but please finalize discussion of a patch series
before submitting a new series that rejects feedback without discussion and
does something completely different in new version.

Reinette