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

Reinette Chatre
2024-05-28T22:55:35Z

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