[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <91eeb2d6-e0bc-43cd-8641-ddede63f3786@intel.com>
Date: Thu, 9 May 2024 14:11:44 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <fenghua.yu@...el.com>,
<shuah@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
<ilpo.jarvinen@...ux.intel.com>, <maciej.wieczor-retman@...el.com>,
<peternewman@...gle.com>, <eranian@...gle.com>
Subject: Re: [PATCH v2 3/4] selftests/resctrl: Add support for MBM and MBA
tests on AMD
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@....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
Powered by blists - more mailing lists