[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fc551b62-6afa-40f8-b030-9e43253881db@intel.com>
Date: Fri, 14 Jun 2024 11:38:18 -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 v3 2/4] selftests/resctrl: Pass sysfs controller name of
the vendor
Hi Babu,
Subject and changelog mentions how controller name is "passed" but the
patch does not seem to "pass" anything new.
On 6/5/24 3:45 PM, Babu Moger wrote:
> The test detects number of memory controllers by looking at the sysfs
> file system. Detect the vendor to pass the controller name appropriately.
> So, that rest of the code will be generic and can be used to support other
> vendors.
>
> Also change the search to look for the full string "uncore_imc_". Replace
> the sizeof with strlen.
>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> v3: Change the search string to "uncore_imc_".
>
> v2: No changes
> ---
> tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++++-------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 2d5e961b3a68..23c0e0a1d845 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -10,7 +10,7 @@
> */
> #include "resctrl.h"
>
> -#define UNCORE_IMC "uncore_imc"
> +#define UNCORE_IMC "uncore_imc_"
> #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"
> @@ -206,24 +206,30 @@ static int num_of_mem_controllers(void)
> char mc_dir[512], *temp;
> unsigned int count = 0;
> struct dirent *ep;
> - int ret;
> + char *sysfs_name;
> + int ret, vendor;
> DIR *dp;
>
> + vendor = get_vendor();
get_vendor() is already optimized to only need to do actual detection once.
I thus do not see a need for a local variable.
> + if (vendor == ARCH_INTEL) {
> + sysfs_name = UNCORE_IMC;
> + } else {
> + ksft_perror("Unsupported vendor!\n");
ksft_perror() also adds a "\n" so adding it here is not necessary. Also
please drop the exclamation.
> + 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;
>
> /*
> * 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.
> + * increment the pointer to point to <n>.
> */
> - temp = temp + sizeof(UNCORE_IMC);
> + temp = temp + strlen(sysfs_name);
>
> /*
> * Some directories under "DYN_PMU_PATH" could have
Reinette
Powered by blists - more mailing lists