Received-SPF: pass (google.com: domain of linux-kernel+bounces-186729-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <aa7d64b1-7c8e-4594-9e91-dfef2bae191b@intel.com>
Date: Wed, 22 May 2024 14:19:48 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v18 12/17] x86/resctrl: Create Sub-NUMA (SNC) monitor
 files
To: Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>, Maciej
 Wieczor-Retman <maciej.wieczor-retman@intel.com>, Peter Newman
	<peternewman@google.com>, James Morse <james.morse@arm.com>, Babu Moger
	<babu.moger@amd.com>, Drew Fustini <dfustini@baylibre.com>, Dave Martin
	<Dave.Martin@arm.com>
CC: <x86@kernel.org>, <linux-kernel@vger.kernel.org>,
	<patches@lists.linux.dev>
References: <20240515222326.74166-1-tony.luck@intel.com>
 <20240515222326.74166-13-tony.luck@intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240515222326.74166-13-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NE5kNjhIcWdBbUtYZ3hHcE1nWTYzUFpINXV0YWVzK0lhRXR1K1pWTWZCeHFn?=
 =?utf-8?B?KzJkQzhJbE85RTZzdHprOWkrZVN1RytnQmRCbUtWL2FOWGY0cjhrdXZwa2V2?=
 =?utf-8?B?RDYzRzIvY1FMNHR5WHMwemhrd0dPSzJ2YWxJbTRTbi9idHp0WnI3M3RhWnBk?=
 =?utf-8?B?Ui95d0FqWFhLZkdRdkx2WVNWbmU0R3dxbjh6N0IxMUxZcmpER1hrczRoT0gx?=
 =?utf-8?B?cDZYSGFZMjlXT0JxYk1JYWdBV01EYlprM0dFQXM1RjAvOGFObXZ0NVA1WThV?=
 =?utf-8?B?SFRwRVJvZWMyM0hhZHZWWXFnR3NIVjRHMUoxd0Urayt3S3hqMnBWSThHQkoy?=
 =?utf-8?B?ZXVRLzhwY2R0QXAzWEVmcGx4cE03bW1QTjViczJXTDlSRWpCL1lRTXFqYWNS?=
 =?utf-8?B?UnhOclgwWkhXdGxPYklWaUVubWptWThNbGxaTFFQZTJHNXJXMEk0Y3Nwbkh5?=
 =?utf-8?B?MUJtWjF0QTBBck1OKytDUUhaVm1oZ0pWenlOTmZzMWgzcjZVbjlxYWJJMnc3?=
 =?utf-8?B?cUtLWVEyU3p5elhnK3NlS242SjArM2pQK3RxZ2tDQ01udXZtalJPWEFSRnpI?=
 =?utf-8?B?c0l2ckhnMDdqczRSRWhzdVNQZEJSODV6eGExNGgvK1ZVSGNBVHdOYWVRVG9y?=
 =?utf-8?B?YzJZbTh2MnhLeFpQVEZnU051MzRtNEJ6ZTBzdUpVRm42ZW03MFFUL2lkZWUy?=
 =?utf-8?B?Tkl1ZGw3ZzlKb1l6UUc0L1RCQjNuRHZwUDVLWTVMekUydmVNaElyZXpDRktr?=
 =?utf-8?B?RDhJeUhIUGt2ZXM5ZGx1a2RFYTdkREtyL1JYRGhGa1lGeW5CRmZqeXlNb3gz?=
 =?utf-8?B?UUlPZ3k3aUNKWlV0SUVzVzc2UkdWN3hUUlhrS1Q1SVpjd3NFV1VmcTM0YkJ5?=
 =?utf-8?B?SjNOQmxBclBpNldIYmZKWVNncXBUbFJ4UUJ0VVNyYTgrdkpFTzduVFFoYzln?=
 =?utf-8?B?dmN4bHNMUjBZQTEwY3hJUC8xNFVpS0N3TXB0OGtDOFFxVW1ndm9iSEtCWGN2?=
 =?utf-8?B?aFhINERkY0E0Z3Ayc0QzaFlwRDFsYUkrcUMzYzZBS08xOTB5U25VNEZJK0tQ?=
 =?utf-8?B?eW0vZDNvMXc5NWx6b2tFb1U3Wk1SdHBIenN2bkpxOWZ0b2ZBMFZlSUhPQUlj?=
 =?utf-8?B?bEFMNDErVktaWTBmTTRtYkZ5d1NhRVBXNzErMlhoOWRLTHgvTW5wMTFQdFhh?=
 =?utf-8?B?Q1pFTzlCNFg2SWNwQmw1SDQ5cmlxTWhEQldmN2FqNlJUN0tobXBpb3E4M1NH?=
 =?utf-8?B?QjNGOE5aWi9iRGVxc3FnR1p5NEhOOXdkdEJISDZJa3JVaEZpcmdYRGpQaDJw?=
 =?utf-8?B?Q0NIMXlLNXdjdXFLeUxQdlVVVmJ5M3NxeFViSnQ5bzdCdDJVWkNqSUZ2Q01a?=
 =?utf-8?B?aE52Q0d3MWF1cnU2U05XOE4yNnBOdkp0WnE4RG1pVGZxS0dXazFZRnlYbVZR?=
 =?utf-8?B?cEJKek9BelpOYjBOT2U3cmlUbUFiOHg4VmdLaExJaFlWV0MyS2VkSEliUjJD?=
 =?utf-8?B?UEFMd1A0eFJGQit4WUdnMFdtS3EzVGJqNHlzZWRpKzFBSVdkU3NLUXZYQXBL?=
 =?utf-8?B?cWVVNllaU1c4aTVyWDdzM04zNUoydTBEMTY1WGwrRmlvNHBHY0RlNWM5ZnpW?=
 =?utf-8?B?S3JxTTZsOGc3Ny8xMnlUaHhyMjNPTmx4TkRlZzFoY3JYK3BjdzJrUnRLQUdw?=
 =?utf-8?B?S3FoQm5sRVNBYTE0RHZjOUVpYWpPNThyamlCL0J6Wkc4MXZFSzJmbFVMUnVE?=
 =?utf-8?B?cjVDK2RneHZraGxUSGZiVjJrS1h0SjBLOVNjUllyNHZqdXNJZCtQU3RWRVRa?=
 =?utf-8?B?UjJKZ1NFSUpVMm5ZdG8wQXpRamdsTVRRMzdXNkF3T3lDTUZpWWdKNnpCVHVV?=
 =?utf-8?B?NHE4K3FDdTdsdmU0aCtSbDlJQm9NUmJTR1MwVVMzU2QvU3U0cnhocWZVVVpT?=
 =?utf-8?B?eDVmdlRMNGx2RDlHZ2srUUdwRy9QSVFySTRhZEx1RXkyam5rc0crbVlrMVN5?=
 =?utf-8?B?d1h2R3V2NW1yRzZ1N0xMMUVCcGZxc3BEZEpSRVFUWEgxcnQxUUdIaGwrM1hx?=
 =?utf-8?B?SUdiTzVOb3QvK2FJLzg0bTNmcW9nWlhkblJNVUxBU3p4UWhLOTB3b0FmbzlU?=
 =?utf-8?B?cUx0MGtnSEtjbTNod0NoQVJmb3RzQ2ZJQTlrL0kyM1o5M0lLVXhsOW1Cdjd0?=
 =?utf-8?B?U3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 781814a0-6c34-4a6a-5e2b-08dc7aa4ea71
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2024 21:19:50.7347
 (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: H4Mq9efCntedfhfZZZJRAIzf6B/bHRcGjuiQG3zi/dCiDqGJMNmN1BF2kCQDAfoLlor36PQtt+Lfh4iCWiQPC2IYfcwkx4C2otOsVAULPjs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5887
X-OriginatorOrg: intel.com

Hi Tony,

On 5/15/2024 3:23 PM, Tony Luck wrote:
> When SNC mode is enabled, create subdirectories and file to monitor

"and file" -> "and files"?

> at the SNC node granularity. Monitor files at the L3 granularity are
> tagged with a "sum" attribute to indicate that all SNC nodes sharing
> an L3 cache should be read and summed to provide the result to the
> user.

Why go through effort to create a generic "monitor display scope" and
then just always refer to it as L3 cache scope? One consequence is that
the code and changelog seems to have a disconnect.

> 
> Note that the "domid" field for files that must sum across SNC domains
> has the L3 cache instance id, while non-summing files use the domain id.
> 
> Also the "sum" files do not need to make a call to mon_event_read() to
> initialize the MBM counters. This will be handled by initializing the
> individual SNC nodes that share the L3.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 53 ++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 7a6c40aefdcc..f0f468babdea 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3026,7 +3026,8 @@ static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>  }
>  
>  static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
> -			     struct rdt_resource *r, struct rdtgroup *prgrp)
> +			     struct rdt_resource *r, struct rdtgroup *prgrp,
> +			     bool do_sum)
>  {
>  	union mon_data_bits priv;
>  	struct mon_evt *mevt;
> @@ -3037,15 +3038,18 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
>  		return -EPERM;
>  
>  	priv.u.rid = r->rid;
> -	priv.u.domid = d->hdr.id;
> +	priv.u.domid = do_sum ? d->display_id : d->hdr.id;
> +	priv.u.sum = do_sum;
>  	list_for_each_entry(mevt, &r->evt_list, list) {
>  		priv.u.evtid = mevt->evtid;
>  		ret = mon_addfile(kn, mevt->name, priv.priv);
>  		if (ret)
>  			return ret;
>  
> -		if (is_mbm_event(mevt->evtid))
> +		if (!do_sum && is_mbm_event(mevt->evtid)) {
> +			rr.sumdomains = 0;
>  			mon_event_read(&rr, r, d, prgrp, mevt->evtid, true);
> +		}
>  	}
>  
>  	return 0;
> @@ -3055,23 +3059,42 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>  				struct rdt_mon_domain *d,
>  				struct rdt_resource *r, struct rdtgroup *prgrp)
>  {
> -	struct kernfs_node *kn;
> +	struct kernfs_node *kn, *ckn;
>  	char name[32];
> +	bool do_sum;
>  	int ret;
>  
> -	sprintf(name, "mon_%s_%02d", r->name, d->hdr.id);
> -	/* create the directory */
> -	kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, prgrp);
> -	if (IS_ERR(kn))
> -		return PTR_ERR(kn);
> +	do_sum = r->mon_scope != r->mon_display_scope;
> +	sprintf(name, "mon_%s_%02d", r->name, d->display_id);

Why not just determine "display_id" dynamically here and pass it as parameter
to mon_add_all_files()? Previously you mentioned that error handling is a problem
but this flow can surely handle errors, no?

> +	kn = kernfs_find_and_get_ns(parent_kn, name, NULL);
> +	if (!kn) {
> +		/* create the directory */
> +		kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, prgrp);
> +		if (IS_ERR(kn))
> +			return PTR_ERR(kn);
>  
> -	ret = rdtgroup_kn_set_ugid(kn);
> -	if (ret)
> -		goto out_destroy;
> +		ret = rdtgroup_kn_set_ugid(kn);
> +		if (ret)
> +			goto out_destroy;
> +		ret = mon_add_all_files(kn, d, r, prgrp, do_sum);
> +		if (ret)
> +			goto out_destroy;
> +	}
>  
> -	ret = mon_add_all_files(kn, d, r, prgrp);
> -	if (ret)
> -		goto out_destroy;
> +	if (do_sum) {
> +		sprintf(name, "mon_sub_%s_%02d", r->name, d->hdr.id);
> +		ckn = kernfs_create_dir(kn, name, parent_kn->mode, prgrp);
> +		if (IS_ERR(ckn))
> +			goto out_destroy;
> +
> +		ret = rdtgroup_kn_set_ugid(ckn);
> +		if (ret)
> +			goto out_destroy;
> +
> +		ret = mon_add_all_files(ckn, d, r, prgrp, false);
> +		if (ret)
> +			goto out_destroy;
> +	}
>  
>  	kernfs_activate(kn);
>  	return 0;

Reinette