Re: [PATCH v19 00/20] Add support for Sub-NUMA cluster (SNC) systems

[Reinette Chatre]

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@xxxxxxxxx>
>
> ---
> Changes since v18: https://lore.kernel.org/all/20240515222326.74166-1-tony.luck@xxxxxxxxx/
>
> 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