Received-SPF: pass (google.com: domain of linux-kernel+bounces-195761-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <2a761266-e934-4740-bf15-95dbfe4e4d5d@intel.com>
Date: Thu, 30 May 2024 10:55:48 -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>
 <a4d37f57-328f-4456-92bc-5e6469d092c8@intel.com>
 <ZlirIUbLw8fLHe0j@agluck-desk3.sc.intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <ZlirIUbLw8fLHe0j@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?ZCtja1V1Si90ODBJUXRKMktBS2RVcFJ3QmZyUnJURXVtL2w2WXZxM2JYM1ZS?=
 =?utf-8?B?Z0YvQThURVZaQkFYeUZwTTRVWmI0SXIzRWxkT1VBQkZSbkNMT3R3QlZaTGRr?=
 =?utf-8?B?S0kzQzhiY1M5c3I5TzNqWVlFTUQvZ0NLanA4VmdBMVJUQ3JOdUh3eVBxaEJx?=
 =?utf-8?B?SjVkU0tXZ3A2V0pDR201Yjl3MWc1dnYvTkt6bWxhZklMcnlEMlk3QzBBbWNn?=
 =?utf-8?B?aldXVjB5UWFMQjFrREt3ZTVnTjdTTzdOLzVwTERLRStLaG9hS016MDN6cUNm?=
 =?utf-8?B?b1M1QzMxYm5oNmlsUldYODR5SU1Tb2J5eEVLc050SmJDTEgyUm5HdWhaQ2F5?=
 =?utf-8?B?Q2kwQXJISmVoQXd1bVd1ODd0enRWNHZIdWhkejlvYU1QZFZab0liUk9ZZUtn?=
 =?utf-8?B?VjR6RFAwY1c4WTNNQ25uaS9ydlY0a2VLQmFzdzgvbFdkSmtUd2dkY0pvbWNx?=
 =?utf-8?B?TDBjRUgzb1lGdkNWVjM3eXVaV090d0x6alo1VE94RmtPRldYTkJIb1hzeFFE?=
 =?utf-8?B?OTVKZ3ZDMVRTQndnSkZtNVhSRmJwaEpJc1RISVR0aTFIRElSYTV3bW9rSmNY?=
 =?utf-8?B?cmxmRW11dlZ6YStlTWVCSlhPZVhPRlNIQVhWWFpjUkUxS2lvc2N0Y2YyeTQ3?=
 =?utf-8?B?ZHpiWCtrOWd5YWo1TklncFBWaVVScHZ2NitKVkJROVkzYklEM1lRalNmN3Jl?=
 =?utf-8?B?cU5FaDBXOG14Ym9VaVZrNGQ2YU1Zbjlra1FrMHA4WHZJbnRKVVlYdXlFZEVS?=
 =?utf-8?B?SEpTNEl2enFNa2hXUG5MMzY2cjZJMUJHbzFQRkgrUTRNaTVBdkhWVEVwRTlK?=
 =?utf-8?B?U3RZeDBFbE9VZ04yUmpPbWUxWWtzdWpQRkF1bStNSmxLcXkrN29JRnpNVWJq?=
 =?utf-8?B?QkthWjRENzduUmRPcnpra1paeHVJeFBnT3BUS3RQZVVuTzNDVVNDeDZ2bDZC?=
 =?utf-8?B?RGk4bmFHT2w2eXNOWGF5YmMyZUxrbVJSS1RySVNzMFZ5L1hkY0FybWVFRVNp?=
 =?utf-8?B?OUlPdXRTR2xObkVUV091Q2hCK1RjY0RmelN0SDFhQVU3UDdBNGdJeUJGWnBL?=
 =?utf-8?B?dmRzNTBiUGFObU03eGVLb09xKzREQTlPNk4rWm1tK0ZvaVArNldwbEJtRHds?=
 =?utf-8?B?bTdNOE1NbHBaMWhvR2kvaGNMMEpVZ3ZLTXkzWEMrajBGMW9pN1NiRUJWVTVD?=
 =?utf-8?B?ZkxyVjNLZ3pjMkxYS2pVQ3hURnhVYVVCY001bnVVQnBYSUVPTW9GTW8ySVow?=
 =?utf-8?B?MnNSOHdOOXNXRXYzQmc5OG1zbGt6by9QbHVFOXhDcENiNU5pN1djUVhpTVA4?=
 =?utf-8?B?WDRXNThQdndYSGN3MDk3eU16NjRsQ3lJVHMvdWNHQm1uM3UwNXpTVkw3ckVj?=
 =?utf-8?B?akM0MUE0OFQxUFhGU1BWUW40alZ0d2ExN0FjbEdtS2F1a2NGL0Y5YWpwdC94?=
 =?utf-8?B?YXJSQU5mdC9ZaVIxWjhXSTQ4SHR3NHR2ZzVydndDY1FJTzFiTmlsVHA4MFdT?=
 =?utf-8?B?LzRka1JtQ3dzSkYzNEpUMy90a2xtc3BlSlJZYkJxM1FnYVg0dkk2WmM2Qmox?=
 =?utf-8?B?a1ZyRk1ITGxmQnNtQytCd0RZV2xzUDMvR3E3aHVUQ2ZhSFFMOWhPZWVSMnVO?=
 =?utf-8?B?bmdsbjZCOHNKVmpEMmpKaHh3bXczc1ZyWnpnM3JSYjk0ZG5ja1pXQ2xDaklk?=
 =?utf-8?B?TmNkeUlwZEpWVzNRNUo4K25LUzNWeW9nN0JMaE1EaUtlN05FZHRuVUkyM1FK?=
 =?utf-8?B?b1VWeXp0YWNqcU03R2pQRFVTNXo2UXh2eDl2eTVhWnhNYnRmTEhENGFuenFq?=
 =?utf-8?B?Uys5aVd2NWduaTVyNmVScmhlbXFjaUdrSmNCZi82RUlsdlBadEM1OTgxMnJX?=
 =?utf-8?B?emxzZERTd25oZlA4TmQxS2h6OWRWcS9uaEVsS2p3SVkzei9NYWkzaVRmQi9E?=
 =?utf-8?B?ZXI3bUpmSkJ5cmZHNVlEV1htNGhLc3N6UnFhVDhYbUZ5eUR4bXc4ZzNROVhX?=
 =?utf-8?B?ZjZvVnd3bUNSVjIwbWorVjVHMGRwQU5POGJBengxSTVDQjF0aWxqekJQdVBN?=
 =?utf-8?B?cWVRMWJJa1RxbnJrak9GNDVrV3hEYVN0TlFCQnduemlSSnd2alFsYTVVSXor?=
 =?utf-8?B?OWtHK2xET2JHYkh5ZFRmM2NOTEUyOUU2c241MGUzVmlJMjJPTlFlRklqVmFT?=
 =?utf-8?B?cnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d4b09e06-5e13-4b0d-5f34-08dc80d1be29
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 17:55:50.7853
 (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: LGnRuR6SIUB3nxMhaOKqpCJHa8K74WF32xBwcVFbBdKkkOEzZbhFjif9uekOuRJHnngSmpYX+3ZqzTWgCdbfefd1XsyD2q2QQeuD3poyiJg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4592
X-OriginatorOrg: intel.com

Hi Tony,

On 5/30/24 9:36 AM, Tony Luck wrote:
> On Wed, May 29, 2024 at 07:46:27PM -0700, Reinette Chatre wrote:
>> 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)
> 
> Will fix.
> 
>>>    */
>>> 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?
> 
> I wondered that too. There are no complaints from gcc. How do people
> deal with false positives from static checkers? Simplest would be to
> provide an initializer:
> 
> 	char subname[32] = "";
> 
> While that might shut up the static check, it would be more confusing
> for human readers.

or	char subname[32] = {};

Please elaborate how this will be confusing to human readers? A comment
may help to address that.

I took the time to run a static checker on this series and it did
not complain about this issue. I did not run it with this fixup though, with
just original submission that seem to have similar pattern. I do still think
it would be good to initialize the arrays.

btw ... the static checker I ran did have four other complaints, three about
uninitialized data and one about divide by zero. Most problems appear to be
in mbm_update() that does not initialize rr.sumdomains nor rr.ci before
calling __mon_event_count().

Please use available tools to check code before posting.

> 
>>> 	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?
> 
> There aren't any kernfs_put()'s in the existing code.

Why should it? Existing code does not have the kernfs_put()'s because
the extra reference is only obtained in this new code.

Reinette