Date: Tue, 2 Jul 2024 15:21:49 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
	<shuah@...nel.org>, <fenghua.yu@...el.com>
CC: <linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
	<ilpo.jarvinen@...ux.intel.com>, <tony.luck@...el.com>
Subject: Re: [PATCH v3 2/2] selftests/resctrl: Adjust SNC support messages

Hi Maciej,

On 7/1/24 7:18 AM, Maciej Wieczor-Retman wrote:
> Resctrl selftest prints a message on test failure that Sub-Numa
> Clustering (SNC) could be enabled and points the user to check theirs BIOS

theirs BIOS -> their BIOS?

> settings. No actual check is performed before printing that message so
> it is not very accurate in pinpointing a problem.
> 
> Figuring out if SNC is enabled is only one part of the problem, the
> others being whether the detected SNC mode is reliable and whether the
> kernel supports SNC in resctrl.
> 
> When there is SNC support for kernel's resctrl subsystem and SNC is
> enabled then sub node files are created for each node in the resctrlfs.
> The sub node files exist in each regular node's L3 monitoring directory.
> The reliable path to check for existence of sub node files is
> /sys/fs/resctrl/mon_data/mon_L3_00/mon_sub_L3_00.
> 
> To check if SNC detection is reliable one can check the
> /sys/devices/system/cpu/offline file. If it's empty, it means all cores
> are operational and the ratio should be calculated correctly. If it has
> any contents, it means the detected SNC mode can't be trusted and should
> be disabled.
> 
> Add helpers for detecting SNC mode and checking its reliability.
> 
> Detect SNC mode once and let other tests inherit that information.
> 
> Add messages to alert the user when SNC detection could return incorrect
> results.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
> ---
> Changelog v3:
> - Change snc_ways() to snc_nodes_per_l3_cache(). (Reinette)
> - Add printing the discovered SNC mode. (Reinette)
> - Change method of kernel support discovery from cache sizes to
>    existance of sub node files.
> - Check if SNC detection is unreliable.
> - Move SNC detection to only the first run_single_test() instead on
>    error at the end of test runs.
> - Add global value to remind user at the end of relevant tests if SNC
>    detection was found to be unreliable.
> - Redo the patch message after the changes.
> 
> Changelog v2:
> - Move snc_ways() checks from individual tests into
>    snc_kernel_support().
> - Write better comment for snc_kernel_support().
> 
>   tools/testing/selftests/resctrl/cache.c       |  3 +
>   tools/testing/selftests/resctrl/cmt_test.c    |  4 +-
>   tools/testing/selftests/resctrl/mba_test.c    |  4 ++
>   tools/testing/selftests/resctrl/mbm_test.c    |  6 +-
>   tools/testing/selftests/resctrl/resctrl.h     |  4 ++
>   .../testing/selftests/resctrl/resctrl_tests.c |  7 ++
>   tools/testing/selftests/resctrl/resctrlfs.c   | 70 ++++++++++++++++++-
>   7 files changed, 93 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 1ff1104e6575..9885d64b8a21 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -186,4 +186,7 @@ void show_cache_info(int no_of_bits, __u64 avg_llc_val, size_t cache_span, bool
>   	ksft_print_msg("Average LLC val: %llu\n", avg_llc_val);
>   	ksft_print_msg("Cache span (%s): %zu\n", lines ? "lines" : "bytes",
>   		       cache_span);
> +	if (snc_unreliable)
> +		ksft_print_msg("SNC detection unreliable due to offline CPUs!\n");

The message abour SNC detection being unreliable is already printed at beginning of every
test so I do not think it is necessary to print it again at this point.

> +
>   }
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 0c045080d808..588543ae2654 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -175,8 +175,8 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
>   		goto out;
>   
>   	ret = check_results(&param, span, n);
> -	if (ret && (get_vendor() == ARCH_INTEL))
> -		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");

This message does seem to still be applicable if snc_unreliable == 1.

> +	if (ret && (get_vendor() == ARCH_INTEL) && !snc_kernel_support())
> +		ksft_print_msg("Kernel doesn't support Sub-NUMA Clustering but it is enabled. Check BIOS configuration.\n");
>   
>   out:
>   	free(span_str);
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index ab8496a4925b..c91e85f11285 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -108,6 +108,8 @@ static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
>   		ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
>   		ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
>   		ksft_print_msg("avg_bw_resc: %lu\n", avg_bw_resc);
> +		if (snc_unreliable)
> +			ksft_print_msg("SNC detection unreliable due to offline CPUs!\n");

(here I also think this is unnecessary)

>   		if (avg_diff_per > MAX_DIFF_PERCENT)
>   			ret = true;
>   	}
> @@ -179,6 +181,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
>   		return ret;
>   
>   	ret = check_results();
> +	if (ret && (get_vendor() == ARCH_INTEL) && !snc_kernel_support())
> +		ksft_print_msg("Kernel doesn't support Sub-NUMA Clustering but it is enabled. Check BIOS configuration.\n");
>   
>   	return ret;
>   }
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 6b5a3b52d861..562b02118270 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -43,6 +43,8 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>   	ksft_print_msg("Span (MB): %zu\n", span / MB);
>   	ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
>   	ksft_print_msg("avg_bw_resc: %lu\n", avg_bw_resc);
> +	if (snc_unreliable)
> +		ksft_print_msg("SNC detection unreliable due to offline CPUs!\n");
>   
>   	return ret;
>   }
> @@ -147,8 +149,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
>   		return ret;
>   
>   	ret = check_results(DEFAULT_SPAN);
> -	if (ret && (get_vendor() == ARCH_INTEL))
> -		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");

This message does seem to still be applicable if snc_unreliable == 1.

> +	if (ret && (get_vendor() == ARCH_INTEL) && !snc_kernel_support())
> +		ksft_print_msg("Kernel doesn't support Sub-NUMA Clustering but it is enabled. Check BIOS configuration.\n");
>   
>   	return ret;
>   }
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 851b37c9c38a..fa44e1cde21b 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -121,10 +121,13 @@ struct perf_event_read {
>    */
>   extern volatile int *value_sink;
>   
> +extern int snc_unreliable;
> +
>   extern char llc_occup_path[1024];
>   
>   int snc_nodes_per_l3_cache(void);
>   int get_vendor(void);
> +int get_snc_mode(void);
>   bool check_resctrlfs_support(void);
>   int filter_dmesg(void);
>   int get_domain_id(const char *resource, int cpu_no, int *domain_id);
> @@ -167,6 +170,7 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>   int signal_handler_register(const struct resctrl_test *test);
>   void signal_handler_unregister(void);
>   unsigned int count_bits(unsigned long n);
> +int snc_kernel_support(void);
>   
>   void perf_event_attr_initialize(struct perf_event_attr *pea, __u64 config);
>   void perf_event_initialize_read_format(struct perf_event_read *pe_read);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index ecbb7605a981..b17560bbaf5c 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -12,6 +12,7 @@
>   
>   /* Volatile memory sink to prevent compiler optimizations */
>   static volatile int sink_target;
> +static int snc_mode;

This global seems unnecessary (more later) and also potentially confusing since
the get_snc_mode() has a function local static variable of same name.

>   volatile int *value_sink = &sink_target;
>   
>   static struct resctrl_test *resctrl_tests[] = {
> @@ -123,6 +124,12 @@ static void run_single_test(const struct resctrl_test *test, const struct user_p
>   	if (test->disabled)
>   		return;
>   
> +	if (!snc_mode) {
> +		snc_mode = get_snc_mode();
> +		if (snc_mode > 1)


 From what I can tell this is the only place the global is used and this can just be:
		if (get_snc_mode() > 1)

> +			ksft_print_msg("SNC-%d mode discovered!\n", snc_mode);
> +	}
> +
>   	if (!test_vendor_specific_check(test)) {
>   		ksft_test_result_skip("Hardware does not support %s\n", test->name);
>   		return;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 18a31a2ba7b3..004fb6649789 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -13,6 +13,8 @@
>   
>   #include "resctrl.h"
>   
> +int snc_unreliable;
> +
>   static int find_resctrl_mount(char *buffer)
>   {
>   	FILE *mounts;
> @@ -280,7 +282,7 @@ int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size
>   	 * with a fully populated L3 mask in the schemata file.
>   	 */
>   	if (cache_num == 3)
> -		*cache_size /= snc_nodes_per_l3_cache();
> +		*cache_size /= get_snc_mode();

hmmm ... it is not ideal to use second patch to change something from first patch.
Just having a single snc_nodes_per_l3_cache() will eliminate this change (more below).

>   	return 0;
>   }
>   
> @@ -939,3 +941,69 @@ unsigned int count_bits(unsigned long n)
>   
>   	return count;
>   }
> +
> +static bool cpus_offline_empty(void)
> +{
> +	char offline_cpus_str[64];
> +	FILE *fp;
> +
> +	fp = fopen("/sys/devices/system/cpu/offline", "r");
> +	if (fscanf(fp, "%s", offline_cpus_str) < 0) {
> +		if (!errno) {
> +			fclose(fp);
> +			return 1;
> +		}
> +		ksft_perror("Could not read offline CPUs file!");
> +	}
> +
> +	fclose(fp);
> +
> +	return 0;
> +}
> +
> +int get_snc_mode(void)
> +{
> +	static int snc_mode;
> +
> +	if (!snc_mode) {
> +		snc_mode = snc_nodes_per_l3_cache();
> +		if (!cpus_offline_empty()) {
> +			ksft_print_msg("Runtime SNC detection unreliable due to offline CPUs!\n");
> +			ksft_print_msg("Setting SNC mode to disabled.\n");
> +			snc_mode = 1;
> +			snc_unreliable = 1;
> +		}
> +	}
> +
> +	return snc_mode;
> +}

I think the SNC detection will be easier to understand if it is done in a single
place. Can the static local variable and checks using the offline file instead be included in
existing snc_nodes_per_l3_cache()?

> +
> +/**
> + * snc_kernel_support - Compare system reported cache size and resctrl
> + * reported cache size to get an idea if SNC is supported on the kernel side.

This comment does not seem to match what the function does.

> + *
> + * Return: 0 if not supported, 1 if SNC is disabled or SNC is both enabled and
> + * supported, < 0 on failure.
> + */
> +int snc_kernel_support(void)
> +{
> +	char node_path[PATH_MAX];
> +	struct stat statbuf;
> +	int ret;
> +
> +	ret = get_snc_mode();
> +	/*
> +	 * If SNC is disabled then its kernel support isn't important. If value
> +	 * is smaller than 1 an error happened.

How can a value smaller than 1 be returned?

> +	 */
> +	if (ret <= 1)
> +		return ret;
> +
> +	snprintf(node_path, sizeof(node_path), "%s/%s/%s", RESCTRL_PATH, "mon_data",
> +		 "mon_L3_00/mon_sub_L3_00");
> +
> +	if (!stat(node_path, &statbuf))
> +		return 1;
> +
> +	return 0;
> +}


Reinette

Powered by blists - more mailing lists