Date: Tue, 28 May 2024 15:55:29 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghua.yu@...el.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>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v19 00/20] Add support for Sub-NUMA cluster (SNC) systems

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

Powered by blists - more mailing lists