Received-SPF: pass (google.com: domain of linux-kernel+bounces-195638-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <66405792-5a7e-49b0-8cce-48c24419d2e1@amd.com>
Date: Thu, 30 May 2024 11:08:20 -0500
User-Agent: Mozilla Thunderbird
From: "Moger, Babu" <babu.moger@amd.com>
Subject: Re: [PATCH v2 3/4] selftests/resctrl: Add support for MBM and MBA
 tests on AMD
Reply-To: babu.moger@amd.com
To: Reinette Chatre <reinette.chatre@intel.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.1708637563.git.babu.moger@amd.com>
 <cover.1714073751.git.babu.moger@amd.com>
 <0367b45170fbc7411aad12c28802284a298c1c6f.1714073751.git.babu.moger@amd.com>
 <91eeb2d6-e0bc-43cd-8641-ddede63f3786@intel.com>
Content-Language: en-US
In-Reply-To: <91eeb2d6-e0bc-43cd-8641-ddede63f3786@intel.com>
Content-Type: text/plain; charset=UTF-8
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?REh1UGhQSUcyT2ZESmhlZjBSNzZpVTdPelFMZ25JRGlhNWZHeUtLcFJxOUY5?=
 =?utf-8?B?UUM1U0NsMnMyNlRudmdLWUJoMkkvdkhzWnVhK0xodlY1U2pEWGphL3hqTng2?=
 =?utf-8?B?RUxDK2V4dGlWeHYyRlAySmtHM0tLanU4SlFnVW9qQWtiSng5Wm9STldnbHlF?=
 =?utf-8?B?dmxOUmhzVmRpVUxRT1V2NXdUcFpvaVUzRXdRMnRidHR5K0UvbkV2dWQ5WU1s?=
 =?utf-8?B?WTZWam1WUGRvOXVzQUhjSFB0Sy9ZcXNPclM5b1dhdnRWTEhocFBhMEJLQWR3?=
 =?utf-8?B?Q3JQZXozaWhJcHlnMi9TMCtUbzEvRDU4c0J3TFdGYnFodVdjdU9zY0UzK2hB?=
 =?utf-8?B?aUdqZ1ZkSjhRWm5GeWtWRGpOdk45aVQ4TEFrUmcrb3FVTlo3Sml4MzRNMXhq?=
 =?utf-8?B?R0FKdGYrc0xtSmxNVStvOFlhbTNSYjM1V3NaREtZcnNqSmZndW1IblFZZSs2?=
 =?utf-8?B?NmFvdC9YQWtGQ3dhSE5iNFpFajl1eHVGZlVLcW5LT2VhZld4aUhGdCtTS1Fl?=
 =?utf-8?B?dy91M1E1cFdIT0JkT3FmNWI3MXlSS0lLZHYyZDNqak1JZjRIZmJxaE95UnNJ?=
 =?utf-8?B?ODBEeEJIYzEvTGdKNE5TWXp2Wm5rV0Y0QncxNTdVNEJicWczSnZKblBBbG5C?=
 =?utf-8?B?SUhIQkMwZnNIMXMyUzVCeEpGZUIzRnMzWFdROGtXSmVKbGJ0NXZvVzlURVQ1?=
 =?utf-8?B?UCtBd0htVnM5TkpoVU9zR3lmanVtdHJqUEhsT3BmQ0p2QzcrODNKNXh6RVZ1?=
 =?utf-8?B?WDlCeHJ2WVhENngweVJybHd5TlduTVpkVzh4UERaeDdHdVZjOFg4RS9ZMVJI?=
 =?utf-8?B?T2VrQnUyYU5XZjl2dFFFZWQ5cXk1RDRhc2F4RTlTOE9oc0JiN25VWUdua2Vz?=
 =?utf-8?B?ZW9DckpLeGxiMzczWHNHdjQrK29FQWlnTWc2YUhYU0NaZEltL3VjNEdINEZT?=
 =?utf-8?B?V200SlFtTXN3YmxIcHkxM1JIY25pK3I4RGNzN0VtOW9DN2JKaGZPeTJkNGll?=
 =?utf-8?B?R1BDTkVESEpvczFyY0d6RVE3dCs2UHNiS25zSjlWQVpxVU5ZNUp5S2k1OWt4?=
 =?utf-8?B?TnFoMFhOYTVGd1RlODA5Y2Y5L0RsUytYS2xyK01POHlLbG00dTFMcExMczZ6?=
 =?utf-8?B?OExldUMxazNGMURVRWlUeDVhWVBWcXRwdVc3UDVSMitKMjZnTjJoc0l2ajFE?=
 =?utf-8?B?UXo2aUd5aW0rTzVxSnhKakp0ODUrdE1KUzdIS1pJNmZDbnRESk1HakU2aTZu?=
 =?utf-8?B?VEdiRTZnVm9IcW53YytJUjZsZkcxeTNUNmFmdHJqbURNaUlObkYxNm5KdVF2?=
 =?utf-8?B?YmVoaGxRREVJT2FtczJyUVRmamhPTitDQkpDM29vVC95SDhhMkJuVmFsd2ZF?=
 =?utf-8?B?L3Byc1ZsZmtBR1c3ZFJMcm9kVGFoaU0wZzljZzlUWHplL2YraHl0dDhDVGFw?=
 =?utf-8?B?dVJ0UWo1aWJkSXg5QXZSc0VTOWNIZzlSZDRFL29mdlZwaVgvVDNOZlBjZ2py?=
 =?utf-8?B?cHI2ZnJvOHUzSTlQR3h2Y3F6SWMxT012NHVVL1gySEJpUVk4YnM5M2tmTjVw?=
 =?utf-8?B?MGx2VStrZW5CWWJQK3lJK0crSEY5amNpN2cxREJWa09yYS9wbWtPUHB4dzQ1?=
 =?utf-8?B?OTZtbkxTckhiS0JhajJYdGoyMWRNb2k2RWVUWkZCVGIra0dZSTl4UExWbWti?=
 =?utf-8?B?R09LcmREVXRHWTROeVJZcngrVkx6anBzMEtuOWQvcmtaajFhNkU0Yjc3eU9P?=
 =?utf-8?B?NWdxczNyeHJLR212dURtdm1mRjB3ZTdjK0swaGN4THZoS010bTYvYktJQkVV?=
 =?utf-8?B?T1hvazBsZFlBS1VNK0Y5anZhaXhFeXNaRUoxTDB2cEt5NHJKblBtM2NBV1Rs?=
 =?utf-8?B?QytreDNaamxmRkdSVDZHMzBNR0tmT1h1cWNZRGhBT3V6MVEwWjBXbTNSVFJB?=
 =?utf-8?B?MkFMQ3pLS25XOTFoeDE4TnQ0ZFVkdW42SDNpbXU5WEFCbTdScDNGdFVFZHZx?=
 =?utf-8?B?UHM5dHV2UGpKMkRlUmJCOXI1MHRGcmp4Z3dlZXBBQ0c3eGlaVElFUWtMbzdX?=
 =?utf-8?B?U212RjFEekpnNTdhS1RlK2tYUkh4V0NlQWgrLzVBU2R0S0V5TWNqZjc0TjB5?=
 =?utf-8?Q?jI+E=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fcf248e7-801b-46ca-23e1-08dc80c2bb55
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 16:08:23.5883
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: J8sw3TUuzj7QNUhCoaiDIzbb3g/COVC8cQwGyzUiMeWbd7h0Zk4gjcd7KoDFWOPp
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR12MB9119

Hi Reinette,

On 5/9/24 16:11, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/25/2024 1:17 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>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 67 ++++++++++++++++---
>>  1 file changed, 59 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index e3b09128ec3d..d90d3196d7b5 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"
>> @@ -146,6 +147,47 @@ static int open_perf_event(int i, int cpu_no, int j)
>>  	return 0;
>>  }
>>  
>> +/* Get type and config (read and write) of an UMC counter */
>> +static int read_from_umc_dir(char *umc_dir, int count)
>> +{
>> +	char umc_counter_type[PATH_MAX];
>> +	FILE *fp;
>> +
>> +	/* Get type of iMC counter */
> 
> iMC counter?

Yes. Will  fix it.

> 
>> +	sprintf(umc_counter_type, "%s%s", umc_dir, "type");
>> +	fp = fopen(umc_counter_type, "r");
>> +	if (!fp) {
>> +		ksft_perror("Failed to open imc counter type file");
> 
> Why go through effort of changing to generic names and then follow
> by using Intel naming in AMD specific code?

My bad.

> 
>> +		return -1;
>> +	}
>> +
>> +	if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) {
>> +		ksft_perror("Could not get imc type");
> 
> Same here.

Sure.

> 
>> +		fclose(fp);
>> +		return -1;
>> +	}
>> +
>> +	fclose(fp);
>> +
>> +	imc_counters_config[count][WRITE].type =
>> +		imc_counters_config[count][READ].type;
>> +
> 
> Up to here seems to be a copy&paste of read_from_imc_dir(). Could you
> instead split read_from_imc_dir() so that AMD and Intel can share the
> code to determine type?

Sure. Will address this in next revision.
> 
> Reinette
> 

-- 
Thanks
Babu Moger