[PATCH 0/4] selftests/resctrl: Enable MBM and MBA tests on AMD

[PATCH 0/4] selftests/resctrl: Enable MBM and MBA tests on AMD

[Babu Moger]

The MBM (Memory Bandwidth Monitoring) and MBA (Memory Bandwidth Allocation)
features are not enabled for AMD systems. The reason was lack of perf
counters to compare the resctrl test results.

Starting with the commit
25e56847821f ("perf/x86/amd/uncore: Add memory controller support"), AMD
now supports the UMC (Unified Memory Controller) perf events. These events
can be used to compare the test results.

This series adds the support to detect the UMC events and enable MBM/MBA
tests for AMD systems.


Babu Moger (4):
  selftests/resctrl: Rename variable imcs and num_of_imcs() to generic
    names
  selftests/resctrl: Pass sysfs controller name of the vendor
  selftests/resctrl: Add support for MBM and MBA tests on AMD
  selftests/resctrl: Skip the tests if iMC/UMC counters are unavailable

 tools/testing/selftests/resctrl/resctrl.h     |   1 +
 .../testing/selftests/resctrl/resctrl_tests.c |  16 ++-
 tools/testing/selftests/resctrl/resctrl_val.c | 105 ++++++++++++++----
 3 files changed, 96 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names

[Babu Moger]

In an effort to support MBM and MBA tests for AMD, renaming for variable
and functions to generic names. For Intel, the memory controller is called
Integrated Memory Controllers (IMC). For AMD, it is called Unified
Memory Controller (UMC). No functional change.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 25 ++++++++++---------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 88789678917b..52e23a8076d3 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -60,7 +60,7 @@ struct imc_counter_config {
 };
 
 static char mbm_total_path[1024];
-static int imcs;
+static int number_of_mem_ctrls;
 static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
 
 void membw_initialize_perf_event_attr(int i, int j)
@@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
 }
 
 /*
- * A system can have 'n' number of iMC (Integrated Memory Controller)
- * counters, get that 'n'. For each iMC counter get it's type and config.
+ * A system can have 'n' number of iMC (Integrated Memory Controller for
+ * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
+ * Memory Controller). For each iMC/UMC counter get it's type and config.
  * Also, each counter has two configs, one for read and the other for write.
  * A config again has two parts, event and umask.
  * Enumerate all these details into an array of structures.
  *
  * Return: >= 0 on success. < 0 on failure.
  */
-static int num_of_imcs(void)
+static int get_number_of_mem_ctrls(void)
 {
 	char imc_dir[512], *temp;
 	unsigned int count = 0;
@@ -279,12 +280,12 @@ static int initialize_mem_bw_imc(void)
 {
 	int imc, j;
 
-	imcs = num_of_imcs();
-	if (imcs <= 0)
-		return imcs;
+	number_of_mem_ctrls = get_number_of_mem_ctrls();
+	if (number_of_mem_ctrls <= 0)
+		return number_of_mem_ctrls;
 
 	/* Initialize perf_event_attr structures for all iMC's */
-	for (imc = 0; imc < imcs; imc++) {
+	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
 		for (j = 0; j < 2; j++)
 			membw_initialize_perf_event_attr(imc, j);
 	}
@@ -309,7 +310,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 
 	/* Start all iMC counters to log values (both read and write) */
 	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
-	for (imc = 0; imc < imcs; imc++) {
+	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
 		for (j = 0; j < 2; j++) {
 			ret = open_perf_event(imc, cpu_no, j);
 			if (ret)
@@ -322,7 +323,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 	sleep(1);
 
 	/* Stop counters after a second to get results (both read and write) */
-	for (imc = 0; imc < imcs; imc++) {
+	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
 		for (j = 0; j < 2; j++)
 			membw_ioctl_perf_event_ioc_disable(imc, j);
 	}
@@ -331,7 +332,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 	 * Get results which are stored in struct type imc_counter_config
 	 * Take over flow into consideration before calculating total b/w
 	 */
-	for (imc = 0; imc < imcs; imc++) {
+	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
 		struct imc_counter_config *r =
 			&imc_counters_config[imc][READ];
 		struct imc_counter_config *w =
@@ -368,7 +369,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		writes += w->return_value.value * of_mul_write * SCALE;
 	}
 
-	for (imc = 0; imc < imcs; imc++) {
+	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
 		close(imc_counters_config[imc][READ].fd);
 		close(imc_counters_config[imc][WRITE].fd);
 	}
-- 
2.34.1

Re: [PATCH 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names

[Ilpo Järvinen]

On Thu, 22 Feb 2024, Babu Moger wrote:

> In an effort to support MBM and MBA tests for AMD, renaming for variable
> and functions to generic names. For Intel, the memory controller is called
> Integrated Memory Controllers (IMC). For AMD, it is called Unified
> Memory Controller (UMC). No functional change.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 25 ++++++++++---------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 88789678917b..52e23a8076d3 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -60,7 +60,7 @@ struct imc_counter_config {
>  };
>  
>  static char mbm_total_path[1024];
> -static int imcs;
> +static int number_of_mem_ctrls;
>  static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
>  
>  void membw_initialize_perf_event_attr(int i, int j)
> @@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
>  }
>  
>  /*
> - * A system can have 'n' number of iMC (Integrated Memory Controller)
> - * counters, get that 'n'. For each iMC counter get it's type and config.
> + * A system can have 'n' number of iMC (Integrated Memory Controller for
> + * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
> + * Memory Controller). For each iMC/UMC counter get it's type and config.
>   * Also, each counter has two configs, one for read and the other for write.
>   * A config again has two parts, event and umask.
>   * Enumerate all these details into an array of structures.
>   *
>   * Return: >= 0 on success. < 0 on failure.
>   */
> -static int num_of_imcs(void)
> +static int get_number_of_mem_ctrls(void)

It's a bit odd to shorten "memory" and "controller" but not "number". In 
fact, I'd prefer to use "controllers" in the longer form because ctrls 
is ambiguous ("control" vs "controller").

So perhaps num_of_mem_controllers(void) (or that with get_ prefix if you 
insist).

>  {
>  	char imc_dir[512], *temp;
>  	unsigned int count = 0;
> @@ -279,12 +280,12 @@ static int initialize_mem_bw_imc(void)
>  {
>  	int imc, j;
>  
> -	imcs = num_of_imcs();
> -	if (imcs <= 0)
> -		return imcs;
> +	number_of_mem_ctrls = get_number_of_mem_ctrls();
> +	if (number_of_mem_ctrls <= 0)
> +		return number_of_mem_ctrls;
>  
>  	/* Initialize perf_event_attr structures for all iMC's */
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {

So you renamed imcs, but not imc. Perhaps the variable names could just be 
"mc" and "mcs"?

>  		for (j = 0; j < 2; j++)
>  			membw_initialize_perf_event_attr(imc, j);
>  	}
> @@ -309,7 +310,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>  
>  	/* Start all iMC counters to log values (both read and write) */
>  	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>  		for (j = 0; j < 2; j++) {
>  			ret = open_perf_event(imc, cpu_no, j);
>  			if (ret)
> @@ -322,7 +323,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>  	sleep(1);
>  
>  	/* Stop counters after a second to get results (both read and write) */
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>  		for (j = 0; j < 2; j++)
>  			membw_ioctl_perf_event_ioc_disable(imc, j);
>  	}
> @@ -331,7 +332,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>  	 * Get results which are stored in struct type imc_counter_config
>  	 * Take over flow into consideration before calculating total b/w
>  	 */
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>  		struct imc_counter_config *r =
>  			&imc_counters_config[imc][READ];
>  		struct imc_counter_config *w =
> @@ -368,7 +369,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>  		writes += w->return_value.value * of_mul_write * SCALE;
>  	}
>  
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>  		close(imc_counters_config[imc][READ].fd);
>  		close(imc_counters_config[imc][WRITE].fd);

I've a yet submitted patch to convert these close() calls to use the 
loop approach which is consistent with the rest of the code, since you
will end up touching this when you do imc -> mc rename, it would be 
preferrable if you add one patch into your series which converts them to:

		for (j = 0; j < 2; j++)
			close(imc_counters_config[mc][j].fd);

(Given how long kselftest patches tend to linger unapplied, it would 
make things a lot easier since those changes will obviously conflict 
otherwise).

-- 
 i.

Re: [PATCH 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names

[Moger, Babu]

Hi Ilpo,

On 2/23/24 04:38, Ilpo Järvinen wrote:
> On Thu, 22 Feb 2024, Babu Moger wrote:
> 
>> In an effort to support MBM and MBA tests for AMD, renaming for variable
>> and functions to generic names. For Intel, the memory controller is called
>> Integrated Memory Controllers (IMC). For AMD, it is called Unified
>> Memory Controller (UMC). No functional change.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 25 ++++++++++---------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 88789678917b..52e23a8076d3 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -60,7 +60,7 @@ struct imc_counter_config {
>>  };
>>  
>>  static char mbm_total_path[1024];
>> -static int imcs;
>> +static int number_of_mem_ctrls;
>>  static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
>>  
>>  void membw_initialize_perf_event_attr(int i, int j)
>> @@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
>>  }
>>  
>>  /*
>> - * A system can have 'n' number of iMC (Integrated Memory Controller)
>> - * counters, get that 'n'. For each iMC counter get it's type and config.
>> + * A system can have 'n' number of iMC (Integrated Memory Controller for
>> + * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
>> + * Memory Controller). For each iMC/UMC counter get it's type and config.
>>   * Also, each counter has two configs, one for read and the other for write.
>>   * A config again has two parts, event and umask.
>>   * Enumerate all these details into an array of structures.
>>   *
>>   * Return: >= 0 on success. < 0 on failure.
>>   */
>> -static int num_of_imcs(void)
>> +static int get_number_of_mem_ctrls(void)
> 
> It's a bit odd to shorten "memory" and "controller" but not "number". In 
> fact, I'd prefer to use "controllers" in the longer form because ctrls 
> is ambiguous ("control" vs "controller").
> 
> So perhaps num_of_mem_controllers(void) (or that with get_ prefix if you 
> insist).

Sure. num_of_mem_controllers looks good.

> 
>>  {
>>  	char imc_dir[512], *temp;
>>  	unsigned int count = 0;
>> @@ -279,12 +280,12 @@ static int initialize_mem_bw_imc(void)
>>  {
>>  	int imc, j;
>>  
>> -	imcs = num_of_imcs();
>> -	if (imcs <= 0)
>> -		return imcs;
>> +	number_of_mem_ctrls = get_number_of_mem_ctrls();
>> +	if (number_of_mem_ctrls <= 0)
>> +		return number_of_mem_ctrls;
>>  
>>  	/* Initialize perf_event_attr structures for all iMC's */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
> 
> So you renamed imcs, but not imc. Perhaps the variable names could just be 
> "mc" and "mcs"?

How about mem_ctrls ?
> 
>>  		for (j = 0; j < 2; j++)
>>  			membw_initialize_perf_event_attr(imc, j);
>>  	}
>> @@ -309,7 +310,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  
>>  	/* Start all iMC counters to log values (both read and write) */
>>  	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		for (j = 0; j < 2; j++) {
>>  			ret = open_perf_event(imc, cpu_no, j);
>>  			if (ret)
>> @@ -322,7 +323,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  	sleep(1);
>>  
>>  	/* Stop counters after a second to get results (both read and write) */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		for (j = 0; j < 2; j++)
>>  			membw_ioctl_perf_event_ioc_disable(imc, j);
>>  	}
>> @@ -331,7 +332,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  	 * Get results which are stored in struct type imc_counter_config
>>  	 * Take over flow into consideration before calculating total b/w
>>  	 */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		struct imc_counter_config *r =
>>  			&imc_counters_config[imc][READ];
>>  		struct imc_counter_config *w =
>> @@ -368,7 +369,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  		writes += w->return_value.value * of_mul_write * SCALE;
>>  	}
>>  
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		close(imc_counters_config[imc][READ].fd);
>>  		close(imc_counters_config[imc][WRITE].fd);
> 
> I've a yet submitted patch to convert these close() calls to use the 
> loop approach which is consistent with the rest of the code, since you
> will end up touching this when you do imc -> mc rename, it would be 
> preferrable if you add one patch into your series which converts them to:
> 
> 		for (j = 0; j < 2; j++)
> 			close(imc_counters_config[mc][j].fd);

Sure. Will do.

> 
> (Given how long kselftest patches tend to linger unapplied, it would 
> make things a lot easier since those changes will obviously conflict 
> otherwise).
> 

Actually, I can wait for you to submit your patches.  Let me know.

-- 
Thanks
Babu Moger

[PATCH 2/4] selftests/resctrl: Pass sysfs controller name of the vendor

[Babu Moger]

Detect the vendor and pass the sysfs name for the vendor for searching
the controller information.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 52e23a8076d3..792116d3565f 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -224,14 +224,24 @@ static int get_number_of_mem_ctrls(void)
 {
 	char imc_dir[512], *temp;
 	unsigned int count = 0;
+	int ret, vendor, size;
 	struct dirent *ep;
-	int ret;
+	char *sysfs_name;
 	DIR *dp;
 
+	vendor = get_vendor();
+	if (vendor == ARCH_INTEL) {
+		sysfs_name = UNCORE_IMC;
+		size = sizeof(UNCORE_IMC);
+	} else {
+		perror("Unsupported Vendor!\n");
+		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;
 
@@ -242,7 +252,7 @@ static int get_number_of_mem_ctrls(void)
 			 * well and hence the last underscore character in
 			 * uncore_imc'_' need not be counted.
 			 */
-			temp = temp + sizeof(UNCORE_IMC);
+			temp = temp + size;
 
 			/*
 			 * Some directories under "DYN_PMU_PATH" could have
-- 
2.34.1

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

[Babu Moger]

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>
---
 .../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;
 	}
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];
+	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");
+
+		return -1;
+	}
+	if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) {
+		perror("Could not get imc type");
+		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)
 {
@@ -233,6 +275,9 @@ static int get_number_of_mem_ctrls(void)
 	if (vendor == ARCH_INTEL) {
 		sysfs_name = UNCORE_IMC;
 		size = sizeof(UNCORE_IMC);
+	} else if (vendor == ARCH_AMD) {
+		sysfs_name = AMD_UMC;
+		size = sizeof(AMD_UMC);
 	} else {
 		perror("Unsupported Vendor!\n");
 		return -1;
@@ -246,11 +291,12 @@ static int get_number_of_mem_ctrls(void)
 				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.
+			 * Intel 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.
+			 * For AMD, it will be amd_umc_<n>.
 			 */
 			temp = temp + size;
 
@@ -262,7 +308,11 @@ static int get_number_of_mem_ctrls(void)
 			if (temp[0] >= '0' && temp[0] <= '9') {
 				sprintf(imc_dir, "%s/%s/", DYN_PMU_PATH,
 					ep->d_name);
-				ret = read_from_imc_dir(imc_dir, count);
+				if (vendor == ARCH_INTEL)
+					ret = read_from_imc_dir(imc_dir, count);
+				else
+					ret = read_from_umc_dir(imc_dir, count);
+
 				if (ret) {
 					closedir(dp);
 
-- 
2.34.1

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

[Ilpo Järvinen]

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@amd.com>
> ---
>  .../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.

> 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

> +	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().

> +

Drop this newline (to slowly move towards more concise error handling 
blocks w/o all those extra newlines).

> +		return -1;


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

Ditto.

> +		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)
>  {

-- 
 i.

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@amd.com>
>> ---
>>  .../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

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

[Moger, Babu]

On 2/23/24 13:30, Moger, Babu wrote:
> 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@amd.com>
>>> ---
>>>  .../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.

Oh. ok. Here it is
https://lore.kernel.org/lkml/cover.1708072203.git.maciej.wieczor-retman@intel.com/

I will rebase it on top of this next revision.

Thanks
Babu

> 
>>
>>> 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

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

[Reinette Chatre]

Hi Babu,

On 2/23/2024 11:47 AM, Moger, Babu wrote:
> On 2/23/24 13:30, Moger, Babu wrote:
>> On 2/23/24 04:53, Ilpo Järvinen wrote:
>>> On Thu, 22 Feb 2024, Babu Moger wrote:
>>>> @@ -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.
> 
> Oh. ok. Here it is
> https://lore.kernel.org/lkml/cover.1708072203.git.maciej.wieczor-retman@intel.com/
> 
> I will rebase it on top of this next revision.

The resctrl selftest changes are routed through the kselftest repo. Shuah just picked
up this series you noticed but it is based on the framework changes from Ilpo that
is already merged. I recommend that you base your next series on the "next" branch
of the kselftest repo [1].

Reinette

[1] git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git

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

[Moger, Babu]

Hi Reinette

On 2/23/24 16:39, Reinette Chatre wrote:
> Hi Babu,
> 
> On 2/23/2024 11:47 AM, Moger, Babu wrote:
>> On 2/23/24 13:30, Moger, Babu wrote:
>>> On 2/23/24 04:53, Ilpo Järvinen wrote:
>>>> On Thu, 22 Feb 2024, Babu Moger wrote:
>>>>> @@ -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.
>>
>> Oh. ok. Here it is
>> https://lore.kernel.org/lkml/cover.1708072203.git.maciej.wieczor-retman@intel.com/
>>
>> I will rebase it on top of this next revision.
> 
> The resctrl selftest changes are routed through the kselftest repo. Shuah just picked
> up this series you noticed but it is based on the framework changes from Ilpo that
> is already merged. I recommend that you base your next series on the "next" branch
> of the kselftest repo [1].

Sure. Will do.
> 
> Reinette
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git

-- 
Thanks
Babu Moger

[PATCH 4/4] selftests/resctrl: Skip the tests if iMC/UMC counters are unavailable

[Babu Moger]

Older systems do not support UMC (Unified Memory Controller) perf counters.
Skip the tests if the system does not support UMC counters.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 tools/testing/selftests/resctrl/resctrl.h       |  1 +
 tools/testing/selftests/resctrl/resctrl_tests.c | 10 ++++++++++
 tools/testing/selftests/resctrl/resctrl_val.c   |  4 ++--
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index a33f414f6019..5c2556af0649 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -116,5 +116,6 @@ int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
 		    size_t cache_span, unsigned long max_diff,
 		    unsigned long max_diff_percent, unsigned long num_of_runs,
 		    bool platform, bool cmt);
+int get_number_of_mem_ctrls(void);
 
 #endif /* RESCTRL_H */
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 231233b8d354..5423882529ec 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -98,6 +98,11 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
 
 	ksft_print_msg("Starting MBM BW change ...\n");
 
+	if (get_number_of_mem_ctrls() < 0) {
+		ksft_test_result_skip("Unable find iMC/UMC counters!\n");
+		return;
+	}
+
 	if (test_prepare()) {
 		ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
 		return;
@@ -124,6 +129,11 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
 
 	ksft_print_msg("Starting MBA Schemata change ...\n");
 
+	if (get_number_of_mem_ctrls() < 0) {
+		ksft_test_result_skip("Unable find iMC/UMC counters!\n");
+		return;
+	}
+
 	if (test_prepare()) {
 		ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
 		return;
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index c5a4607aa9d9..68cee87c26ef 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -262,7 +262,7 @@ static int read_from_imc_dir(char *imc_dir, int count)
  *
  * Return: >= 0 on success. < 0 on failure.
  */
-static int get_number_of_mem_ctrls(void)
+int get_number_of_mem_ctrls(void)
 {
 	char imc_dir[512], *temp;
 	unsigned int count = 0;
@@ -323,7 +323,7 @@ static int get_number_of_mem_ctrls(void)
 		}
 		closedir(dp);
 		if (count == 0) {
-			perror("Unable find iMC counters!\n");
+			perror("Unable find iMC/UMC counters!\n");
 
 			return -1;
 		}
-- 
2.34.1

Re: [PATCH 4/4] selftests/resctrl: Skip the tests if iMC/UMC counters are unavailable

[Ilpo Järvinen]

On Thu, 22 Feb 2024, Babu Moger wrote:

> Older systems do not support UMC (Unified Memory Controller) perf counters.
> Skip the tests if the system does not support UMC counters.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  tools/testing/selftests/resctrl/resctrl.h       |  1 +
>  tools/testing/selftests/resctrl/resctrl_tests.c | 10 ++++++++++
>  tools/testing/selftests/resctrl/resctrl_val.c   |  4 ++--
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a33f414f6019..5c2556af0649 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -116,5 +116,6 @@ int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
>  		    size_t cache_span, unsigned long max_diff,
>  		    unsigned long max_diff_percent, unsigned long num_of_runs,
>  		    bool platform, bool cmt);
> +int get_number_of_mem_ctrls(void);
>  
>  #endif /* RESCTRL_H */
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 231233b8d354..5423882529ec 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -98,6 +98,11 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
>  
>  	ksft_print_msg("Starting MBM BW change ...\n");
>  
> +	if (get_number_of_mem_ctrls() < 0) {
> +		ksft_test_result_skip("Unable find iMC/UMC counters!\n");
> +		return;
> +	}
> +
>  	if (test_prepare()) {
>  		ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
>  		return;
> @@ -124,6 +129,11 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
>  
>  	ksft_print_msg("Starting MBA Schemata change ...\n");
>  
> +	if (get_number_of_mem_ctrls() < 0) {
> +		ksft_test_result_skip("Unable find iMC/UMC counters!\n");
> +		return;
> +	}
> +
>  	if (test_prepare()) {
>  		ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
>  		return;

This also needs rebasing and adaptation to the generic test framework.

-- 
 i.

Re: [PATCH 4/4] selftests/resctrl: Skip the tests if iMC/UMC counters are unavailable

[Moger, Babu]

Hi Ilpo,

On 2/23/24 04:56, Ilpo Järvinen wrote:
> On Thu, 22 Feb 2024, Babu Moger wrote:
> 
>> Older systems do not support UMC (Unified Memory Controller) perf counters.
>> Skip the tests if the system does not support UMC counters.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  tools/testing/selftests/resctrl/resctrl.h       |  1 +
>>  tools/testing/selftests/resctrl/resctrl_tests.c | 10 ++++++++++
>>  tools/testing/selftests/resctrl/resctrl_val.c   |  4 ++--
>>  3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index a33f414f6019..5c2556af0649 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -116,5 +116,6 @@ int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
>>  		    size_t cache_span, unsigned long max_diff,
>>  		    unsigned long max_diff_percent, unsigned long num_of_runs,
>>  		    bool platform, bool cmt);
>> +int get_number_of_mem_ctrls(void);
>>  
>>  #endif /* RESCTRL_H */
>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
>> index 231233b8d354..5423882529ec 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>> @@ -98,6 +98,11 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
>>  
>>  	ksft_print_msg("Starting MBM BW change ...\n");
>>  
>> +	if (get_number_of_mem_ctrls() < 0) {
>> +		ksft_test_result_skip("Unable find iMC/UMC counters!\n");
>> +		return;
>> +	}
>> +
>>  	if (test_prepare()) {
>>  		ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
>>  		return;
>> @@ -124,6 +129,11 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
>>  
>>  	ksft_print_msg("Starting MBA Schemata change ...\n");
>>  
>> +	if (get_number_of_mem_ctrls() < 0) {
>> +		ksft_test_result_skip("Unable find iMC/UMC counters!\n");
>> +		return;
>> +	}
>> +
>>  	if (test_prepare()) {
>>  		ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
>>  		return;
> 
> This also needs rebasing and adaptation to the generic test framework.

Looks like I need to wait for your patches to merge first.
Thanks
Babu Moger

[PATCH v2 0/4] selftests/resctrl: Enable MBM and MBA tests on AMD

[Babu Moger]

The MBM (Memory Bandwidth Monitoring) and MBA (Memory Bandwidth Allocation)
features are not enabled for AMD systems. The reason was lack of perf
counters to compare the resctrl test results.

Starting with the commit
25e56847821f ("perf/x86/amd/uncore: Add memory controller support"), AMD
now supports the UMC (Unified Memory Controller) perf events. These events
can be used to compare the test results.

This series adds the support to detect the UMC events and enable MBM/MBA
tests for AMD systems.

v2: Changes.
    a. Rebased on top of tip/master (Apr 25, 2024)
    b. Addressed Ilpo comments except the one about close call.
       It seems more clear to keep READ and WRITE separate.
       https://lore.kernel.org/lkml/8e4badb7-6cc5-61f1-e041-d902209a90d5@linux.intel.com/
    c. Used ksft_perror call when applicable.
    d. Added vendor check for non contiguous CBM check.
  
v1: https://lore.kernel.org/lkml/cover.1708637563.git.babu.moger@amd.com/

Babu Moger (4):
  selftests/resctrl: Rename variable imcs and num_of_imcs() to generic
    names
  selftests/resctrl: Pass sysfs controller name of the vendor
  selftests/resctrl: Add support for MBM and MBA tests on AMD
  selftests/resctrl: Enable MBA/MBA tests on AMD

 tools/testing/selftests/resctrl/cat_test.c    |   2 +-
 tools/testing/selftests/resctrl/mba_test.c    |   1 -
 tools/testing/selftests/resctrl/mbm_test.c    |   1 -
 tools/testing/selftests/resctrl/resctrl_val.c | 142 +++++++++++++-----
 4 files changed, 103 insertions(+), 43 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names

[Babu Moger]

In an effort to support MBM and MBA tests for AMD, renaming for variable
and functions to generic names. For Intel, the memory controller is called
Integrated Memory Controllers (IMC). For AMD, it is called Unified
Memory Controller (UMC). No functional change.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 59 ++++++++++---------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 5a49f07a6c85..a30cfcff605f 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -60,7 +60,7 @@ struct imc_counter_config {
 };
 
 static char mbm_total_path[1024];
-static int imcs;
+static int mcs;
 static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
 
 void membw_initialize_perf_event_attr(int i, int j)
@@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
 }
 
 /*
- * A system can have 'n' number of iMC (Integrated Memory Controller)
- * counters, get that 'n'. For each iMC counter get it's type and config.
+ * A system can have 'n' number of iMC (Integrated Memory Controller for
+ * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
+ * Memory Controller). For each iMC/UMC counter get it's type and config.
  * Also, each counter has two configs, one for read and the other for write.
  * A config again has two parts, event and umask.
  * Enumerate all these details into an array of structures.
  *
  * Return: >= 0 on success. < 0 on failure.
  */
-static int num_of_imcs(void)
+static int num_of_mem_controllers(void)
 {
 	char imc_dir[512], *temp;
 	unsigned int count = 0;
@@ -275,25 +276,25 @@ static int num_of_imcs(void)
 	return count;
 }
 
-static int initialize_mem_bw_imc(void)
+static int initialize_mem_bw_mc(void)
 {
-	int imc, j;
+	int mc, j;
 
-	imcs = num_of_imcs();
-	if (imcs <= 0)
-		return imcs;
+	mcs = num_of_mem_controllers();
+	if (mcs <= 0)
+		return mcs;
 
 	/* Initialize perf_event_attr structures for all iMC's */
-	for (imc = 0; imc < imcs; imc++) {
+	for (mc = 0; mc < mcs; mc++) {
 		for (j = 0; j < 2; j++)
-			membw_initialize_perf_event_attr(imc, j);
+			membw_initialize_perf_event_attr(mc, j);
 	}
 
 	return 0;
 }
 
 /*
- * get_mem_bw_imc:	Memory band width as reported by iMC counters
+ * get_mem_bw_mc:	Memory band width as reported by iMC counters
  * @cpu_no:		CPU number that the benchmark PID is binded to
  * @bw_report:		Bandwidth report type (reads, writes)
  *
@@ -302,40 +303,40 @@ static int initialize_mem_bw_imc(void)
  *
  * Return: = 0 on success. < 0 on failure.
  */
-static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
+static int get_mem_bw_mc(int cpu_no, char *bw_report, float *bw_imc)
 {
 	float reads, writes, of_mul_read, of_mul_write;
-	int imc, j, ret;
+	int mc, j, ret;
 
 	/* Start all iMC counters to log values (both read and write) */
 	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
-	for (imc = 0; imc < imcs; imc++) {
+	for (mc = 0; mc < mcs; mc++) {
 		for (j = 0; j < 2; j++) {
-			ret = open_perf_event(imc, cpu_no, j);
+			ret = open_perf_event(mc, cpu_no, j);
 			if (ret)
 				return -1;
 		}
 		for (j = 0; j < 2; j++)
-			membw_ioctl_perf_event_ioc_reset_enable(imc, j);
+			membw_ioctl_perf_event_ioc_reset_enable(mc, j);
 	}
 
 	sleep(1);
 
 	/* Stop counters after a second to get results (both read and write) */
-	for (imc = 0; imc < imcs; imc++) {
+	for (mc = 0; mc < mcs; mc++) {
 		for (j = 0; j < 2; j++)
-			membw_ioctl_perf_event_ioc_disable(imc, j);
+			membw_ioctl_perf_event_ioc_disable(mc, j);
 	}
 
 	/*
 	 * Get results which are stored in struct type imc_counter_config
 	 * Take over flow into consideration before calculating total b/w
 	 */
-	for (imc = 0; imc < imcs; imc++) {
+	for (mc = 0; mc < mcs; mc++) {
 		struct imc_counter_config *r =
-			&imc_counters_config[imc][READ];
+			&imc_counters_config[mc][READ];
 		struct imc_counter_config *w =
-			&imc_counters_config[imc][WRITE];
+			&imc_counters_config[mc][WRITE];
 
 		if (read(r->fd, &r->return_value,
 			 sizeof(struct membw_read_format)) == -1) {
@@ -368,9 +369,9 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		writes += w->return_value.value * of_mul_write * SCALE;
 	}
 
-	for (imc = 0; imc < imcs; imc++) {
-		close(imc_counters_config[imc][READ].fd);
-		close(imc_counters_config[imc][WRITE].fd);
+	for (mc = 0; mc < mcs; mc++) {
+		close(imc_counters_config[mc][READ].fd);
+		close(imc_counters_config[mc][WRITE].fd);
 	}
 
 	if (strcmp(bw_report, "reads") == 0) {
@@ -598,7 +599,7 @@ static int measure_vals(const struct user_params *uparams,
 			unsigned long *bw_resc_start)
 {
 	unsigned long bw_resc, bw_resc_end;
-	float bw_imc;
+	float bw_mc;
 	int ret;
 
 	/*
@@ -608,7 +609,7 @@ static int measure_vals(const struct user_params *uparams,
 	 * Compare the two values to validate resctrl value.
 	 * It takes 1sec to measure the data.
 	 */
-	ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
+	ret = get_mem_bw_mc(uparams->cpu, param->bw_report, &bw_mc);
 	if (ret < 0)
 		return ret;
 
@@ -617,7 +618,7 @@ static int measure_vals(const struct user_params *uparams,
 		return ret;
 
 	bw_resc = (bw_resc_end - *bw_resc_start) / MB;
-	ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
+	ret = print_results_bw(param->filename, bm_pid, bw_mc, bw_resc);
 	if (ret)
 		return ret;
 
@@ -795,7 +796,7 @@ int resctrl_val(const struct resctrl_test *test,
 
 	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
 	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-		ret = initialize_mem_bw_imc();
+		ret = initialize_mem_bw_mc();
 		if (ret)
 			goto out;
 
-- 
2.34.1

Re: [PATCH v2 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names

[Reinette Chatre]

Hi Babu,

On 4/25/2024 1:16 PM, Babu Moger wrote:
> In an effort to support MBM and MBA tests for AMD, renaming for variable
> and functions to generic names. For Intel, the memory controller is called
> Integrated Memory Controllers (IMC). For AMD, it is called Unified
> Memory Controller (UMC). No functional change.

This is a resonable change yet the actual changes seem inconsistent to me.
Per the changelog the goal is to switch from "IMC" specific naming to generic
"MC" naming in all the code that will be shared between AMD and Intel.
From what I can tell this patch only changes *some* of the shared variables,
functions, and data structures and it is not obvious to me why some are
changed and some are not. This makes the code inconsistent.

There are many examples of the inconsistencies in this patch alone that
I will try to highlight what I mean without considering areas untouched by
this patch.
 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 59 ++++++++++---------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 5a49f07a6c85..a30cfcff605f 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -60,7 +60,7 @@ struct imc_counter_config {
>  };
>  
>  static char mbm_total_path[1024];
> -static int imcs;
> +static int mcs;
>  static struct imc_counter_config imc_counters_config[MAX_IMCS][2];

Global "imcs" is changed to "mcs" ... but why are
global imc_counters_config[][] and its struct imc_counter_config
not changed?

>  
>  void membw_initialize_perf_event_attr(int i, int j)
> @@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
>  }
>  
>  /*
> - * A system can have 'n' number of iMC (Integrated Memory Controller)
> - * counters, get that 'n'. For each iMC counter get it's type and config.
> + * A system can have 'n' number of iMC (Integrated Memory Controller for
> + * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
> + * Memory Controller). For each iMC/UMC counter get it's type and config.
>   * Also, each counter has two configs, one for read and the other for write.
>   * A config again has two parts, event and umask.
>   * Enumerate all these details into an array of structures.
>   *
>   * Return: >= 0 on success. < 0 on failure.
>   */
> -static int num_of_imcs(void)
> +static int num_of_mem_controllers(void)
>  {
>  	char imc_dir[512], *temp;

Similarly, what about imc_dir[]?

>  	unsigned int count = 0;
> @@ -275,25 +276,25 @@ static int num_of_imcs(void)
>  	return count;
>  }
>  
> -static int initialize_mem_bw_imc(void)
> +static int initialize_mem_bw_mc(void)
>  {
> -	int imc, j;
> +	int mc, j;
>  
> -	imcs = num_of_imcs();
> -	if (imcs <= 0)
> -		return imcs;
> +	mcs = num_of_mem_controllers();
> +	if (mcs <= 0)
> +		return mcs;
>  
>  	/* Initialize perf_event_attr structures for all iMC's */

Note comment still refers to iMC

> -	for (imc = 0; imc < imcs; imc++) {
> +	for (mc = 0; mc < mcs; mc++) {
>  		for (j = 0; j < 2; j++)
> -			membw_initialize_perf_event_attr(imc, j);
> +			membw_initialize_perf_event_attr(mc, j);
>  	}
>  
>  	return 0;
>  }
>  
>  /*
> - * get_mem_bw_imc:	Memory band width as reported by iMC counters
> + * get_mem_bw_mc:	Memory band width as reported by iMC counters

Comment still refers to iMC

>   * @cpu_no:		CPU number that the benchmark PID is binded to
>   * @bw_report:		Bandwidth report type (reads, writes)
>   *
> @@ -302,40 +303,40 @@ static int initialize_mem_bw_imc(void)
>   *
>   * Return: = 0 on success. < 0 on failure.
>   */
> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
> +static int get_mem_bw_mc(int cpu_no, char *bw_report, float *bw_imc)

The intent of the function is to "get" bw_mc ... so not renaming "bw_imc"
seems like a miss. Especially when considering that its caller does just this.

>  {
>  	float reads, writes, of_mul_read, of_mul_write;
> -	int imc, j, ret;
> +	int mc, j, ret;
>  
>  	/* Start all iMC counters to log values (both read and write) */

iMC?

>  	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (mc = 0; mc < mcs; mc++) {
>  		for (j = 0; j < 2; j++) {
> -			ret = open_perf_event(imc, cpu_no, j);
> +			ret = open_perf_event(mc, cpu_no, j);
>  			if (ret)
>  				return -1;
>  		}
>  		for (j = 0; j < 2; j++)
> -			membw_ioctl_perf_event_ioc_reset_enable(imc, j);
> +			membw_ioctl_perf_event_ioc_reset_enable(mc, j);
>  	}
>  
>  	sleep(1);
>  
>  	/* Stop counters after a second to get results (both read and write) */
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (mc = 0; mc < mcs; mc++) {
>  		for (j = 0; j < 2; j++)
> -			membw_ioctl_perf_event_ioc_disable(imc, j);
> +			membw_ioctl_perf_event_ioc_disable(mc, j);
>  	}
>  
>  	/*
>  	 * Get results which are stored in struct type imc_counter_config
>  	 * Take over flow into consideration before calculating total b/w
>  	 */
> -	for (imc = 0; imc < imcs; imc++) {
> +	for (mc = 0; mc < mcs; mc++) {
>  		struct imc_counter_config *r =
> -			&imc_counters_config[imc][READ];
> +			&imc_counters_config[mc][READ];
>  		struct imc_counter_config *w =
> -			&imc_counters_config[imc][WRITE];
> +			&imc_counters_config[mc][WRITE];
>  
>  		if (read(r->fd, &r->return_value,
>  			 sizeof(struct membw_read_format)) == -1) {
> @@ -368,9 +369,9 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>  		writes += w->return_value.value * of_mul_write * SCALE;
>  	}
>  
> -	for (imc = 0; imc < imcs; imc++) {
> -		close(imc_counters_config[imc][READ].fd);
> -		close(imc_counters_config[imc][WRITE].fd);
> +	for (mc = 0; mc < mcs; mc++) {
> +		close(imc_counters_config[mc][READ].fd);
> +		close(imc_counters_config[mc][WRITE].fd);
>  	}
>  
>  	if (strcmp(bw_report, "reads") == 0) {
> @@ -598,7 +599,7 @@ static int measure_vals(const struct user_params *uparams,
>  			unsigned long *bw_resc_start)
>  {
>  	unsigned long bw_resc, bw_resc_end;
> -	float bw_imc;
> +	float bw_mc;
>  	int ret;
>  
>  	/*
> @@ -608,7 +609,7 @@ static int measure_vals(const struct user_params *uparams,
>  	 * Compare the two values to validate resctrl value.
>  	 * It takes 1sec to measure the data.
>  	 */
> -	ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
> +	ret = get_mem_bw_mc(uparams->cpu, param->bw_report, &bw_mc);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -617,7 +618,7 @@ static int measure_vals(const struct user_params *uparams,
>  		return ret;
>  
>  	bw_resc = (bw_resc_end - *bw_resc_start) / MB;
> -	ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
> +	ret = print_results_bw(param->filename, bm_pid, bw_mc, bw_resc);
>  	if (ret)
>  		return ret;
>  
> @@ -795,7 +796,7 @@ int resctrl_val(const struct resctrl_test *test,
>  
>  	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
>  	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> -		ret = initialize_mem_bw_imc();
> +		ret = initialize_mem_bw_mc();
>  		if (ret)
>  			goto out;
>  

Please note that this patch conflicts with other in-progress work [1].

Reinette

[1] https://lore.kernel.org/lkml/20240408163247.3224-1-ilpo.jarvinen@linux.intel.com/

Re: [PATCH v2 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names

[Moger, Babu]

Hi Reinette,
Was doing few other things. Sorry for the delay.

On 5/9/24 16:10, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/25/2024 1:16 PM, Babu Moger wrote:
>> In an effort to support MBM and MBA tests for AMD, renaming for variable
>> and functions to generic names. For Intel, the memory controller is called
>> Integrated Memory Controllers (IMC). For AMD, it is called Unified
>> Memory Controller (UMC). No functional change.
> 
> This is a resonable change yet the actual changes seem inconsistent to me.
> Per the changelog the goal is to switch from "IMC" specific naming to generic
> "MC" naming in all the code that will be shared between AMD and Intel.
>>From what I can tell this patch only changes *some* of the shared variables,
> functions, and data structures and it is not obvious to me why some are
> changed and some are not. This makes the code inconsistent.

Agree. Will address it in next version.

> 
> There are many examples of the inconsistencies in this patch alone that
> I will try to highlight what I mean without considering areas untouched by
> this patch.
>  
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 59 ++++++++++---------
>>  1 file changed, 30 insertions(+), 29 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 5a49f07a6c85..a30cfcff605f 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -60,7 +60,7 @@ struct imc_counter_config {
>>  };
>>  
>>  static char mbm_total_path[1024];
>> -static int imcs;
>> +static int mcs;
>>  static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
> 
> Global "imcs" is changed to "mcs" ... but why are
> global imc_counters_config[][] and its struct imc_counter_config
> not changed?

Yes. Will address it.
> 
>>  
>>  void membw_initialize_perf_event_attr(int i, int j)
>> @@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
>>  }
>>  
>>  /*
>> - * A system can have 'n' number of iMC (Integrated Memory Controller)
>> - * counters, get that 'n'. For each iMC counter get it's type and config.
>> + * A system can have 'n' number of iMC (Integrated Memory Controller for
>> + * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
>> + * Memory Controller). For each iMC/UMC counter get it's type and config.
>>   * Also, each counter has two configs, one for read and the other for write.
>>   * A config again has two parts, event and umask.
>>   * Enumerate all these details into an array of structures.
>>   *
>>   * Return: >= 0 on success. < 0 on failure.
>>   */
>> -static int num_of_imcs(void)
>> +static int num_of_mem_controllers(void)
>>  {
>>  	char imc_dir[512], *temp;
> 
> Similarly, what about imc_dir[]?

Yes. Sure.

> 
>>  	unsigned int count = 0;
>> @@ -275,25 +276,25 @@ static int num_of_imcs(void)
>>  	return count;
>>  }
>>  
>> -static int initialize_mem_bw_imc(void)
>> +static int initialize_mem_bw_mc(void)
>>  {
>> -	int imc, j;
>> +	int mc, j;
>>  
>> -	imcs = num_of_imcs();
>> -	if (imcs <= 0)
>> -		return imcs;
>> +	mcs = num_of_mem_controllers();
>> +	if (mcs <= 0)
>> +		return mcs;
>>  
>>  	/* Initialize perf_event_attr structures for all iMC's */
> 
> Note comment still refers to iMC

Yes.
> 
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (mc = 0; mc < mcs; mc++) {
>>  		for (j = 0; j < 2; j++)
>> -			membw_initialize_perf_event_attr(imc, j);
>> +			membw_initialize_perf_event_attr(mc, j);
>>  	}
>>  
>>  	return 0;
>>  }
>>  
>>  /*
>> - * get_mem_bw_imc:	Memory band width as reported by iMC counters
>> + * get_mem_bw_mc:	Memory band width as reported by iMC counters
> 
> Comment still refers to iMC

Will address it.

> 
>>   * @cpu_no:		CPU number that the benchmark PID is binded to
>>   * @bw_report:		Bandwidth report type (reads, writes)
>>   *
>> @@ -302,40 +303,40 @@ static int initialize_mem_bw_imc(void)
>>   *
>>   * Return: = 0 on success. < 0 on failure.
>>   */
>> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>> +static int get_mem_bw_mc(int cpu_no, char *bw_report, float *bw_imc)
> 
> The intent of the function is to "get" bw_mc ... so not renaming "bw_imc"
> seems like a miss. Especially when considering that its caller does just this.

Yes. Will take care of this.

> 
>>  {
>>  	float reads, writes, of_mul_read, of_mul_write;
>> -	int imc, j, ret;
>> +	int mc, j, ret;
>>  
>>  	/* Start all iMC counters to log values (both read and write) */
> 
> iMC?

Sure.
> 
>>  	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (mc = 0; mc < mcs; mc++) {
>>  		for (j = 0; j < 2; j++) {
>> -			ret = open_perf_event(imc, cpu_no, j);
>> +			ret = open_perf_event(mc, cpu_no, j);
>>  			if (ret)
>>  				return -1;
>>  		}
>>  		for (j = 0; j < 2; j++)
>> -			membw_ioctl_perf_event_ioc_reset_enable(imc, j);
>> +			membw_ioctl_perf_event_ioc_reset_enable(mc, j);
>>  	}
>>  
>>  	sleep(1);
>>  
>>  	/* Stop counters after a second to get results (both read and write) */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (mc = 0; mc < mcs; mc++) {
>>  		for (j = 0; j < 2; j++)
>> -			membw_ioctl_perf_event_ioc_disable(imc, j);
>> +			membw_ioctl_perf_event_ioc_disable(mc, j);
>>  	}
>>  
>>  	/*
>>  	 * Get results which are stored in struct type imc_counter_config
>>  	 * Take over flow into consideration before calculating total b/w
>>  	 */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (mc = 0; mc < mcs; mc++) {
>>  		struct imc_counter_config *r =
>> -			&imc_counters_config[imc][READ];
>> +			&imc_counters_config[mc][READ];
>>  		struct imc_counter_config *w =
>> -			&imc_counters_config[imc][WRITE];
>> +			&imc_counters_config[mc][WRITE];
>>  
>>  		if (read(r->fd, &r->return_value,
>>  			 sizeof(struct membw_read_format)) == -1) {
>> @@ -368,9 +369,9 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  		writes += w->return_value.value * of_mul_write * SCALE;
>>  	}
>>  
>> -	for (imc = 0; imc < imcs; imc++) {
>> -		close(imc_counters_config[imc][READ].fd);
>> -		close(imc_counters_config[imc][WRITE].fd);
>> +	for (mc = 0; mc < mcs; mc++) {
>> +		close(imc_counters_config[mc][READ].fd);
>> +		close(imc_counters_config[mc][WRITE].fd);
>>  	}
>>  
>>  	if (strcmp(bw_report, "reads") == 0) {
>> @@ -598,7 +599,7 @@ static int measure_vals(const struct user_params *uparams,
>>  			unsigned long *bw_resc_start)
>>  {
>>  	unsigned long bw_resc, bw_resc_end;
>> -	float bw_imc;
>> +	float bw_mc;
>>  	int ret;
>>  
>>  	/*
>> @@ -608,7 +609,7 @@ static int measure_vals(const struct user_params *uparams,
>>  	 * Compare the two values to validate resctrl value.
>>  	 * It takes 1sec to measure the data.
>>  	 */
>> -	ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
>> +	ret = get_mem_bw_mc(uparams->cpu, param->bw_report, &bw_mc);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> @@ -617,7 +618,7 @@ static int measure_vals(const struct user_params *uparams,
>>  		return ret;
>>  
>>  	bw_resc = (bw_resc_end - *bw_resc_start) / MB;
>> -	ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
>> +	ret = print_results_bw(param->filename, bm_pid, bw_mc, bw_resc);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -795,7 +796,7 @@ int resctrl_val(const struct resctrl_test *test,
>>  
>>  	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
>>  	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
>> -		ret = initialize_mem_bw_imc();
>> +		ret = initialize_mem_bw_mc();
>>  		if (ret)
>>  			goto out;
>>  
> 
> Please note that this patch conflicts with other in-progress work [1].

Yes. Noted.

> 
> Reinette
> 
> [1] https://lore.kernel.org/lkml/20240408163247.3224-1-ilpo.jarvinen@linux.intel.com/
> 

-- 
Thanks
Babu Moger

[PATCH v2 2/4] selftests/resctrl: Pass sysfs controller name of the vendor

[Babu Moger]

Detect the vendor and pass the sysfs name for the vendor for searching
the controller information.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index a30cfcff605f..e3b09128ec3d 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -224,14 +224,24 @@ static int num_of_mem_controllers(void)
 {
 	char imc_dir[512], *temp;
 	unsigned int count = 0;
+	int ret, vendor, size;
 	struct dirent *ep;
-	int ret;
+	char *sysfs_name;
 	DIR *dp;
 
+	vendor = get_vendor();
+	if (vendor == ARCH_INTEL) {
+		sysfs_name = UNCORE_IMC;
+		size = sizeof(UNCORE_IMC);
+	} else {
+		perror("Unsupported Vendor!\n");
+		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;
 
@@ -242,7 +252,7 @@ static int num_of_mem_controllers(void)
 			 * well and hence the last underscore character in
 			 * uncore_imc'_' need not be counted.
 			 */
-			temp = temp + sizeof(UNCORE_IMC);
+			temp = temp + size;
 
 			/*
 			 * Some directories under "DYN_PMU_PATH" could have
-- 
2.34.1

Re: [PATCH v2 2/4] selftests/resctrl: Pass sysfs controller name of the vendor

[Reinette Chatre]

Hi Babu,

On 4/25/2024 1:17 PM, Babu Moger wrote:
> Detect the vendor and pass the sysfs name for the vendor for searching
> the controller information.

Could you please write a proper changelog?

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index a30cfcff605f..e3b09128ec3d 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -224,14 +224,24 @@ static int num_of_mem_controllers(void)
>  {
>  	char imc_dir[512], *temp;
>  	unsigned int count = 0;
> +	int ret, vendor, size;
>  	struct dirent *ep;
> -	int ret;
> +	char *sysfs_name;
>  	DIR *dp;
>  
> +	vendor = get_vendor();
> +	if (vendor == ARCH_INTEL) {
> +		sysfs_name = UNCORE_IMC;
> +		size = sizeof(UNCORE_IMC);

Why is separate size needed? Can strlen() just be used when needed?

> +	} else {
> +		perror("Unsupported Vendor!\n");

ksft_perror()?

In the message, "Vendor" need not start with capital. It may also
help to print the vendor value in this unlikely case.

> +		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;
>  
> @@ -242,7 +252,7 @@ static int num_of_mem_controllers(void)
>  			 * well and hence the last underscore character in
>  			 * uncore_imc'_' need not be counted.
>  			 */
> -			temp = temp + sizeof(UNCORE_IMC);
> +			temp = temp + size;

strlen()? (Keeping in mind the adjustment for the "_" character).

>  
>  			/*
>  			 * Some directories under "DYN_PMU_PATH" could have

Reinette

Re: [PATCH v2 2/4] selftests/resctrl: Pass sysfs controller name of the vendor

[Moger, Babu]

Hi Reinette,

On 5/9/24 16:11, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/25/2024 1:17 PM, Babu Moger wrote:
>> Detect the vendor and pass the sysfs name for the vendor for searching
>> the controller information.
> 
> Could you please write a proper changelog?

Sure. Will do.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index a30cfcff605f..e3b09128ec3d 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -224,14 +224,24 @@ static int num_of_mem_controllers(void)
>>  {
>>  	char imc_dir[512], *temp;
>>  	unsigned int count = 0;
>> +	int ret, vendor, size;
>>  	struct dirent *ep;
>> -	int ret;
>> +	char *sysfs_name;
>>  	DIR *dp;
>>  
>> +	vendor = get_vendor();
>> +	if (vendor == ARCH_INTEL) {
>> +		sysfs_name = UNCORE_IMC;
>> +		size = sizeof(UNCORE_IMC);
> 
> Why is separate size needed? Can strlen() just be used when needed?

Yes.

> 
>> +	} else {
>> +		perror("Unsupported Vendor!\n");
> 
> ksft_perror()?
> 
> In the message, "Vendor" need not start with capital. It may also
> help to print the vendor value in this unlikely case.

ok. Sure.

> 
>> +		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;
>>  
>> @@ -242,7 +252,7 @@ static int num_of_mem_controllers(void)
>>  			 * well and hence the last underscore character in
>>  			 * uncore_imc'_' need not be counted.
>>  			 */
>> -			temp = temp + sizeof(UNCORE_IMC);
>> +			temp = temp + size;
> 
> strlen()? (Keeping in mind the adjustment for the "_" character).

Sure.
> 
>>  
>>  			/*
>>  			 * Some directories under "DYN_PMU_PATH" could have
> 
> Reinette
> 

-- 
Thanks
Babu Moger

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

[Babu Moger]

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 */
+	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");
+		return -1;
+	}
+
+	if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) {
+		ksft_perror("Could not get imc type");
+		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)
 {
@@ -233,6 +275,9 @@ static int num_of_mem_controllers(void)
 	if (vendor == ARCH_INTEL) {
 		sysfs_name = UNCORE_IMC;
 		size = sizeof(UNCORE_IMC);
+	} else if (vendor == ARCH_AMD) {
+		sysfs_name = AMD_UMC;
+		size = sizeof(AMD_UMC);
 	} else {
 		perror("Unsupported Vendor!\n");
 		return -1;
@@ -246,11 +291,12 @@ static int num_of_mem_controllers(void)
 				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.
+			 * Intel 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.
+			 * For AMD, it will be amd_umc_<n>.
 			 */
 			temp = temp + size;
 
@@ -262,7 +308,11 @@ static int num_of_mem_controllers(void)
 			if (temp[0] >= '0' && temp[0] <= '9') {
 				sprintf(imc_dir, "%s/%s/", DYN_PMU_PATH,
 					ep->d_name);
-				ret = read_from_imc_dir(imc_dir, count);
+				if (vendor == ARCH_INTEL)
+					ret = read_from_imc_dir(imc_dir, count);
+				else
+					ret = read_from_umc_dir(imc_dir, count);
+
 				if (ret) {
 					closedir(dp);
 
@@ -273,8 +323,9 @@ static int num_of_mem_controllers(void)
 		}
 		closedir(dp);
 		if (count == 0) {
-			ksft_print_msg("Unable to find iMC 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 {
-- 
2.34.1

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

[Reinette Chatre]

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?

> +	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?

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

Same here.

> +		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?

Reinette

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

[Moger, Babu]

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

[PATCH v2 4/4] selftests/resctrl: Enable MBA/MBA tests on AMD

[Babu Moger]

Enable MBA/MBM tests if UMC (Unified Memory Controller) support is
available on the system. Tests will be skipped otherwise.

Update noncont_cat_run_test to check for vendor. AMD supports
non contiguous CBM masks but does not report it via CPUID.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 tools/testing/selftests/resctrl/cat_test.c | 2 +-
 tools/testing/selftests/resctrl/mba_test.c | 1 -
 tools/testing/selftests/resctrl/mbm_test.c | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 4cb991be8e31..b682eaf65bfd 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -314,7 +314,7 @@ static int noncont_cat_run_test(const struct resctrl_test *test,
 	else
 		return -EINVAL;
 
-	if (sparse_masks != ((ecx >> 3) & 1)) {
+	if ((get_vendor() == ARCH_INTEL) && sparse_masks != ((ecx >> 3) & 1)) {
 		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
 		return 1;
 	}
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 7946e32e85c8..353054724fa7 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -177,7 +177,6 @@ static bool mba_feature_check(const struct resctrl_test *test)
 struct resctrl_test mba_test = {
 	.name = "MBA",
 	.resource = "MB",
-	.vendor_specific = ARCH_INTEL,
 	.feature_check = mba_feature_check,
 	.run_test = mba_run_test,
 };
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index d67ffa3ec63a..f2223315b5b8 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -147,7 +147,6 @@ static bool mbm_feature_check(const struct resctrl_test *test)
 struct resctrl_test mbm_test = {
 	.name = "MBM",
 	.resource = "MB",
-	.vendor_specific = ARCH_INTEL,
 	.feature_check = mbm_feature_check,
 	.run_test = mbm_run_test,
 };
-- 
2.34.1

Re: [PATCH v2 4/4] selftests/resctrl: Enable MBA/MBA tests on AMD

[Ilpo Järvinen]

On Thu, 25 Apr 2024, Babu Moger wrote:

> Enable MBA/MBM tests if UMC (Unified Memory Controller) support is
> available on the system. Tests will be skipped otherwise.
> 
> Update noncont_cat_run_test to check for vendor. AMD supports
> non contiguous CBM masks but does not report it via CPUID.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  tools/testing/selftests/resctrl/cat_test.c | 2 +-
>  tools/testing/selftests/resctrl/mba_test.c | 1 -
>  tools/testing/selftests/resctrl/mbm_test.c | 1 -
>  3 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 4cb991be8e31..b682eaf65bfd 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -314,7 +314,7 @@ static int noncont_cat_run_test(const struct resctrl_test *test,
>  	else
>  		return -EINVAL;
>  
> -	if (sparse_masks != ((ecx >> 3) & 1)) {
> +	if ((get_vendor() == ARCH_INTEL) && sparse_masks != ((ecx >> 3) & 1)) {

This looks independent change to me which should be put into own patch.

-- 
 i.

>  		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
>  		return 1;
>  	}
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 7946e32e85c8..353054724fa7 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -177,7 +177,6 @@ static bool mba_feature_check(const struct resctrl_test *test)
>  struct resctrl_test mba_test = {
>  	.name = "MBA",
>  	.resource = "MB",
> -	.vendor_specific = ARCH_INTEL,
>  	.feature_check = mba_feature_check,
>  	.run_test = mba_run_test,
>  };
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index d67ffa3ec63a..f2223315b5b8 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -147,7 +147,6 @@ static bool mbm_feature_check(const struct resctrl_test *test)
>  struct resctrl_test mbm_test = {
>  	.name = "MBM",
>  	.resource = "MB",
> -	.vendor_specific = ARCH_INTEL,
>  	.feature_check = mbm_feature_check,
>  	.run_test = mbm_run_test,
>  };
> 

Re: [PATCH v2 4/4] selftests/resctrl: Enable MBA/MBA tests on AMD

[Moger, Babu]

Hi Ilpo,

On 4/26/2024 2:06 AM, Ilpo Järvinen wrote:
> On Thu, 25 Apr 2024, Babu Moger wrote:
>
>> Enable MBA/MBM tests if UMC (Unified Memory Controller) support is
>> available on the system. Tests will be skipped otherwise.
>>
>> Update noncont_cat_run_test to check for vendor. AMD supports
>> non contiguous CBM masks but does not report it via CPUID.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   tools/testing/selftests/resctrl/cat_test.c | 2 +-
>>   tools/testing/selftests/resctrl/mba_test.c | 1 -
>>   tools/testing/selftests/resctrl/mbm_test.c | 1 -
>>   3 files changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 4cb991be8e31..b682eaf65bfd 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -314,7 +314,7 @@ static int noncont_cat_run_test(const struct resctrl_test *test,
>>   	else
>>   		return -EINVAL;
>>   
>> -	if (sparse_masks != ((ecx >> 3) & 1)) {
>> +	if ((get_vendor() == ARCH_INTEL) && sparse_masks != ((ecx >> 3) & 1)) {
> This looks independent change to me which should be put into own patch.

Sure. Will do.

Thanks

Babu

Re: [PATCH v2 4/4] selftests/resctrl: Enable MBA/MBA tests on AMD

[Reinette Chatre]

On 4/26/2024 12:06 AM, Ilpo Järvinen wrote:
> On Thu, 25 Apr 2024, Babu Moger wrote:
> 
>> Enable MBA/MBM tests if UMC (Unified Memory Controller) support is
>> available on the system. Tests will be skipped otherwise.
>>
>> Update noncont_cat_run_test to check for vendor. AMD supports
>> non contiguous CBM masks but does not report it via CPUID.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  tools/testing/selftests/resctrl/cat_test.c | 2 +-
>>  tools/testing/selftests/resctrl/mba_test.c | 1 -
>>  tools/testing/selftests/resctrl/mbm_test.c | 1 -
>>  3 files changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 4cb991be8e31..b682eaf65bfd 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -314,7 +314,7 @@ static int noncont_cat_run_test(const struct resctrl_test *test,
>>  	else
>>  		return -EINVAL;
>>  
>> -	if (sparse_masks != ((ecx >> 3) & 1)) {
>> +	if ((get_vendor() == ARCH_INTEL) && sparse_masks != ((ecx >> 3) & 1)) {
> 
> This looks independent change to me which should be put into own patch.
> 

Own patch that is separate from this series. This should go in
as a fix with
Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test")

Reinette

Re: [PATCH v2 4/4] selftests/resctrl: Enable MBA/MBA tests on AMD

[Moger, Babu]

Hi Reinette,

On 5/9/24 16:11, Reinette Chatre wrote:
> 
> 
> On 4/26/2024 12:06 AM, Ilpo Järvinen wrote:
>> On Thu, 25 Apr 2024, Babu Moger wrote:
>>
>>> Enable MBA/MBM tests if UMC (Unified Memory Controller) support is
>>> available on the system. Tests will be skipped otherwise.
>>>
>>> Update noncont_cat_run_test to check for vendor. AMD supports
>>> non contiguous CBM masks but does not report it via CPUID.
>>>
>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>> ---
>>>  tools/testing/selftests/resctrl/cat_test.c | 2 +-
>>>  tools/testing/selftests/resctrl/mba_test.c | 1 -
>>>  tools/testing/selftests/resctrl/mbm_test.c | 1 -
>>>  3 files changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>>> index 4cb991be8e31..b682eaf65bfd 100644
>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>> @@ -314,7 +314,7 @@ static int noncont_cat_run_test(const struct resctrl_test *test,
>>>  	else
>>>  		return -EINVAL;
>>>  
>>> -	if (sparse_masks != ((ecx >> 3) & 1)) {
>>> +	if ((get_vendor() == ARCH_INTEL) && sparse_masks != ((ecx >> 3) & 1)) {
>>
>> This looks independent change to me which should be put into own patch.
>>
> 
> Own patch that is separate from this series. This should go in
> as a fix with
> Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test")
> 

Sure. Will do.

-- 
Thanks
Babu Moger

Re: [PATCH v2 0/4] selftests/resctrl: Enable MBM and MBA tests on AMD

[Reinette Chatre]

Hi Babu,

On 4/25/2024 1:16 PM, Babu Moger wrote:
> 
> The MBM (Memory Bandwidth Monitoring) and MBA (Memory Bandwidth Allocation)
> features are not enabled for AMD systems. The reason was lack of perf
> counters to compare the resctrl test results.
> 
> Starting with the commit
> 25e56847821f ("perf/x86/amd/uncore: Add memory controller support"), AMD
> now supports the UMC (Unified Memory Controller) perf events. These events
> can be used to compare the test results.
> 
> This series adds the support to detect the UMC events and enable MBM/MBA
> tests for AMD systems.
> 
> v2: Changes.
>     a. Rebased on top of tip/master (Apr 25, 2024)

Please note that resctrl selftest changes flow upstream via the kselftest
repo. The latest resctrl selftest changes can be found on the "next" branch
there.

Reinette

Re: [PATCH v2 0/4] selftests/resctrl: Enable MBM and MBA tests on AMD

[Moger, Babu]

Hi Reinette,

On 5/9/24 16:10, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/25/2024 1:16 PM, Babu Moger wrote:
>>
>> The MBM (Memory Bandwidth Monitoring) and MBA (Memory Bandwidth Allocation)
>> features are not enabled for AMD systems. The reason was lack of perf
>> counters to compare the resctrl test results.
>>
>> Starting with the commit
>> 25e56847821f ("perf/x86/amd/uncore: Add memory controller support"), AMD
>> now supports the UMC (Unified Memory Controller) perf events. These events
>> can be used to compare the test results.
>>
>> This series adds the support to detect the UMC events and enable MBM/MBA
>> tests for AMD systems.
>>
>> v2: Changes.
>>     a. Rebased on top of tip/master (Apr 25, 2024)
> 
> Please note that resctrl selftest changes flow upstream via the kselftest
> repo. The latest resctrl selftest changes can be found on the "next" branch
> there.

Yes. Got it.
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/
-- 
Thanks
Babu Moger