Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <0871b932-cce3-4ab3-a2ba-2f1a2d27f2c0@intel.com>
Date:   Mon, 30 Oct 2023 14:22:18 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 6/8] x86/resctrl: Introduce snc_nodes_per_l3_cache
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        "Peter Newman" <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>, <x86@kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        James Morse <james.morse@arm.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Babu Moger <babu.moger@amd.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <patches@lists.linux.dev>
References: <20231003213043.13565-1-tony.luck@intel.com>
 <20231020213100.123598-1-tony.luck@intel.com>
 <20231020213100.123598-7-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231020213100.123598-7-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Vi83R2dOTDVQelU0ZUJLUXVDNXJ5NDNQWGlxR3dxb0U3NHJZNGJIcVZhNWxa?=
 =?utf-8?B?VzkxbXljYmNnUEc0R2ZKNUVhQ2MzQ2JvejhoTDFsV1lpL1ZuOC9GTExLNVNl?=
 =?utf-8?B?ZkxXU3craFlHbDg3ZEdKdnNqT3E4MnhlV0E4K21QKy9iUkc5MHBWL0M0Q3Iy?=
 =?utf-8?B?NnFIT1lvbzJwZWp2cy9qSkdSMDB4VTVqcUxJTmpta1JrQ2ZFanRncmdOZFcr?=
 =?utf-8?B?Ui9BQWxoa2o5dG44UHhhcEJlcjNiNllLZE5jS1ZxNXE4azRkcFk4TDhUcFov?=
 =?utf-8?B?Z0VFUDFRakx6bk1WTFRyV2RmdldZOGFxYlNHMDZRQ3VWakdRazArTWtMcytH?=
 =?utf-8?B?azJvY1BWTHVQWVB6aGlqWTVPV0srbVprYjdaQkZDOVQyTzJzZFVTZERhL1ZD?=
 =?utf-8?B?cEgvSmo4VzdMMDhqR0daa0VnRDVtTTN4NGZ5bFc2MVhNYVY0WGtlQ1ZoUGZj?=
 =?utf-8?B?MlpSaTlzMGU0SlZMV1YxVlpwcENKZHcwZ2k2UHI2a2xkeXZCM2pkV0VBZEZR?=
 =?utf-8?B?MEVKdGlrSWlobGg3UWxZQVhaWk5HdDNnREpiQVFkZGo1Tm5OQlBhMU9aa0hL?=
 =?utf-8?B?N3Q0eit0akpzUWdxd1JFM0dmN29DNVA4UWxWeWVnZ3Evdkpsc3dvSy9RaWo5?=
 =?utf-8?B?cTFEZ2ZSUDlDc0g5YUw2UU1KZE1CdEVWRmtQVW9YZ3FJZGhZUlFwYllvU0lq?=
 =?utf-8?B?ZHFNTjVOUk1LUlFLNVoyM2lOeDVPajl6dGl2N1dVWWRkelIzSVVGWjVQWVJt?=
 =?utf-8?B?Tzl0NVl3dmo1QWdSbWg1L3I5RTVNNk03NEMxSjBEaXNqUzZFM1MyQjNBa29S?=
 =?utf-8?B?L0dPcEp1M1BHQWt4V3p4OGpBL0pDejRaY2VjS0FCU1FsSy9UNWIzYTdJd3Z0?=
 =?utf-8?B?bWkyK2pXUFdReU9vRjZhU2tqU2FrbEZ0Z2hIMVJ4M1A0ckd4OEFLZWgvaGVV?=
 =?utf-8?B?bklJV2xsTUZWbTkyM0Q5QjBua2N3NHZ5SVlydHRHY2NpNHBDeExuSGYzVDFs?=
 =?utf-8?B?cnNtd0wybXZhdDNOVEU3MVU0aG9SR2ZlL0FvNUg1dENLV250TUs1WkJLRXJB?=
 =?utf-8?B?cTdybXRQTFFUeHhCZmNTT0lmT3NPVi90SDZzM1A0UU04d2xVZVBkQ2tDOXA4?=
 =?utf-8?B?VGNlTzJmNXpFUFZHeXp3NE9jWlhxaXNXako1TDZMUmVBOVFqTlE2WGFRWlZJ?=
 =?utf-8?B?OGRCVm5zQ0kxRjQ3SVB3Vy9FSFFXT0RNSDZWS2RGb0c4RnlwNUlSWnczaTkr?=
 =?utf-8?B?VHBzNGZpVVlHREVFNGJkOTFaRVhSMHlaTHlpbWJtUlhxeGo0K1RBNUN2Z2wz?=
 =?utf-8?B?cVFUc1BqeVBPT3FMenhJTm9mU0ZqbStMZEN0QXVqanZtRnlZeUw3VGVBM0pz?=
 =?utf-8?B?QlVLNFkwek5SMDlMWnFsLzV3Y3pMRWZtTTRacVhkMGdmUW9iaWFJMGQ4UUV1?=
 =?utf-8?B?Uk9mUlhjeGtYT0lObTdSU2NZbW5pYnJXSUlMb3FHL1lTbnB0TC9pZkRNcjBa?=
 =?utf-8?B?a0tMejhRVmhFbDdJZVJadzVRQ2FMWmJidVF3aVl5QlRuSnFaSTV3YWJtUmFm?=
 =?utf-8?B?L3Z0T29wMGVUQ1NvL0ZqR0M3QUhlOWZ5VGpBYkptOVpkVlpIdmlnRHNxYVBD?=
 =?utf-8?B?Z21IV2NVMjdRWXkrcmppeHZKTERVcmZxWS8vc2dKVlFDbWhyRXAvRFR3SHE4?=
 =?utf-8?B?MzY5UmEzQkxhQVNXdFp5Q09FamdhTVVnU3RXRE45QXNkTEN4MnhGYTBPbHUx?=
 =?utf-8?B?QlB0dUZYc1dnaWJqL05oUDlVUHFJeXhhbFI3M2R3SnlYTkt6UUJoU29LakhY?=
 =?utf-8?B?YlBaUU9OR05GR0s1OTZKTFlYallSc2RqbVRXWjdtVWxMVU8zRVMrSUp1YzVj?=
 =?utf-8?B?MHR5VVVJT0ZzWVRJTzM0Y2xlVlBCZXlMT28rWU40Tm84cks1ZThHOHVBbWx3?=
 =?utf-8?B?TTU4SUpWb2ZicUtwZXJ6NzdhdC9JVFlETGhQS0c4MmFVU2dZWmJzZCtyL1Js?=
 =?utf-8?B?OUhMWjR5OEdET2x3V1hvdG1hQnlzOC80MGRtYTVhelFBZnFqelROTU0rZzRw?=
 =?utf-8?B?TDJJWTNPcjNZUDl0NmtjcGtrYzhHbnc1MmRGd1FrMXVZL1VXcjF3Z2pzekc2?=
 =?utf-8?B?K0dEVmwwSDFiYitzVllRZStZeWZNb0V2TlI3MWNlV2pDVTV4d1VhblNXUnRZ?=
 =?utf-8?B?QWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f1d96405-1dd0-44b0-1e84-08dbd98e4cad
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Oct 2023 21:22:19.9268
 (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: rj57jYwWzq+/Hq9P24OHSILFDTnF1air+2wLEt+Dj2gEDihWJRZ+K+35FqSaN/G8vbwZ1YrADUv2ZODq/bQI0saXVPRlNwPvcEg3rhMTesM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8136
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-1.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Mon, 30 Oct 2023 14:22:39 -0700 (PDT)

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@google.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 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