Received-SPF: pass (google.com: domain of linux-kernel+bounces-215413-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <61fd7414-a36c-47a8-8633-163cb29b0e2f@intel.com>
Date: Fri, 14 Jun 2024 11:39:22 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 3/4] selftests/resctrl: Add support for MBM and MBA
 tests on AMD
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>
 <3c2034e3391634b35192819b69eabd7db8cffa8f.1717626661.git.babu.moger@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <3c2034e3391634b35192819b69eabd7db8cffa8f.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?WVVqTzJLSXh4U2o4dVBBc0ZxT1FMTmExRGZ4eTNvZEM3bFJRamQxR0xGVU1x?=
 =?utf-8?B?Y3lOUDg0NTE3ZlpPKzhMTnZENDFCb3dDeWxzUGlzNDdDY1RsT1JoM29YUUxh?=
 =?utf-8?B?Q2hnbHBvUkRxb1ZQaHp3d2hRWnZSLzUyQXc1dWRwSFNWN2E3ZFhtbm5iUW9E?=
 =?utf-8?B?MzhoV0FheGFjczRVaWtUSjZEaDFJVjhyZ1luTmhPeXNyUmNpNlNrQitWSHBQ?=
 =?utf-8?B?U28rYWg5WmFpYzYxM0UyczQzUDlLOUxGZkNoRlpSZzVnSEdYdHgvQVRpRHNi?=
 =?utf-8?B?UDEvaHQ1ZmxtMmhoLy9kV0ZUK05Rc3Bma3QvZGdOdUo1MkR0T29GOXErVE83?=
 =?utf-8?B?dkxMVlpMdmh2OHhXd3N5d0tiR0YydEs3VC9qMmRlUVkzK2pUY3Raamx4aTRo?=
 =?utf-8?B?QVpkenJuMjRUWnB6OG5yWWx1bmtobzRkZHpTeUhra2pSQmtNUmJ6c0NnSWhw?=
 =?utf-8?B?eWJFcGpLbDJ2Ykc2TnpxSUk0cmUvbDdwVnJxRkFLTWdmM3J6UUlBb0ZGTGZ0?=
 =?utf-8?B?aXBneWxnZGFWdDQ5SHJueWN2bTBBeWk5dUVuRU4xL0hwUmk1TjBvS1o0N1Ru?=
 =?utf-8?B?V1FmNEVJay9NVlhBU1llRjVhQnlvazVnN2pJc1Y4UFJMd3g2cHRIekpIVTRw?=
 =?utf-8?B?RnhYczQybnhWbXl0QlowV3ZUbXdRd3hHaFJmK1pkU24xWTN2eXJYZjlXYXB4?=
 =?utf-8?B?cDBrdlJZWVYwOERrZDROS0tOWDVsamNacFRyRFFOU3I2YnJ6TFlCbXAyWGh5?=
 =?utf-8?B?bHlFNjdKdzJsamd6Q2pPdEhBWjBYSFZvK2d1UlV6dnhOeUlwWkNZWVhmUmRX?=
 =?utf-8?B?TUxWdy9DWEFQSzZSRHJQaURTSDhkYzdmSXVKU2owV0F6SGU1OUpGRmVMbndt?=
 =?utf-8?B?aEJTc2JBYVZmVGVjZG1GNCtHZG5PR2k5bUNxKzN5T21ldnVlZ3JhczdEYTVD?=
 =?utf-8?B?MDZxNDVSVlNkN2I5NmVCbm5zclJVKy9FUjc5L3cyVG5zdk9oM0w1Z0dXNVlr?=
 =?utf-8?B?QkJuRjcvK2pMejFoWnc2ZFhPdFRzU2lxVDlmRTU2NlNaUG15K0tCanhZeExF?=
 =?utf-8?B?ekxkYlRMa0R0dTlYSG0zc2hLRXl3UEhWeGExaXIxc3dZMzQ2WVltMm5kZDdB?=
 =?utf-8?B?ZEVCZE5aeS9QQ3RxR29uSSt4emtOSjFVdUlOcGFCOXFkcnNWWXhKa3pYOGk3?=
 =?utf-8?B?TDh2Mjh4TVRiak9uL2QxZXorNWpBN3dmVFJNdC9UQWJnQVNKVi9JcnZhOE1z?=
 =?utf-8?B?RXROQ3czK3lSSjljRk9kalJuamJyRk1hdVpMNkJJdWVQWGhsa2RZT0s5T25a?=
 =?utf-8?B?eTlQV3JMNDMyZWpxNEtGdk5qTXFxMmE2TEhFMGUvSklCNTRYNTBIdUJibkJK?=
 =?utf-8?B?cVlOYmNBdSt2V0ZGaWU2T0REbjNGanBUQWlqYTlWdFNnZU1BQ0FQVXhPMHBQ?=
 =?utf-8?B?REx1VTdpM2dJODlJM3YyOFEveFR5OTJUQ2lLT1dud3pRRHlQT1haRmhKYnE3?=
 =?utf-8?B?OWRrTkZsTlFadW1hZkdkeUN0ZjVUWWVhUkxZUkNLTzZvdktqMXcydjJDbiti?=
 =?utf-8?B?L0FvdlNUQ0lINWg2Nk9pb2JiMFpLQkhGZjZpdVQ4YlJmYWJHbVdTcDEvanU5?=
 =?utf-8?B?a0JRNkZqdithZ3d0eVM0ekFiZVBXQlorK0RTWDVJTmtLVDdtQkxEV1N3QnMr?=
 =?utf-8?B?MWsxV3lxK3NqVmtFYUtFbWdxNjFrSXp6eHJmQWhsOUpNYmNjYm42TTdDUG9O?=
 =?utf-8?B?ODhPbHI0V3huUEZPaEhCcEhwRlVDcFQ4KzJFVEFLY0ZaSEFuOHRmRHgxd0Nw?=
 =?utf-8?B?VGJxT1VWZk1Zd09yelVNTWZVVVhrWUt6ellDc2ZtNStuZlpFZ2s3ZHpjV3Mx?=
 =?utf-8?B?UFZnQ05ZVU9DcTdKOHU2V3BuNC9vaHZEekxpcThoaUhCTGphNmNBbXdyMXNY?=
 =?utf-8?B?d2hVdWIxSWV1Z3dabVBMZWtBcUtTWDJTQkhablNaVFpORWgrd3lHc2k0RmVm?=
 =?utf-8?B?RlRaakNKdlVURk1xVndCM2FPd0d4SmYzelBzNjFGN1h1T2lhcGNBUW5NTVlD?=
 =?utf-8?B?SGxrSzNPdTNnVGZHS0NyK2NRRjVuY3RRTkZnQ2xad1FLWnY4MUdDc2pHRDhh?=
 =?utf-8?B?MC9UNk5nZUswak40TSttVUNCOEhoTFlia3JueTdHYURpU0NZcExJa1QyRU5B?=
 =?utf-8?B?VHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bebf791f-f0d2-4ad2-4791-08dc8ca1500a
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:39:24.0991
 (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: TBDY2vlS+ybj3Wr82AE8c2Oy9LN2Q/VZvmWf/Ymf4RGqL0oTP2OYrJwJuusrZpctxyBpJuOV9TmrS1UrBvJ9K8dQcLnLMh+89PcY9LMnXlI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5159
X-OriginatorOrg: intel.com

Hi Babu,

On 6/5/24 3:45 PM, Babu Moger wrote:
> Add support to read UMC (Unified Memory Controller) perf events to compare
> the numbers with QoS monitor for AMD.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v3: Made read_from_mc_dir function generic to both AMD and Intel.
>      Rest are mostly related to rebase.
> 
> v2: Replace perror with ksft_perror.
> ---
>   tools/testing/selftests/resctrl/resctrl_val.c | 80 ++++++++++++-------
>   1 file changed, 50 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 23c0e0a1d845..ffacafb535cd 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -11,6 +11,7 @@
>   #include "resctrl.h"
>   
>   #define UNCORE_IMC		"uncore_imc_"
> +#define AMD_UMC			"amd_umc_"
>   #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"
> @@ -128,7 +129,7 @@ static int open_perf_event(int i, int cpu_no, int j)
>   }
>   
>   /* Get type and config (read and write) of an MC counter */
> -static int read_from_mc_dir(char *mc_dir, int count)
> +static int read_from_mc_dir(char *mc_dir, int count, int vendor)
>   {
>   	char cas_count_cfg[1024], mc_counter_cfg[1024], mc_counter_type[1024];
>   	FILE *fp;
> @@ -152,41 +153,56 @@ static int read_from_mc_dir(char *mc_dir, int count)
>   	mc_counters_config[count][WRITE].type =
>   				mc_counters_config[count][READ].type;
>   
> -	/* Get read config */
> -	sprintf(mc_counter_cfg, "%s%s", mc_dir, READ_FILE_NAME);
> -	fp = fopen(mc_counter_cfg, "r");
> -	if (!fp) {
> -		ksft_perror("Failed to open MC config file");
> +	if (vendor == ARCH_AMD) {
> +		/*
> +		 * Setup the event and umasks for UMC events
> +		 * Number of CAS commands sent for reads:
> +		 * EventCode = 0x0a, umask = 0x1
> +		 * Number of CAS commands sent for writes:
> +		 * EventCode = 0x0a, umask = 0x2
> +		 */
> +		mc_counters_config[count][READ].event = 0xa;
> +		mc_counters_config[count][READ].umask = 0x1;
>   
> -		return -1;
> -	}
> -	if (fscanf(fp, "%s", cas_count_cfg) <= 0) {
> -		ksft_perror("Could not get MC cas count read");
> +		mc_counters_config[count][WRITE].event = 0xa;
> +		mc_counters_config[count][WRITE].umask = 0x2;
> +	} else {
> +		/* Get read config */
> +		sprintf(mc_counter_cfg, "%s%s", mc_dir, READ_FILE_NAME);
> +		fp = fopen(mc_counter_cfg, "r");
> +		if (!fp) {
> +			ksft_perror("Failed to open MC config file");
> +
> +			return -1;
> +		}
> +		if (fscanf(fp, "%s", cas_count_cfg) <= 0) {
> +			ksft_perror("Could not get MC cas count read");
> +			fclose(fp);
> +
> +			return -1;
> +		}
>   		fclose(fp);
>   
> -		return -1;
> -	}
> -	fclose(fp);
> +		get_event_and_umask(cas_count_cfg, count, READ);
>   
> -	get_event_and_umask(cas_count_cfg, count, READ);
> +		/* Get write config */
> +		sprintf(mc_counter_cfg, "%s%s", mc_dir, WRITE_FILE_NAME);
> +		fp = fopen(mc_counter_cfg, "r");
> +		if (!fp) {
> +			ksft_perror("Failed to open MC config file");
>   
> -	/* Get write config */
> -	sprintf(mc_counter_cfg, "%s%s", mc_dir, WRITE_FILE_NAME);
> -	fp = fopen(mc_counter_cfg, "r");
> -	if (!fp) {
> -		ksft_perror("Failed to open MC config file");
> +			return -1;
> +		}
> +		if  (fscanf(fp, "%s", cas_count_cfg) <= 0) {
> +			ksft_perror("Could not get MC cas count write");
> +			fclose(fp);
>   
> -		return -1;
> -	}
> -	if  (fscanf(fp, "%s", cas_count_cfg) <= 0) {
> -		ksft_perror("Could not get MC cas count write");
> +			return -1;
> +		}
>   		fclose(fp);
>   
> -		return -1;
> +		get_event_and_umask(cas_count_cfg, count, WRITE);
>   	}
> -	fclose(fp);
> -
> -	get_event_and_umask(cas_count_cfg, count, WRITE);
>   
>   	return 0;
>   }
> @@ -213,6 +229,8 @@ static int num_of_mem_controllers(void)
>   	vendor = get_vendor();
>   	if (vendor == ARCH_INTEL) {
>   		sysfs_name = UNCORE_IMC;
> +	} else if (vendor == ARCH_AMD) {
> +		sysfs_name = AMD_UMC;
>   	} else {
>   		ksft_perror("Unsupported vendor!\n");
>   		return -1;
> @@ -228,6 +246,7 @@ static int num_of_mem_controllers(void)
>   			/*
>   			 * imc counters are named as "uncore_imc_<n>", hence
>   			 * increment the pointer to point to <n>.
> +			 * For AMD, it will be amd_umc_<n>.
>   			 */
>   			temp = temp + strlen(sysfs_name);
>   
> @@ -239,7 +258,7 @@ static int num_of_mem_controllers(void)
>   			if (temp[0] >= '0' && temp[0] <= '9') {
>   				sprintf(mc_dir, "%s/%s/", DYN_PMU_PATH,
>   					ep->d_name);
> -				ret = read_from_mc_dir(mc_dir, count);
> +				ret = read_from_mc_dir(mc_dir, count, vendor);
>   				if (ret) {
>   					closedir(dp);
>   
> @@ -250,8 +269,9 @@ static int num_of_mem_controllers(void)
>   		}
>   		closedir(dp);
>   		if (count == 0) {
> -			ksft_print_msg("Unable to find MC counters\n");
> -
> +			ksft_print_msg("Unable to find iMC/UMC counters\n");
> +			if (vendor == ARCH_AMD)
> +				ksft_print_msg("Try loading amd_uncore module\n");
>   			return -1;
>   		}
>   	} else {

Can all the vendor checking be contained in num_of_mem_controllers() instead of
scattered through multiple layers? There can be two vendor specific functions to
initialize mc_counters_config[][]. Only the type setting code ends up
being shared so that can be split into a function that is called by both
vendor functions?

Reinette