Received-SPF: pass (google.com: domain of linux-kernel+bounces-215411-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: <fc551b62-6afa-40f8-b030-9e43253881db@intel.com>
Date: Fri, 14 Jun 2024 11:38:18 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/4] selftests/resctrl: Pass sysfs controller name of
 the vendor
To: Babu Moger <babu.moger@amd.com>, <fenghua.yu@intel.com>,
	<shuah@kernel.org>
CC: <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>, <maciej.wieczor-retman@intel.com>,
	<peternewman@google.com>, <eranian@google.com>
References: <cover.1717626661.git.babu.moger@amd.com>
 <f45784f8882fa41d454ac3b35b203ee6d04ecc25.1717626661.git.babu.moger@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <f45784f8882fa41d454ac3b35b203ee6d04ecc25.1717626661.git.babu.moger@amd.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?dGo4Q01jbzYrZnJnRThwSU5EUmVhSjV2eERtejB2QWZVVWtTajlXdVlta09s?=
 =?utf-8?B?dkZjV0JPNDJjTmh4SkZIN3diNWU0VUFQWmp5QlFjSGZxWHpxYzRqVk01dGl5?=
 =?utf-8?B?d0pkUk9TTE9sR0laOTE4cUhtcFBuVjJJOUtwNWxoaUNiYUp1SWl1eVN4dUNF?=
 =?utf-8?B?WDJGclpTbkxXc2E3RkZ3RHBBeGNTMDdJUnRCUUE0RXdEcTV6YzBxaU1kajZw?=
 =?utf-8?B?RUtZRDhxdWRpVFRQdmVocm96K0JCdFJKUjYxZS9tSi9mRDBkdm05NEtzeG5t?=
 =?utf-8?B?R2M5aHY3dnVlTGZnNHdLQW0xSkNUNG1CVFV1MXNhd2lkbHNaRzlJRzZlQjk5?=
 =?utf-8?B?TXZJN3Q1bU5aMHk2bVJHQkNPRG1PeFQ3RWdwczIxTTJlelE3U2VsZVRyK056?=
 =?utf-8?B?R1hIdTFVSksvdGpQQTAyUVYwZFJjMzlubFZlZEdNSk0vRVdYT0xNSit2bEQv?=
 =?utf-8?B?MUwvZUMxLzNza2x5b3FSdzZEaEYwTERMb1lydFNhbW82b01vaXFzQ0UzWm15?=
 =?utf-8?B?dEFZZTJUTW9sbnlyK3pYazJsZ0hoY2FSbE5OQTVQS1o0SE5nQkdpZVgxVThG?=
 =?utf-8?B?SG9sUjJOblVYWGdJM1UyN1YwZnRqL1Z1UmlZTjJTbkh1SmYxLzFkQlZ4OFZv?=
 =?utf-8?B?L2piWDV1NTh1dUdybWYyK1pCWjNVdUhPYzRDMnFYWjl6NmQ2ai9UaFdkY25M?=
 =?utf-8?B?MmpQK056RDlHVllZQnNnSDIrRmgrTzViZVRiczdvQXJVenNSM2dHNW4yOEhs?=
 =?utf-8?B?M3dNUERMbndraWkrNjEzd2FiOE1wWHBXK1VCU0tzL1h0U1hnZWFUeEJRZE1k?=
 =?utf-8?B?R3RadVErZERYdDdvcy8zTFpwdTU4NGFQZ2NyUnJRTmlNQzV3R0cwUTRZckc5?=
 =?utf-8?B?bGppNG5iRzFVSXhUTkNNdjdYMUxxSnZybjlGdjE0Y1BIOTdSbkNVMGFVVWdD?=
 =?utf-8?B?Rit2ajhsSndYaDlkZHd4MUtyZ0dtNjIwamZ6SHdMaDUxRyt6bEREalowN0hu?=
 =?utf-8?B?WUpUVkN2NHdqOGxRdE5KWURJYlVNRHFZczFHa1VPV3JvamJEME5uOGZxaXNz?=
 =?utf-8?B?cWs4QzFMRjBpMXQ4bTM0Yi90cHpjRHJzWFllSytHQjdCbEF4YmNFUUl6KzIv?=
 =?utf-8?B?UlVEZTZRUUNRTFRVRXdYTkNQK2R3cjdJbGhyanNjbnJWUVMzcHY3bDlvWW8r?=
 =?utf-8?B?TDV6S2t2WTdSMWNCQzRXOWxhUUM0T1EyY0RCZURscktJMjZnV010UWVlUXpH?=
 =?utf-8?B?SlJGLzJCcXpZMWd1VUlFZmg3Vy9ZMmhQUnpFeWg4dndkOWFjaE94azVDbTNR?=
 =?utf-8?B?VUsraFkxTlhrU2xZK3RZdkExc0cwNlkrOG5tcEt3aS9lV2x6NCtnS0ZmdFpk?=
 =?utf-8?B?TDl5aGFYN3lRazdXQXBKK3NuSkZWRndpc2wxRWVrc2JtL3Z0QmFDcHR3K0dY?=
 =?utf-8?B?UWw2a3dDajJoZkp2MXZvUXRaSnZtTjdyVUQrOGJ1TGZHZUtZMkdmVUd1bkhC?=
 =?utf-8?B?L1RHSnZaMmRwYXVKYlh4dWxZQmJPU3lBU3A5clJLZjdySWRtWThqYTZpbElU?=
 =?utf-8?B?dCsyTVR1Q3VVWXJteVQ2Q3NXSVRvQTE5LzBOTmVVenBiOTJKdlZyMTBSS0Ni?=
 =?utf-8?B?L2F3a3k1cU5mTHVDcDg0UHdQd3RhVm5jUEZIRHZYOVV4ME5pQjBKMVZOMWdx?=
 =?utf-8?B?M2RZRkRXNG1UWU53WTdoOW9OTTMvQmdhaHlaZ2pMcStxeWNGR0UxWlhUTXk4?=
 =?utf-8?B?dzFzell2TXlaK2hvWEQ4bWIva09tSmRJYUt2UWJlbktybHJLZkFIZ2pxNWRl?=
 =?utf-8?B?RDBVK284RFlJOWg4cmo2TzFmRG5tM1ArZk5zb1dxSUg3TStENnRIbWdxL2N2?=
 =?utf-8?B?eENRV0k3SDIrYU5EMWlwcXBGNDBsdEV2Tmg2emp5Q21tcU5YclJxcEl2cVBD?=
 =?utf-8?B?OHRkbUpjczI0OE9ZaU5RSDhHbDRXcVphZEZxN1phZGR0cDI5QXczUDhpZWhx?=
 =?utf-8?B?bVVXR3VuekQ2QkFZdmFlVnJteVNJek1XUnY4eXBlOVlSNU1JMk93c3JXYkly?=
 =?utf-8?B?c2xOMGU0Qk01b2JUWXpqd2V2RjgyUFlzLzNJbjk3M2xYTHFRbmZsaE9DcmM3?=
 =?utf-8?B?cWx3ZVRtcDlUYXdKc25TM3JjRVdYUTM4SjEwNW1uS1VZUVhQb1dQTFhrMWo2?=
 =?utf-8?B?Zmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e9e70d80-d2ab-43e9-12a1-08dc8ca12a4f
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 18:38:20.7938
 (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: c1qGmZNZkLD44KhnL3s8fvt7L/kXoVxV5lTTgR1qWAG4aR35QtEes3qSpIE96VVlAEn4rG1K9ax+uLxQv1M+t/RvAMIYxkpIO6yhbqR2PSM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4929
X-OriginatorOrg: intel.com

Hi Babu,

Subject and changelog mentions how controller name is "passed" but the
patch does not seem to "pass" anything new.

On 6/5/24 3:45 PM, Babu Moger wrote:
> The test detects number of memory controllers by looking at the sysfs
> file system. Detect the vendor to pass the controller name appropriately.
> So, that rest of the code will be generic and can be used to support other
> vendors.
> 
> Also change the search to look for the full string "uncore_imc_". Replace
> the sizeof with strlen.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v3: Change the search string to "uncore_imc_".
> 
> v2: No changes
> ---
>   tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++++-------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 2d5e961b3a68..23c0e0a1d845 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -10,7 +10,7 @@
>    */
>   #include "resctrl.h"
>   
> -#define UNCORE_IMC		"uncore_imc"
> +#define UNCORE_IMC		"uncore_imc_"
>   #define READ_FILE_NAME		"events/cas_count_read"
>   #define WRITE_FILE_NAME		"events/cas_count_write"
>   #define DYN_PMU_PATH		"/sys/bus/event_source/devices"
> @@ -206,24 +206,30 @@ static int num_of_mem_controllers(void)
>   	char mc_dir[512], *temp;
>   	unsigned int count = 0;
>   	struct dirent *ep;
> -	int ret;
> +	char *sysfs_name;
> +	int ret, vendor;
>   	DIR *dp;
>   
> +	vendor = get_vendor();

get_vendor() is already optimized to only need to do actual detection once.
I thus do not see a need for a local variable.

> +	if (vendor == ARCH_INTEL) {
> +		sysfs_name = UNCORE_IMC;
> +	} else {
> +		ksft_perror("Unsupported vendor!\n");

ksft_perror() also adds a "\n" so adding it here is not necessary. Also
please drop the exclamation.

> +		return -1;
> +	}
> +
>   	dp = opendir(DYN_PMU_PATH);
>   	if (dp) {
>   		while ((ep = readdir(dp))) {
> -			temp = strstr(ep->d_name, UNCORE_IMC);
> +			temp = strstr(ep->d_name, sysfs_name);
>   			if (!temp)
>   				continue;
>   
>   			/*
>   			 * imc counters are named as "uncore_imc_<n>", hence
> -			 * increment the pointer to point to <n>. Note that
> -			 * sizeof(UNCORE_IMC) would count for null character as
> -			 * well and hence the last underscore character in
> -			 * uncore_imc'_' need not be counted.
> +			 * increment the pointer to point to <n>.
>   			 */
> -			temp = temp + sizeof(UNCORE_IMC);
> +			temp = temp + strlen(sysfs_name);
>   
>   			/*
>   			 * Some directories under "DYN_PMU_PATH" could have

Reinette