Received-SPF: pass (google.com: domain of linux-kernel+bounces-194890-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: <a4d37f57-328f-4456-92bc-5e6469d092c8@intel.com>
Date: Wed, 29 May 2024 19:46:27 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 00/20] Add support for Sub-NUMA cluster (SNC) systems
To: Tony Luck <tony.luck@intel.com>
CC: 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>,
	<x86@kernel.org>, <linux-kernel@vger.kernel.org>, <patches@lists.linux.dev>
References: <20240528222006.58283-1-tony.luck@intel.com>
 <08ca8fe7-9da6-41da-aad2-0081b789326c@intel.com>
 <ZleODtL_6D-Q_OMX@agluck-desk3.sc.intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZleODtL_6D-Q_OMX@agluck-desk3.sc.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?clRjUTh6eDRZbklpd1ZRMWdvN0JGNEZNYXYvdnRJMC9ZN0h5MVNkbHIwR1Y3?=
 =?utf-8?B?Z1pKblpwNFd6Q3VvQmNISTdhTnIxZEZaVlpXbGJvOUJNbFVJYm1XUGtJbi8z?=
 =?utf-8?B?MlNVMmk2WkpmcXFxbjJxL1U2TFcvb0c3Rk8waHBoSXFNZFh4UEVpcUxSZzJz?=
 =?utf-8?B?SWtQYTYrTDFoZE03bmxINVlpZGxpMUJobFJtUUczWjcyYXdkU0REK1pMYzkr?=
 =?utf-8?B?RmdwSzdqTkFIdVRzOERzaWY4cU4zOW9pM2dUZ1NDeStaUXNFSHdnRktwWDUv?=
 =?utf-8?B?cEY5ZnlhbWdKbU0zYnNsSFdRWGZ4MTFTYnh1SStxeElNNENiRzNRUWFyV0d0?=
 =?utf-8?B?SlBJL3RuZkF2Mjlhb1habmNDMFVxckNxc0hyaTQrV3lWb2NsYXNtZVVEeWFn?=
 =?utf-8?B?L0pYZmJHU2d2MFhYQ0ZNYXcyUUhnYXVFcWw1SWtqcXVKMW9BTHJRYXRqNTRE?=
 =?utf-8?B?Nm1sVFJPd3Nkb1ZMZGNjS2NOakIvd0dXY3JOcm5WQ09vSVN1OHM5ZlNqNzdz?=
 =?utf-8?B?MWxGckozYXJwQmd2WXQ4NGs2UDhVbWNMaXl4M0tiLy9RRXVDMDlzd1dVbFEv?=
 =?utf-8?B?aE9wbzZkUUUvZzhialN4ZWkxVkZHZW5VU2s4NkdUcERUTTlPQWJYRXpDYjJY?=
 =?utf-8?B?aWlLSTl3ZXdiU00rNGhpZWVSWFpSWDVSUlJhRmdtNkY5Ry92SWFEdVdTUERa?=
 =?utf-8?B?NG0ybU5sU0VDd2VTSCt5Q1JKdjVwL05SWTliNzFSM2pCUytadStreEZlTWJV?=
 =?utf-8?B?My8wakpDY1QrSjhJbVJIODlZSis1T2FwWk02U2V5Y1NKZXdmSVFwZXM3djVM?=
 =?utf-8?B?dzhpcS9xcC91eDJ1d2FzTU9hNjJaY2JST0RNUWdQYXNHcWt5SG96U2F4V3RF?=
 =?utf-8?B?b2NaaVBZTXMwZHJueCtPajdkTmF0T3RCOTFCdkxZVlVzcDhsTzJlU0Uza3ZS?=
 =?utf-8?B?dHRTdEFpY2NRWk0rdkpmR3FlRkwyTjRYVHp4RDV2WW5KWXRMc2FsZTJ0UDRl?=
 =?utf-8?B?NXdrVldZTWNLWlh1RC92aUtuZHpHYk5yeHBNNllleXZxWUpoOUcxQTVEbmJq?=
 =?utf-8?B?NEVvYkNRTkNweTRCOE96YTdTZmRPQ1lxeklkSGVaR1BFa3pqTXBPa3k5QzAy?=
 =?utf-8?B?Vm84T2xIcFU1U21Xelp3cjlzZkFaZjYzRkJSeW9GNXBtc2ZKcmsvUjFYNkRw?=
 =?utf-8?B?VGovVHBJYTVsWWlYdmUwazAyWFAyd3RCcjMreVlwY2xhblRSclBIWXRhU0w0?=
 =?utf-8?B?RXk0Rk1GYUFGTDR1MWNQcHVYKzBmeDJTUU1teWdCMk4zSDFQdGsvSmtkUW5y?=
 =?utf-8?B?bkdYYnRybXJFV2ZvMDBFTGxpTFJCNW1zM3dtMXlpNlIzVXNTS3VFcFpNU1oz?=
 =?utf-8?B?Q2NmYlhYbWlROEI1TmUyeldLMC8rYXNtcjluZXF2OXlwMEVMeTQyZ2JyTU9v?=
 =?utf-8?B?VGVTd3dRc2RBd1VJS3FzNzBkVU54TklQTkF0TXRmRTlRV1NjdnpJNU1QdDJQ?=
 =?utf-8?B?ZjRsQW5tMlBjNTc0emZGY2l3Mk1MS1FiRlpVaXFSOFVRWjczRlAyaGdINksx?=
 =?utf-8?B?MDQrd1d3a3MyUkhjekRZSWVPVnV4RDNOYisyRXN6T3lsQnoweTlhVUZxbEpI?=
 =?utf-8?B?VVBDNEMzWlFRYVc0V3orZ1VvQjhWWURDc2ZlVzQyaVVjMTFBNHVRbDRWZkli?=
 =?utf-8?B?MFlaVEtOQTQ3TXFldjNRZk1xV2VwOGQ4eFdHL2xtdFRraGxQOHhYMGMyTVZD?=
 =?utf-8?B?VmwwK3l5Tk9aMnZORkM5c0RURGJhYngrTzhaK3N6Y2U1alhKRWNFNEM0Sys4?=
 =?utf-8?B?cUx6WGdiemhkZGlYaFRsc1M0MVV5Sk9xcjJUMFg3bWhWbURDdlc0Q0t3YkVE?=
 =?utf-8?B?RWd0bEtaUlAxQzlheU1YNitMcTBkd1FMMkVvK2drL2JFVWFKaGJGcGc4WWw5?=
 =?utf-8?B?bzF1K3FrMDNwdmxmcHZnR1ltb2pvbDlreUxvQmp3bTFkeXpKT2UrVHhHUzVq?=
 =?utf-8?B?Ym81WDE0NllFK3Q4MkdDYUJwTm1TSEhXUWllNnZvSHU0d1dzalVkWEtTZmIr?=
 =?utf-8?B?ZTZrR3RsRW5zNmtsQkVzU1N6cHQ0OXJ4cmkzdDdhazkrWGZRQU1SSy9EVHdt?=
 =?utf-8?B?bGQ0N0JUUjdyV0FSM2NUTjFMaWtEV29kYm1hNFl5NzNZSFdYQXF0K1ZwakZO?=
 =?utf-8?B?ckE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b3e438b6-674c-492e-76d3-08dc8052b529
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 02:46:29.5827
 (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: ln/5Ua9cbcP7rUIPTXiIOmqmzs5x2xxBm4LiPig/nRZnNgHu1iw0dhE3LrgRqYET70j2vtbNOlUxBURU+prkecJ3WUpn8YOc182d0Khwy8g=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8492
X-OriginatorOrg: intel.com

Hi Tony,

On 5/29/24 1:20 PM, Tony Luck wrote:
> On Tue, May 28, 2024 at 03:55:29PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>> 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.
> 
> So right. My turn to grumble about other peoples choice of names. If
> that field was named "kn" instead of "sd" I would have spotted this
> too.
> 
>>> 	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,
> 
> So here's what rmdir_mondata_subdir_allrdtgrp() looks like using the
> subdirs check. It might need an update/better header comment.
> 
> -Tony
> 
> ---
> 
> /*
>   * Remove all subdirectories of mon_data of ctrl_mon groups
>   * and monitor groups with given domain id.

(note comment still considers that domain id is parameter)

>   */
> static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
> 					   struct rdt_mon_domain *d)
> {
> 	struct rdtgroup *prgrp, *crgrp;
> 	struct kernfs_node *kn;
> 	char subname[32];

I wonder if static checkers will know that this cannot be used
uninitialized?

> 	char name[32];
> 
> 	sprintf(name, "mon_%s_%02d", r->name, d->ci->id);
> 	if (r->mon_scope != RESCTRL_L3_CACHE) {
> 		/*
> 		 * SNC mode: Unless the last domain is being removed must
> 		 * just remove the SNC subdomain.
> 		 */
> 		sprintf(subname, "mon_sub_%s_%02d", r->name, d->hdr.id);
> 	}
> 
> 	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> 		kn = kernfs_find_and_get(prgrp->mon.mon_data_kn, name);
> 		if (!kn)
> 			continue;
> 
> 		if (kn->dir.subdirs <= 1)
> 			kernfs_remove(kn);
> 		else
> 			kernfs_remove_by_name(kn, subname);
> 
> 		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
> 			kn = kernfs_find_and_get(crgrp->mon.mon_data_kn, name);
> 			if (!kn)
> 				continue;
> 
> 			if (kn->dir.subdirs <= 1)
> 				kernfs_remove(kn);
> 			else
> 				kernfs_remove_by_name(kn, subname);
> 		}
> 	}
> }

This solution looks more intuitive to me. I do think that it may be
missing some kernfs_put()'s?

Reinette

ps. Please do give me a couple of days more with this series before you
     submit a new version.