Hi Maciej,
On 7/3/24 12:43 AM, Maciej Wieczór-Retman wrote:
On 3.07.2024 00:21, Reinette Chatre wrote:
On 7/1/24 7:18 AM, Maciej Wieczor-Retman wrote:
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.
The "SNC detection was unreliable" only gets printed on the first execution of run_single_test().
There is more about this later, but this can be printed at start of each test.
That's what the global snc_mode was for mostly, it starts initialized to 0, and then on the first
run of run_single_test() it is set so other tests don't call get_snc_mode(). And then the local static
variable inside get_snc_mode() prevents the detection from running more than once when other places
call get_snc_mode() (like when the cache size is adjusted).
The shadowing of variables can get confusing. I think the global snc_mode is not necessary, having the
local static variable within snc_nodes_per_l3_cache() should be sufficient and run_single_test()
can just do a:
int snc_mode; /* new name welcome */
snc_mode = snc_nodes_per_l3_cache();
if (snc_mode > 1)
ksft_print_msg("SNC-%d mode discovered\n", snc_mode);
else if (snc_unreliable)
ksft_print_msg("SNC detection unreliable due to offline CPUs. Test results may not be accurate if SNC enabled.\n");
And as we discussed last time it's beneficial to put error messages at the end of the test in case the
user misses the initial warning at the very beginning.
Right. What I found unexpected was that it is done "at the end" but from two places, from the show*info()
as well as from run*test(). I expect "the end" to be a single place.
}
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(¶m, 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.
I was going for this one error message to specifically catch the kernel
not having snc support for resctrl while snc is enabled. While the
above message could be true when snc_unreliable == 1, it doesn't have to.
If a test fails when snc_unreliable == 1 then nothing is certain and some generic message
is needed.
SNC might not be enabled at all so there would be no reason to send the user
to check their BIOS - instead they can learn they have offline CPUs and they can
work on fixing that. In my opinion it could be beneficial to have more specialized
messages in the selftests to help users diagnose problems quicker.
My goal is indeed to have specialized messages. There cannot be a specialized message
if snc_reliable == 1. In this case it needs to be generic since SNC may or may not be
enabled and it is up to the user to investigate further.
Having only this one message wihtout the "if snc unreliable" messages would
mean nothing would get printed at the end on success with unreliable SNC detection.
Having a pass/fail is what user will focus on. If the test passes then SNC detection
should not matter. The messages are just there to help user root cause where a failure
may be.
...
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)
I wanted to print the message below only on the first call to run_single_test() and then
print relevant warnings at the very end of each test. I thought that was your intention
when we discussed what messages are supposed to be printed and when in v2 of this series.
Do you think it would be better to just print this message at the start of each test?
Yes. If there is a problem with a test the user could be expected to start tracing back
messages printed from beginning of failing test.
Or should I make "snc_mode" into local static inside run_single_test()? Or maybe add
a second local static variable into get_snc_mode() that would control whether or not
the message should be printed?
I do not see where more local static variables may be needed.
Reinette
