Re: [PATCH 3/4] selftests/resctrl: Add support for MBM and MBA tests on AMD

[Moger, Babu]

Hi Ilpo,

On 2/23/24 04:53, Ilpo Järvinen wrote:
> On Thu, 22 Feb 2024, 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@xxxxxxx>
>> ---
>>  .../testing/selftests/resctrl/resctrl_tests.c |  6 +-
>>  tools/testing/selftests/resctrl/resctrl_val.c | 62 +++++++++++++++++--
>>  2 files changed, 58 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
>> index 2bbe3045a018..231233b8d354 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>> @@ -104,8 +104,7 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
>>  	}
>>  
>>  	if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") ||
>> -	    !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") ||
>> -	    (get_vendor() != ARCH_INTEL)) {
>> +	    !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) {
>>  		ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n");
>>  		goto cleanup;
>>  	}
>> @@ -131,8 +130,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
>>  	}
>>  
>>  	if (!validate_resctrl_feature_request("MB", NULL) ||
>> -	    !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") ||
>> -	    (get_vendor() != ARCH_INTEL)) {
>> +	    !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) {
>>  		ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n");
>>  		goto cleanup;
>>  	}
> 
> These need to be rebased as this code moved into per test files with the 
> generic test framework. The .vendor_specific field is the new way to make 
> tests limited to particular vendors.

Hmm. I picked the latest code from tip/master. Where is the latest code
now? Is it yet to be submitted?

I can wait for your code to merge first.

> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 792116d3565f..c5a4607aa9d9 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[1024];
> 
> PATH_MAX

ok.
> 
>> +	FILE *fp;
>> +
>> +	/* Get type of iMC counter */
>> +	sprintf(umc_counter_type, "%s%s", umc_dir, "type");
>> +	fp = fopen(umc_counter_type, "r");
>> +	if (!fp) {
>> +		perror("Failed to open imc counter type file");
> 
> Please don't add new perror() anymore, I just managed to clean up those
> in favor of ksft_perror().

Ok. Will look into it.
> 
>> +
> 
> Drop this newline (to slowly move towards more concise error handling 
> blocks w/o all those extra newlines).

ok.

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

ok

> 
>> +		fclose(fp);
>> +		return -1;
>> +	}
>> +
>> +	fclose(fp);
>> +
>> +	imc_counters_config[count][WRITE].type =
>> +		imc_counters_config[count][READ].type;
>> +
>> +	/*
>> +	 * 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
>> +	 */
>> +	imc_counters_config[count][READ].event = 0xa;
>> +	imc_counters_config[count][READ].umask = 0x1;
>> +
>> +	imc_counters_config[count][WRITE].event = 0xa;
>> +	imc_counters_config[count][WRITE].umask = 0x2;
>> +
>> +	return 0;
>> +}
>> +
>>  /* Get type and config (read and write) of an iMC counter */
>>  static int read_from_imc_dir(char *imc_dir, int count)
>>  {
> 

-- 
Thanks
Babu Moger