Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <50e1be65-8f27-c290-94b8-7dd1d98dc663@intel.com>
Date:   Tue, 26 Sep 2023 14:03:27 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.15.1
Subject: Re: [PATCH v5 6/8] x86/resctrl: Sub NUMA Cluster detection and enable
Content-Language: en-US
To:     "Luck, Tony" <tony.luck@intel.com>,
        "babu.moger@amd.com" <babu.moger@amd.com>,
        "Yu, Fenghua" <fenghua.yu@intel.com>,
        Peter Newman <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>,
        "x86@kernel.org" <x86@kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        James Morse <james.morse@arm.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "patches@lists.linux.dev" <patches@lists.linux.dev>
References: <20230722190740.326190-1-tony.luck@intel.com>
 <20230829234426.64421-1-tony.luck@intel.com>
 <20230829234426.64421-7-tony.luck@intel.com>
 <ceff3d3a-92a2-2e67-76a1-da5639f91162@amd.com>
 <SJ1PR11MB60838A29873FAAA50C9E9536FCC3A@SJ1PR11MB6083.namprd11.prod.outlook.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <SJ1PR11MB60838A29873FAAA50C9E9536FCC3A@SJ1PR11MB6083.namprd11.prod.outlook.com>
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?NGZWcnFGR1o4STR0Y0F6WXZkQmU5T1dxMmtudnV4ZmI4UFIzdzJwamlUYUhx?=
 =?utf-8?B?bUhGRDNPbDZKeHVXekJRUjM2dEhZS1hoNWViS0htUnFoNFVwZURxNm8zRFgy?=
 =?utf-8?B?VmhiWnBVdjJWdXNWU2xxKys1TWdHUXQyREdUYVNVaHBnenBoaWFjNnZsZ3BI?=
 =?utf-8?B?MTBlWFV0V0tobUxhM0NtdTR0clgvOHQxODd5ejArNE5iZEwxY3V0VmpPWFVC?=
 =?utf-8?B?QlZoVzNaUnd5enM2VEdJdlZwaFJBYnZVZ21sTnB6TGdjREpOdGUrYW9zL0Zp?=
 =?utf-8?B?d1U5dUdsZFpLSkZTRVBXa2xnUWd1NGxjWm0wRzVtYmtYYmVBVzRRMFNuK1M3?=
 =?utf-8?B?Y1pmOW1FZUtZaFBFVy9jUGMzZ2M0akx5b2RGV1FoS0h3Ykh4V2JoamViaDhH?=
 =?utf-8?B?akk4M25EMEtYaEdmUkFSU0JpQ01TeXl4WUZFTlVGdWV2OGhWNG1WYmFoYmFX?=
 =?utf-8?B?WThCVlkwOU4rZlM1d0tTRXFqck9XL1daSHBlNEk2KzJFMHVnejRhZXNIUTln?=
 =?utf-8?B?QTF1Z2N5OE5PaFZuZlFmYS9uODlFdFlkUVpHbkhvOWZrUlBrUnorT3lZUjhU?=
 =?utf-8?B?eGVEZC9CZ1R0RHR4ZkNJYXhQZEkxMysvRUxMQzV6OFZRbzBZL3lCdlh4bGhz?=
 =?utf-8?B?OFVIWWlVMXN5QlVkQTVnUEdwN2xmeHJQcFYxT3FiMmhYLzE2YWx3RlJCcU40?=
 =?utf-8?B?a0Z4a3NSOUI3K0VTaHhIMjJtNDdTY2crUU9ORWtQVWxNTUR0WDRLbGxiVGE5?=
 =?utf-8?B?ZFhKT0prV0dpaVRhY3ZwekRpZms0N0padlNSY205bGFXSm5NTFNHbWdwZDBr?=
 =?utf-8?B?ai9wTXBpeFovQ3hvVkRzdUorb1c4c21ld3g4RTM2aTk1RmllKzEzc0treVFM?=
 =?utf-8?B?UGoxTGRhUnNCYWJveHpBV0dXcHRSM3JIeWdsSEZQYTFFNWRHaVBSd1ZBUXVO?=
 =?utf-8?B?VzhrM0VvVUJyQXNVRTJsOFVoVjJveXYreDFFa3htODJzMzl1dXQ1a2M0NWk3?=
 =?utf-8?B?WXlmWHVvcHFaaGxreDNaT2x4bmxCQnVpelZkRlMxT1Rzek9Pc0g4dEpzSTVw?=
 =?utf-8?B?eXUxL1M0WmFISGJZVHVESVJEeFU1Q1VPdnlSQW5mN01PRDZHeWhxdy9WMjds?=
 =?utf-8?B?clZ2NmpkM0dIdHJ0Yi9mQTJkR09DWWVpdWdOR3lzVjkzbEtiWk1KcjhuN20z?=
 =?utf-8?B?Y0djaXA3ZXhIOEJyc1BoMFdhdWtsbXBNMDhXaTFkc0E0MUIzaDl2L1ZGSE5F?=
 =?utf-8?B?RVg3WEZ5ZytIaWVLaHZIRm9ZeCs5eTRhUlpSVXhMaTdZbm1GVDk1K2IwVjIx?=
 =?utf-8?B?UmtTcFF5eS9JVFRyUURpenBBUEdDU3NnV0QyWUR2Y2RTWDdvUkJycnlmYUJw?=
 =?utf-8?B?TmhiUUoxQUdydnBoMXZqS0JlMy9lUHR4bFB6a3RZZDF0MC9IOHNQYkNCdXJ6?=
 =?utf-8?B?VGFWNFhLNXM0U2JtZDZ5bGlXWXRzeXVweVBRLzAvUFpDVUJtdEUzMk5lTHQ0?=
 =?utf-8?B?OUhubE5panRwQjgyQVdMUHl1ZFdkdTRKU09KcGNSN1FySExVdDkvS1JZbFlH?=
 =?utf-8?B?RnZMelJHbHNVVnhKV2NPT01HN1J3a2wvSkhqbEdDRUdEcGRQeWlZUkswcmFR?=
 =?utf-8?B?WWt6S1J1a0FoK1VUbVRMVkNBY2tNK01VK1pScDduWmJOVTdhaHp0UDB0RW9j?=
 =?utf-8?B?Q1VOMnd6akFmbWFrdGFaalNHNzA1T0l5ak1tOFgxWDFNTHJCUzY4aTF2eHBJ?=
 =?utf-8?B?MCsxLzVjL2xEQVNaakp1bzU0cnh6OVRVVW9HRlZaT1F5WDlZQUdCRmMvYkVK?=
 =?utf-8?B?emNMMU1zaDRFbk4wc3hSenNtN3B6V1JFNjVwZGRVVzFaYjl0VDRWQTRweTgx?=
 =?utf-8?B?TjdsNEpVOW45eVpFRnlYaTUyL0tKc1ZlOTNpTHhiaE90eUh6VHFSOG1Rbytw?=
 =?utf-8?B?SHBwTTBNS0ZoNXBzcSt0d1lLNzAwRjdDTFBac0NCOGtMUTMvMmVhMSt3aGI4?=
 =?utf-8?B?NWpRSTAvN0g2R1haVm04Q2RhdS9HeE1reFhlenlteFZTbkRQV1ExMjZOUEor?=
 =?utf-8?B?S0lvUk5NdldtYkppZkdTZzJMOG9nOXQ4bWRXTWpzZTRUaUp0VXgyUWRpQkVM?=
 =?utf-8?B?NTE4TlV2MmJnZ29XNzAzTmZIMGVlaWFYUS9wQk1zekxNMmg0TUR5ZHF0M1hV?=
 =?utf-8?B?cVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b9bd7f3c-cd63-4e99-cbb4-08dbbed409c0
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2023 21:03:31.0837
 (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: nyS6gvEZ6sJXeZyHLLl4W/6VZGWcoJ8rdMOe+AEGFNVsJf8UWzJEvuIj7Mw9Yrqv5AYwjF71eYR3cmWqMTINRaAKXyEl3zCz4IhKLtPc1Is=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7695
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.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 (groat.vger.email [0.0.0.0]); Tue, 26 Sep 2023 16:36:18 -0700 (PDT)

Hi Tony,

On 9/26/2023 12:40 PM, Luck, Tony wrote:
>>> +#include <linux/mod_devicetable.h>
>>
>> I didn't see the need for this include.
> 
> struct x86_cpu_id is defined in this #include file.
> 
>>> +static void snc_remap_rmids(int cpu)
>>
>> While adding the new functions, i see that new function names start with
>> resctrl_ prefix.  However, we are all not very consistent. Can ypu rename
>> this function to resctrl_snc_remap_rmids?
> 
> I try to put a subsystem prefix on any global symbols to avoid random
> conflicts in other parts of the kernel. But I'm less sure of the value for
> static functions and variables that are only visible inside a single ".c"
> file.
> 
> If it must have a prefix, should it be "intel_" rather than "resctrl_" to
> indicate that it is an Intel specific function?
> 
> 
>>> +static __init int get_snc_config(void)
>>
>> Same comment as above.
> 
> Same answer.
> 
> 
> Reinette: Any opinions on these?
> 

I agree with you that a consistent prefix is expected from the global
symbols and a prefix of resctrl_ is indeed the goal as can be seen in
the growing include/linux/resctrl.h. resctrl has no established pattern
for static functions (look at the other static functions in this file
being changed) or even those non-static functions and data
structures shared between the resctrl files (see
arch/x86/kernel/cpu/resctrl/internal.h).

I'm ok with the static functions named snc_remap_rmids() and get_snc_config().

We can surely start a discussion if there is concern about resctrl static
function prefixes but that discussion will have larger scope than
this series.

If we do want to focus on function naming I do think a change that may
benefit this work is to be consistent with verb placement in the function
names. i.e either always have verb first or have a consistent snc_ prefix
followed by a verb. I do not recall if there are x86 requirements in this
regard.

Reinette