Received-SPF: pass (google.com: domain of linux-kernel+bounces-78180-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 23 Feb 2024 12:53:58 +0200 (EET)
To: Babu Moger <babu.moger@amd.com>
cc: fenghua.yu@intel.com, Reinette Chatre <reinette.chatre@intel.com>, 
    shuah@kernel.org, LKML <linux-kernel@vger.kernel.org>, 
    linux-kselftest@vger.kernel.org, 
    =?ISO-8859-15?Q?Maciej_Wiecz=F3r-Retman?= <maciej.wieczor-retman@intel.com>, 
    peternewman@google.com, eranian@google.com
Subject: Re: [PATCH 3/4] selftests/resctrl: Add support for MBM and MBA tests
 on AMD
In-Reply-To: <cc22790173de9978a6485baca5bc96a8ad8e05a4.1708637563.git.babu.moger@amd.com>
Message-ID: <6f611a0a-f2ef-1017-07c0-5d805abb80d4@linux.intel.com>
References: <cover.1708637563.git.babu.moger@amd.com> <cc22790173de9978a6485baca5bc96a8ad8e05a4.1708637563.git.babu.moger@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

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.