Hi Tony, On 10/20/2023 2:30 PM, Tony Luck wrote: > Intel Sub-NUMA Cluster (SNC) is a feature that subdivides the CPU cores > and memory controllers on a socket into two or more groups. These are > presented to the operating system as NUMA nodes. > > This may enable some workloads to have slightly lower latency to memory > as the memory controller(s) in an SNC node are electrically closer to the > CPU cores on that SNC node. This cost may be offset by lower bandwidth > since the memory accesses for each core can only be interleaved between > the memory controllers on the same SNC node. > > Resctrl monitoring on Intel system depends upon attaching RMIDs to tasks "on Intel system depends" -> "on an Intel system depends" or "on Intel systems depend" or ... ? > to track L3 cache occupancy and memory bandwidth. There is an MSR that > controls how the RMIDs are shared between SNC nodes. > > The default mode divides them numerically. E.g. when there are two SNC > nodes on a socket the lower number half of the RMIDs are given to the > first node, the remainder to the second node. This would be difficult > to use with the Linux resctrl interface as specific RMID values assigned > to resctrl groups are not visible to users. > > The other mode divides the RMIDs and renumbers the ones on the second > SNC node to start from zero. > > Even with this renumbering SNC mode requires several changes in resctrl > behavior for correct operation. > > Add a global integer "snc_nodes_per_l3_cache" that will show how many > SNC nodes share each L3 cache. When this is "1", SNC mode is either > not implemented, or not enabled. > > A later patch will detect SNC mode and set snc_nodes_per_l3_cache to Please remove usages of "later patch" from this series. For reference: https://lore.kernel.org/lkml/20231009171918.GPZSQ2Frs%2Fqp129wsP@fat_crate.local/ Please check whole series. For same reason I expect "earlier patch" to need removal also. > the appropriate value. For now it remains at the default "1" to > indicate SNC mode is not active. > > Code that needs to take action when SNC is enabled is: > 1) The number of logical RMIDs per L3 cache available for use is the > number of physical RMIDs divided by the number of SNC nodes. > 2) Likewise the "mon_scale" value must be adjusted for the number > of SNC nodes. Can this be expanded to indicate how the value needs to be adjusted? > 3) The RMID renumbering operates when using the value from the > IA32_PQR_ASSOC MSR to count accesses by a task. When reading an RMID > counter, code must adjust from the logical RMID used to the physical > RMID value for the SNC node that it wishes to read and load the > adjusted value into the IA32_QM_EVTSEL MSR. > 4) The L3 cache is divided between the SNC nodes. So the value > reported in the resctrl "size" file is adjusted. Can this be expanded to indicate how the value needs to be adjusted? > 5) The "-o mba_MBps" mount option must be disabled in SNC mode > because the monitoring is being done per SNC node, while the > bandwidth allocation is still done at the L3 cache scope. > Trying to use this feedback loop might result in contradictory > changes to the throttling level coming from each of the SNC > node bandwidth measurements. > > Reviewed-by: Peter Newman <peternewman@xxxxxxxxxx> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx> > --- > Changes since v6: > > In commit comment s/redumbering/renumbering/ > > Move check that SNC is not enabled into supports_mba_mbps(). > > Add Peter's review tag. > > arch/x86/kernel/cpu/resctrl/internal.h | 2 ++ > arch/x86/kernel/cpu/resctrl/core.c | 6 ++++++ > arch/x86/kernel/cpu/resctrl/monitor.c | 16 +++++++++++++--- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 5 +++-- > 4 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index 41a23556f57d..563e6203321e 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -446,6 +446,8 @@ DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key); > > extern struct dentry *debugfs_resctrl; > > +extern int snc_nodes_per_l3_cache; > + > enum resctrl_res_level { > RDT_RESOURCE_L3, > RDT_RESOURCE_L2, > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c > index 2c3975c9c20c..0e418dd14070 100644 > --- a/arch/x86/kernel/cpu/resctrl/core.c > +++ b/arch/x86/kernel/cpu/resctrl/core.c > @@ -48,6 +48,12 @@ int max_name_width, max_data_width; > */ > bool rdt_alloc_capable; > > +/* > + * Number of SNC nodes that share each L3 cache. Default is 1 for > + * systems that do not support SNC, or have SNC disabled. > + */ > +int snc_nodes_per_l3_cache = 1; Should this be an unsigned int? > + > static void > mba_wrmsr_intel(struct rdt_ctrl_domain *d, struct msr_param *m, > struct rdt_resource *r); Reinette
