Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <5078f930-e56e-45b5-9df3-99e88c0858dd@intel.com>
Date:   Thu, 30 Nov 2023 15:40:52 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v12 7/8] x86/resctrl: Sub NUMA Cluster detection and
 enable
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>
CC:     Fam Zheng <fam@euphon.net>, Fenghua Yu <fenghua.yu@intel.com>,
        "Peter Newman" <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>, <x86@kernel.org>,
        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>,
        Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
References: <20231109230915.73600-1-tony.luck@intel.com>
 <20231130003418.89964-1-tony.luck@intel.com>
 <20231130003418.89964-8-tony.luck@intel.com> <ZWjOBw0Ygyw226Cc@dell>
 <ZWj3NdI/qLNOgyg0@agluck-desk3>
 <1c1a16a5-f235-4179-9d0f-1556e11d9c11@intel.com>
 <ZWkQBwwtSae4nGgH@agluck-desk3>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZWkQBwwtSae4nGgH@agluck-desk3>
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?ODhRSFhYUnY1bXpkN2UrOXVGM2NaQis0SVVVcW1DT1ZNYXYybmdDeVhlMEw4?=
 =?utf-8?B?azJ6bGErUGdvaTRNMlpiajdVR1NIM2JWTmpMOCtIWnNuaVpySkVGOFVmYkpD?=
 =?utf-8?B?SWR3UjFhQ0k1RUoxSWtjeVg5dHZnL1N3SVFsNEVsS1pxWDYrazNWQk5xUzJH?=
 =?utf-8?B?ZVgvb1lYT3B1aGQyYjFpMVY0aCtJV1JQWFdnSUNtSmRjam95R1YwZVg5UVh2?=
 =?utf-8?B?SnJidzhVZ1hJV2h3WFlaVlJXdVllcmlQampYWkM0S24zU0Y0UW52aGxkZW9v?=
 =?utf-8?B?ODBtU2hvS2JOeFhiM1BURFF4TmFFQlhPSkc1RWZ4elBtbE5RRWgxWS9lN2dF?=
 =?utf-8?B?cXNuZG1UWE1JUTNiV2h1dzB6VFJHRXpXYitHc0JWdlZUa25kaytDVTdLeXZO?=
 =?utf-8?B?bFVDOFV0ZmFraWZSekc1VEM0dWZBa250d0xjbDIxcUc2RWU2WG1KR0hMMndi?=
 =?utf-8?B?d2dIMElJYVZYM29pT3dwTlMyUkxjREV6cUN4VmZ5azZiYWkvWlpIMmNSYklL?=
 =?utf-8?B?RkxrcVVyZEZJZ0V3dmllWDd4dU95ZDR5S29xOFVVTHhmTE5ZYnJLYkE4aWlR?=
 =?utf-8?B?eEg5OWJZc3p2bTJrQ0pVVC8xTnRqQ1VMWXZNNFdBL3lxeFNpeHo2L0YvVHlD?=
 =?utf-8?B?Uk5JeURrOVZmai94dHZ1Uk0zZTlmYkFKaEJOSTVKVmJpSUlGZnpob0JRajdl?=
 =?utf-8?B?RFpJMC9sYXZYODZWNnZ5NWFidnVXQldWZ0RTc1MvRmZvb29kSml0SjlTbUlY?=
 =?utf-8?B?dEpPSStxeloyVjR0emFnZTRTQlRZaVBacFdmQ3M5MTFRcHAyTXE4d2lGUjNI?=
 =?utf-8?B?RU0vTUhCd0thSGR6aTYycUlrSWRjeUV1dHhjRVR4Mlg1amIzZGxQMlRsNFY1?=
 =?utf-8?B?c0V5SmwyVzVkbFpqMUc5MGxrY2ppNE12U1QzK3B3K0RnZHJTTnZRSlJoNi8x?=
 =?utf-8?B?aHl4c0pIaURHTGpaUFVrcXZmazhaam1lYzJObHAxTkxVNDViNE5yVHRnSkE3?=
 =?utf-8?B?aVVmU1luN0xCV3NnemUzUTE1STdHTFVxNnU4T1JpZndvTUNEclV2bFA4QVlv?=
 =?utf-8?B?M2pkc1ZMbWM3R2FRWGUrRkh6MCtYc2U0Nk10UU41SFRnUnR4bk85L1FoUGNT?=
 =?utf-8?B?enN6RjF2YktFYUFQMk5DQ1JoNlVxckZ6MGVSZzlpSkxJVHhNRUUwUWxucHZB?=
 =?utf-8?B?SWNxRnBOVUJuejMrZTdRMUVlVnZvSlhiTW1wdmdFc0VyWVVjVzJYSmFwR2o2?=
 =?utf-8?B?akRDQm4xb1UxSy9ueFZmYkxTMkFUeWtpR2ptVE5JdzY0MEo4QlFGTnhWYWJt?=
 =?utf-8?B?L1lSNE11WEpydGxrTjM5S0QyelMwWDh4Z01DN2Q2UjdFYXJxenJaZCtVZzQr?=
 =?utf-8?B?M2xCOXJzSFZKa2RsYkNEcGdlUFl6R25YQjArekhzRG1yaTRvRUVLSDJFV3R4?=
 =?utf-8?B?UkF3dzdCQVJJSWpqaEQ5TlU4YW9LczhvRjRiQ0ZWdHJNTC9HazgySDhhQXMr?=
 =?utf-8?B?QmI5RDZDZlFiT3RwUzd2TTBqWk5CNzRYa2FXekZjNlhvbTJGRTBWUHBobXh1?=
 =?utf-8?B?VmNMeUt5ZFYvTFphWlBIM0VpVWVsbk1SU0Y2djBoQ1M2bjlkWkJIeHkxUW9z?=
 =?utf-8?B?d1BpS3RkQzZDODBrUFc2Wjh5NHhtbnM1WEhIMHluTk1IdFNWM1ZCRjNvcWsx?=
 =?utf-8?B?anJWcjFoQWh4eTBEbUdSTm91eEFjSG85WTQ0OUgxV3Z0dUhRbWpFNzRjNFNv?=
 =?utf-8?B?RjBzZHpoeDZhbHplUVVNVGhubk5POXBHVUkyeFExc2xJV3g0OXZqa3RWa3M4?=
 =?utf-8?B?K29nV0twTHFSOGh4QSsyUXRXVWplcis1RjBXYXd0RENEbEZlRUxQQUV2cWR2?=
 =?utf-8?B?YUZ3OFNPUzl3WG1wQ2g3M2FZRkxGVkVnOHFjdTRSN2l4WVdpRUptUTZxOFV2?=
 =?utf-8?B?dWgxVWtLcnJmckZqTmdpZE9JazJwWEJXZVFINm01c3NrZ1AxRHBtb1p2VGg3?=
 =?utf-8?B?ZFBITEJEWUJvUFpBYmorV252WEtnMlFGR3cwWWxXU1NqRTNwZkxYa3dIb1Iv?=
 =?utf-8?B?QWIyTnRGOTlER3lGaFVuVE9TcjJyajZ3VVpnZFpiTlZyb0c5aG1ZbjRIZFE4?=
 =?utf-8?B?YmNxTUlybDFQK2RSQk5TaXdhTUU0dFZpNlZaUXNmVjc3dU44TW5WRTlGYzZT?=
 =?utf-8?B?SXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 49ab40fa-0512-4bf3-fa45-08dbf1fdcb86
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Nov 2023 23:40:54.9094
 (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: Wr2CewPyPLOsgXHXLJBheuYOp/iTj7ZhMjgf4gYs9/MWZHMZJ37kkmjh7pWed+iSGDpBdWD9GBhuBfUm7lx3QToDocxItnIiihnaxLPpg8Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8571
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 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,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.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 (fry.vger.email [0.0.0.0]); Thu, 30 Nov 2023 15:41:09 -0800 (PST)

Hi Tony,

On 11/30/2023 2:43 PM, Tony Luck wrote:
> On Thu, Nov 30, 2023 at 01:47:10PM -0800, Reinette Chatre wrote:

...

>>>  	if (!x86_match_cpu(snc_cpu_ids))
>>>  		return 1;
>>
>> I understand and welcome this change as motivated by robustness. Apart
>> from that, with this being a model specific feature for this particular
>> group of systems, it it not clear to me in which scenarios this could
>> run on a system where a present CPU does not have access to L3 cache.
> 
> Agreed that on these systems there should always be an L3 cache. Should
> I drop the check for "-1"?

Please do keep it. I welcome the additional robustness. The static checker I
tried did not complain about this but I expect that it is something that
could trigger checks.

> 
>>>  
>>> -	node_caches = bitmap_zalloc(nr_node_ids, GFP_KERNEL);
>>> +	node_caches = bitmap_zalloc(num_online_cpus(), GFP_KERNEL);
>>
>> Please do take care to take new bitmap size into account in all
>> places. From what I can tell there is a later bitmap_weight() call that
>> still uses nr_node_ids as size.
> 
> Oops. I was also using num_online_cpus() before cpus_read_lock(), so
> things could theoretically change before the bitmap_weight() call.
> I switched to using num_present_cpus() in both places.

Thanks for catching this. I am not sure if num_present_cpus() is the right
choice. I found its comment to say "If HOTPLUG is enabled, then cpu_present_mask
varies dynamically ...". num_possible_cpus() seems more appropriate when looking
for something that does not change while not holding the hotplug lock. Reading its
description more closely also makes me wonder if the later
	num_online_cpus() != num_present_cpus()
should also maybe be 
	num_online_cpus() != num_possible_cpus() ?
It seems to more closely match the intention.

>>>  	if (!node_caches)
>>>  		return 1;
>>>  
>>> @@ -1072,10 +1073,13 @@ static __init int snc_get_config(void)
>>>  
>>>  	for_each_node(node) {
>>>  		cpu = cpumask_first(cpumask_of_node(node));
>>> -		if (cpu < nr_cpu_ids)
>>> -			set_bit(get_cpu_cacheinfo_id(cpu, 3), node_caches);
>>> -		else
>>> +		if (cpu < nr_cpu_ids) {
>>> +			cache_id = get_cpu_cacheinfo_id(cpu, 3);
>>> +			if (cache_id != -1)
>>> +				set_bit(cache_id, node_caches);
>>> +		} else {
>>>  			mem_only_nodes++;
>>> +		}
>>>  	}
>>>  	cpus_read_unlock();
>>>  
>>
>> Could this code be made even more robust by checking the computed
>> snc_nodes_per_l3_cache against the limited actually possible values?
>> Forcing it to 1 if something went wrong?
> 
> Added a couple of extra sanity checks. See updated incremental patch
> below.

Thank you very much. The additional checks look good to me.

Reinette