Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <09847c37-66d7-c286-a313-308eaa338c64@intel.com>
Date:   Tue, 18 Jul 2023 13:36:26 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v3 1/8] x86/resctrl: Refactor in preparation for
 node-scoped resources
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        "Peter Newman" <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>, <x86@kernel.org>
CC:     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>
References: <20230713163207.219710-1-tony.luck@intel.com>
 <20230713163207.219710-2-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230713163207.219710-2-tony.luck@intel.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?S1lxdGRLSXp0c3lETlRFTXZ0VllLcmNyYnZUS1BtaUdTU0p1Zm82REU4dXNI?=
 =?utf-8?B?V0ViQW9sWGdUTWJvbG5tWmhGcGRONzZNUG5Vcml1b2hCQ1J1RnQyMlplenZR?=
 =?utf-8?B?amNHUFhDZWhwU3ZCcHg1RUt5MjdRRHhPM1JTSkZwbFpZbzZyZFV5Sks4NEl0?=
 =?utf-8?B?Y0JqMzREdEViMjFJZVRUbUJ2WmxrZWNlL3VUWnFNa3cvbnRwZHF0UTBLUE12?=
 =?utf-8?B?bnJnYURRU3hhK3ZkYWlpWG1iOWl4VDNranBxbFBlSHhiUDRyQUhKZEFoQkpk?=
 =?utf-8?B?ODl3ZlJyTWtRWkJtUmV0SXhZaXNxRkk5cUJwNmhrWUh0T0hOSjl6YzVza1Vh?=
 =?utf-8?B?S0ltc0tIT051b2Jka0pzS3BpTW9PanU3UzNySHBrcVpCWUh5RGZlUXZlV0ZF?=
 =?utf-8?B?TUp6SmVPbXVua2RXNHNSRkFvNFh1T3Vlc0IrYmppM25sS2o2WHVWNmhMUXY1?=
 =?utf-8?B?VkJpM2xXWjY5S3Jvdm1mOCtYb21jeEdua3lGL1dRZEJpMitXeDRNYzFYbG1r?=
 =?utf-8?B?VS9EZkRPa25zazdvbFNRV0x4VkpEaWFqWklqNzFJbytvVVRKVXVrK2xXUUds?=
 =?utf-8?B?YWRRS2sxRUFEOHIvdlNWeUN2aG5NeVZ1R2ZqVlhQRWF5YVg4VHVMZmdlSm1H?=
 =?utf-8?B?RE1WTzN1WnpQZVd1WDBRc2FkU0ZPbFZ6MUNZMnJOcDdmSkozQ2pIMjQ1Nnd3?=
 =?utf-8?B?S1VzU3VYY3N1WDNWMDc0aFlWbUlRWnczbkNKbWZ2ZlNMUEwzbXN5UWVzNnpa?=
 =?utf-8?B?L0J0ZDJzTlRYaDdFQkZMNi9FM0hpSGwwUVNXVTFuN3pMQS9POEFlK1ZPZW9N?=
 =?utf-8?B?NnZHeWJkTDZUYjZQRURpQkFkTTcyZGFidFFsQkQ5Q2JNVFdjY0J2R0pyeHRr?=
 =?utf-8?B?QnFhZVNMYVZmellqcUJ2Zi8xZmhLRjdrMjdTcFdQVFN6Z2tweEQzNGgxZmZr?=
 =?utf-8?B?N2pZcGJENHp6RUNyWHlmbVE1Y010cFZkQjd4VVQwSkxjZ0tHMFFjWjFPc1hS?=
 =?utf-8?B?dFVhWUNaNTVFc1Q0SEdDb3ZHU3R0MXEvUzY4SklKMkxMbk42NC9mVXM1Risv?=
 =?utf-8?B?dHdxbFVwSjFTbDdlZGhDRzEzRjYreXZoOEozdVIzMy8wOGQ5T25ZNEZiUHds?=
 =?utf-8?B?eU5SQmw2ZGdiU3UwNFZlT2tYRW1XZklDbjc0cFNNTFhIenhITDVhNEJNTW5N?=
 =?utf-8?B?QVoreUNjOFZCYWpOV3BvZElPZ2VzN0s3d2pHYkJNNml6dlN4VHpoTXd5Z09z?=
 =?utf-8?B?bTRhaE0wcmFhb3lLQy83ZkJLWFVzNTd5YXZ3U3V3VzMzbFdCWERTZ0J0TGhj?=
 =?utf-8?B?c3VXNGVaRldnbnZMaHBwTm9oVzVHYkY3VU9nWUZqcGEyWkJOeUcwNFdCTFFF?=
 =?utf-8?B?dUlRbDVkVDNzVmF4K0lsMkZxK21EWCtSc2pGSU1rOFdtWjY2d0w1eVk2UzFq?=
 =?utf-8?B?K3d2UTRJd0hIYWxHV2VSWUErcjloNDJaWlliT0RrSU5QMEMvM1BzdHRvR1Vl?=
 =?utf-8?B?RnZsTTJaOGlKS2RCa1NudGhyL1dEV2M5NUw5OEhXQ0I5MGQweHROMWdMbENP?=
 =?utf-8?B?TXVTMkFmeEtJaVBsUTNZekhxalVQTUJRaTZaZlo1UkI1elhCUlExTkxlWUZZ?=
 =?utf-8?B?NjhyTVV4Z09KU2Y2bVZrbGFwVHkvK3c1YnYxTW9sdGNkckQ4RUx0ZnhGRmVG?=
 =?utf-8?B?ZXcrQjNVc3RvaGYxdU40a2FKdVdDWW1TNWk4MDIxajFsMTB1MkdwWEdsdVF1?=
 =?utf-8?B?dVNzSmh0bUk2ZlQrWGIwMkFYRUtGYzUzUjk4WFdIZnh6MGhKaDNVWUw3S1FL?=
 =?utf-8?B?SWVMeEhScU9MODFaZjZSeThja1lrS3VCSnIrWUJoZDcrdkZwcmNVNGRkWDhn?=
 =?utf-8?B?cVZVNEpWYlZwSm5KcUg5NE9RZE1QOFBLNTdDYnF4RDZQc3ZlWEtjdmdwaFkz?=
 =?utf-8?B?VXdZVnErekloUVA3MWI0Y3FTb0hWekFvZXlMRkZsOW9DZWpvSGNEaUJkZkw0?=
 =?utf-8?B?S3ZXQ2xrR1ZCL3RReXhKN01venh2ejAzQ1poRmRyQjhieGFEQUdxTTVJcVA3?=
 =?utf-8?B?V3Rpd2gwblV3SXk2V1dVYmZGbUsyUm9hLzl5Z2ZiSjlRVU44eXhYSElSWXBT?=
 =?utf-8?B?S3BDOExqNC8yZ0E3Qm52bXZsZXlmeXI0M094QzRlc2xya1BsZEYzNHdYNkxZ?=
 =?utf-8?B?ZkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 74162dcb-54d8-46b5-e2b3-08db87cea9fb
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jul 2023 20:36:29.0954
 (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: pMJyubRtlEmBpTpGJDSavh4Lu+VZwUC5/jZ5yjNIUhGktJATXrUwLDxtK93sDruGZanNA5SAajT5ugWNJJ0z0kg0Em+mm/3KRFnnoxI/Zlg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7576
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Tony,

On 7/13/2023 9:32 AM, Tony Luck wrote:
> Sub-NUMA cluster systems provide monitoring resources at the NUMA
> node scope instead of the L3 cache scope.
> 
> Rename the cache_level field in struct rdt_resource to the more
> generic "scope" and add symbolic names and a helper function.

Can the changelog elaborate how the helper function is intended
to be used? When changelog just states "add a helper function" it
is unnecessary since that is clear from the code.

> 
> No functional change.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Peter Newman <peternewman@google.com>
> ---

...

> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 030d3b409768..6571514752f3 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -65,7 +65,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
>  		.r_resctrl = {
>  			.rid			= RDT_RESOURCE_L3,
>  			.name			= "L3",
> -			.cache_level		= 3,
> +			.scope			= SCOPE_L3_CACHE,
>  			.domains		= domain_init(RDT_RESOURCE_L3),
>  			.parse_ctrlval		= parse_cbm,
>  			.format_str		= "%d=%0*x",
> @@ -79,7 +79,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
>  		.r_resctrl = {
>  			.rid			= RDT_RESOURCE_L2,
>  			.name			= "L2",
> -			.cache_level		= 2,
> +			.scope			= SCOPE_L2_CACHE,
>  			.domains		= domain_init(RDT_RESOURCE_L2),
>  			.parse_ctrlval		= parse_cbm,
>  			.format_str		= "%d=%0*x",
> @@ -93,7 +93,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
>  		.r_resctrl = {
>  			.rid			= RDT_RESOURCE_MBA,
>  			.name			= "MB",
> -			.cache_level		= 3,
> +			.scope			= SCOPE_L3_CACHE,
>  			.domains		= domain_init(RDT_RESOURCE_MBA),
>  			.parse_ctrlval		= parse_bw,
>  			.format_str		= "%d=%*u",
> @@ -105,7 +105,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
>  		.r_resctrl = {
>  			.rid			= RDT_RESOURCE_SMBA,
>  			.name			= "SMBA",
> -			.cache_level		= 3,
> +			.scope			= 3,

Should this be SCOPE_L3_CACHE?

>  			.domains		= domain_init(RDT_RESOURCE_SMBA),
>  			.parse_ctrlval		= parse_bw,
>  			.format_str		= "%d=%*u",

...

> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 458cb7419502..42f124ffb968 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -297,7 +297,7 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>  	plr->size = rdtgroup_cbm_to_size(plr->s->res, plr->d, plr->cbm);
>  
>  	for (i = 0; i < ci->num_leaves; i++) {
> -		if (ci->info_list[i].level == plr->s->res->cache_level) {
> +		if (ci->info_list[i].level == plr->s->res->scope) {
>  			plr->line_size = ci->info_list[i].coherency_line_size;
>  			return 0;
>  		}
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 725344048f85..418658f0a9ad 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1348,7 +1348,7 @@ unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r,
>  	num_b = bitmap_weight(&cbm, r->cache.cbm_len);
>  	ci = get_cpu_cacheinfo(cpumask_any(&d->cpu_mask));
>  	for (i = 0; i < ci->num_leaves; i++) {
> -		if (ci->info_list[i].level == r->cache_level) {
> +		if (ci->info_list[i].level == r->scope) {
>  			size = ci->info_list[i].size / r->cache.cbm_len * num_b;
>  			break;
>  		}

The last two hunks are red flags to me. Clearly the "cache_level"->"scope"
change is done in preparation for "scope" to be assigned more values than
2 or 3. Yet the code continue to use these values as cache levels, comparing
it to cacheinfo->level for which I only expect cache levels 2 or 3 to be valid.
The above two hunks thus now have potential for errors when rdt_resource->scope
has a value that is not 2 or 3. 

Even if these functions may not be called if rdt_resource->scope is not 2 or 3,
this change makes the code harder to understand and maintain because now it
requires users to know in which flows particular functions can be called and/or
when code paths with invalid values are "ok".

Reinette