Received-SPF: pass (google.com: domain of linux-kernel+bounces-195879-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <268305d7-db7e-4369-8dd7-9c9faba210a5@intel.com>
Date: Thu, 30 May 2024 13:22:27 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 12/20] x86/resctrl: Create Sub-NUMA Cluster (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: <20240528222006.58283-1-tony.luck@intel.com>
 <20240528222006.58283-13-tony.luck@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <20240528222006.58283-13-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
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?L2x4OGxiMXAxdS9MajY0TDdpWU0valkzd1lGSjVDNTlYb20reWkxN0ppWEI1?=
 =?utf-8?B?R2oyL1JUcjh1NS9jRUQ1N2wvajlLTFkyNFhkdEhUZHQweDJoYXFaLzNWM01u?=
 =?utf-8?B?dGZkV1htOXNnc3ZwTWZaU3kzY01RcjkzMFU0bVZKMUZOTTRmM0pmZ2NLQ1lR?=
 =?utf-8?B?aDZxc1U5SGpzbWQ0U3RFWWVDZHVxb0NRak1lMlY0NWtSWllZTVBJY09ZcGhi?=
 =?utf-8?B?cERIU3VpWjBobUpvYzhrZGdBRGI0ajVzM1Vlc2p2LzJVU1lsNlBGdFpnM1VJ?=
 =?utf-8?B?Z2xtU0NxQzc0V3E4WVlZZ1o2Q20zMUJ3eGdwOTdOZ0FXRXdoK1JwR2RpNitK?=
 =?utf-8?B?S3VqRnk5VnpyaWUyMlJZcEhNRXNCUTBlT0JCekpEcXUxR1FYOFBHdGJyZStZ?=
 =?utf-8?B?bzljaTg1ZGNoZmdpS1FiaDJudjN6bW8xYzAxMTRZalpycnJKMW96RjRZZmZh?=
 =?utf-8?B?bW1aNWxBbHdXbWJEWndabHlQRkltQThtN01CMlhxMDV0RG1GeFFBb2s3L2hD?=
 =?utf-8?B?WmVZT2hzTTY3c2FPc2hCRGtYeldPYW92T1YwL1ZnSyt3QnN0WlpFYUVjTHBj?=
 =?utf-8?B?bmNGM0RzZWs3eEgyR1dIQTNWNHUxVzZjU3EvYWowTXBCeTYxUDdxTWgrZ1Z4?=
 =?utf-8?B?UG5RNUVlY1ZzaS91N1dhdUhadmpuUEdkMUlhYzkzQVNxWlczWGp5TzNZWXpw?=
 =?utf-8?B?cUtwR0tEb2RGRjFHa2VpU1FsMU9nYlpuN1FIMDdXQmxKLzFNY21qaXEwbkJX?=
 =?utf-8?B?aldoRUpEa3AvMHQ1OTdRMjR1LzdycnJzWHlOZUl3SzJURlRPYjVNK2dmRk81?=
 =?utf-8?B?bkFoY1VBN1RvRlhBaG9mbzJSME90d0p4OFpPWituT3ZGNXRvM3dneWZ0amZq?=
 =?utf-8?B?ZEVybkNCaG5vZ21PMTlTMGFNakpPVXdUVVRXNHJjbVJaVnpvdWtYLzV2R0hh?=
 =?utf-8?B?bmRqd0N3VW1pcHB0bzQ3UHVKN1RxMFZjblpENEloV0ZxV3pPVzBicXRPVW1I?=
 =?utf-8?B?aVVkWTZYZUljRlo5cHd6WVFVaXdCU3p1MkV2ZVF0blBUaEEvMEhQaW9LK01W?=
 =?utf-8?B?SVMxVElJYVRWYmZIVzJCSG91WDJRQ3FoaW9CWTYwQTVIVjhZcXZHZDE1QkFM?=
 =?utf-8?B?UElnWkFWQzN1bnhYYXowWi94N1ZUMEZNdGVLeWNRQXg5QjVyQUdPRFBXOC8x?=
 =?utf-8?B?eXVMWWlnbnd2dXU2UTh0bkRiVWRmYnR2Tk5aUlo5VTg5bllDSEhDeXZqNllM?=
 =?utf-8?B?Kzl6bE9IT3ErcCtXTGVMdk9HMFByYWRTYUlWci9hMmRzRUhDOHpTaSt4TVJH?=
 =?utf-8?B?U0t2cmtiZGppbURldVhvcVNCaGdKdVNWQlNRd1FtYVNpK3c4S3lXNklzbTc5?=
 =?utf-8?B?TXBNaGVvdGZIS0FheUVvdktBMjJxaFBoMzZrUnVhRHNTWkRrZUNrM2JYWnBG?=
 =?utf-8?B?M1RqTU1zM1BBaUZ0dDF4YUIxSmxLTnRYbThTS3FMRm44TWdQY3FvVWVoOWxE?=
 =?utf-8?B?aFlDSU55OHM2eEZtVUIxT1RxNGQvOUV6NzQ3YjJKMWV2eUFGdnplOUZZdDEv?=
 =?utf-8?B?TUJMTVBHbi9WN3VDT0lSNDdacWJUbmRrQ0toQWo2RktWNzJSNnhjSWpCMzh6?=
 =?utf-8?B?VFFKYVJRNS9kU3BQQzdXS1kvMEhabWhKV2hRNkNlUU9zeXRUMmtOaEZEVVR4?=
 =?utf-8?B?UGhTZGlmczZNV2hFU2o2Y0gzNjFKQ2VSa21PL2ZjK1VuVXhqUXY4NGEzUzl0?=
 =?utf-8?B?R0h3RlhPQWpRN3MxT2tRbjdEOTAydzhTOXZ6TE5kQXFGUmlyRXN3cVF5V3Rv?=
 =?utf-8?B?RFE2SXFOVVBralNvNE5GWHZkQm5wU3I3R1h3MzFpMGQwQUFGU0xlUkxxNU42?=
 =?utf-8?B?blNQWWN2Qmhxc1VydFhZbDFBa2FyL3hZZmVPMHUzQ241eWg3NFIvcDZzejRm?=
 =?utf-8?B?ZFFZVmZ6ZUlENXdLT1FFcHpzVm5GN2V0VU1FSnJkeWdHRUJPZk14aS91cDdB?=
 =?utf-8?B?RlBGUndraTUzamM4cEFsMkhJc2VGNnlQMUoxNTZIVFkvanhybGhoUmRXVHIw?=
 =?utf-8?B?R01hVFkrQkJTTDcwdVMxaXpkRG9qRUwwRk1ZTk1XYW15WVcxa3lRdGtjOGRK?=
 =?utf-8?B?ZTF2bCt1amJVQVRJS1Z2OC9TZWNNQnRsUzRBT2w0SVZrWHlJWkxMM3FiSk1k?=
 =?utf-8?B?Q3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b11a9b4-d760-4b7d-9bf9-08dc80e63ae0
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 20:22:29.9452
 (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: ATB1CR2+JCmTVpVTwgsnzpF3cHossyLtLF3DO3I7wkm0sOcNvybs2ydQr2bmk5IhrkilvlTfurNEZJ2M8RQSo+19s0msKz0Eu6wHs/17ThI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4951
X-OriginatorOrg: intel.com

Hi Tony,

On 5/28/24 3:19 PM, Tony Luck wrote:
> When SNC mode is enabled, create subdirectories and files to monitor
> 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.

What does "all SNC nodes sharing an L3 cache should be read and summed" mean?

> 
> 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

Drop "Also" - it is a red flag that a new patch is needed.

> 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 dd386ad9458a..6a5c35a176d5 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->ci->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 snc_mode;
>   	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);
> +	snc_mode = r->mon_scope != RESCTRL_L3_CACHE;
> +	sprintf(name, "mon_%s_%02d", r->name, d->ci->id);
> +	kn = kernfs_find_and_get_ns(parent_kn, name, NULL);

This new kernfs_find_and_get_ns() call takes a reference on the
kernfs_node before it is returned. Where is that new reference
released or why is it ok not to release the reference?

> +	if (!kn) {
> +		/* create the directory */

At this point this comment is not useful.

> +		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, snc_mode);
> +		if (ret)
> +			goto out_destroy;
> +	}
>   
> -	ret = mon_add_all_files(kn, d, r, prgrp);
> -	if (ret)
> -		goto out_destroy;
> +	if (snc_mode) {
> +		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