Date: Thu, 27 Jun 2024 11:50:49 +0200
From: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: <fenghua.yu@...el.com>, <shuah@...nel.org>,
	<linux-kselftest@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<ilpo.jarvinen@...ux.intel.com>, <tony.luck@...el.com>
Subject: Re: [PATCH v2 1/2] selftests/resctrl: Adjust effective L3 cache size
 with SNC enabled

On 2024-06-26 at 09:46:01 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 6/26/24 12:09 AM, Maciej Wieczor-Retman wrote:
>> Hello!,
>> 
>> On 2024-06-25 at 09:28:55 -0700, Reinette Chatre wrote:
>> > Hi Maciej,
>> > 
>> > On 6/25/24 4:04 AM, Maciej Wieczor-Retman wrote:
>> > > Hello,
>> > > sorry it took me so long to get back to this. I prepared the next version with
>> > > your comments applied and Tony's replies taken into account.
>> > 
>> > Thank you very much for sticking with this.
>> > 
>> > > 
>> > > I wanted to briefly discuss this before posting:
>> > > 
>> > > On 2024-05-30 at 16:07:29 -0700, Reinette Chatre wrote:
>> > > > On 5/15/24 4:18 AM, Maciej Wieczor-Retman wrote:
>> > > > > +		return 1;
>> > > > > +	}
>> > > > > +
>> > > > > +	for (i = 1; i <= MAX_SNC ; i++) {
>> > > > > +		if (i * node_cpus >= cache_cpus)
>> > > > > +			return i;
>> > > > > +	}
>> > > > 
>> > > > This is not obvious to me. From the function comments this seems to address the
>> > > > scenarios when CPUs from other nodes are offline. It is not clear to me how
>> > > > this loop addresses this. For example, let's say there are four SNC nodes
>> > > > associated with a cache and only the node0 CPUs are online. The above would
>> > > > detect this as "1", not "4", if I read this right?
>> > > > 
>> > > > I wonder if it may not be easier to just follow what the kernel does
>> > > > (in the new version).
>> > > > User space can learn the number of online and present CPUs from
>> > > > /sys/devices/system/cpu/online and /sys/devices/system/cpu/present
>> > > > respectively. A simple string compare of the contents can be used to
>> > > > determine if they are identical and a warning can be printed if they are not.
>> > > > With a warning when accurate detection cannot be done the simple
>> > > > check will do.
>> > > > 
>> > > > Could you please add an informational message indicating how many SNC nodes
>> > > > were indeed detected?
>> > > 
>> > > Should the information "how many SNC nodes are detected?" get printed every time
>> > > (by which I mean at the end of CMT and MBM tests) or only when we get the error
>> > > "SNC enabled but kernel doesn't support it" happens? Of course in the first case
>> > > if there is only 1 node detected nothing would be printed to avoid noise.
>> > 
>> > I agree that it is not needed to print something about SNC if it is disabled.
>> > hmmm ... so SNC impacts every test but it is only detected by default during CAT
>> > and CMT test, with MBA and MBM "detection" only triggered if the test fails?
>> 
>> Yes, snc_ways() ran before starting CAT and CMT to adjust cache size variable.
>> And then after CAT,CMT,MBM and MBA if the return value indicated failure.
>> 
>> > 
>> > What if the "SNC detection" is moved to be within run_single_test() but instead of
>> > repeating the detection from scratch every time it rather works like get_vendor()
>> > where the full detection is only done on first attempt? run_single_test() can detect if
>> > SNC is enabled and (if number of SNC nodes > 1) print an informational message
>> > that is inherited by all tests.
>> > Any test that needs to know the number of SNC nodes can continue to use the
>> > same function used for detection (that only does actual detection once).
>> > 
>> > What do you think?
>> 
>> I think running the detection once at the start and then reusing the results is
>> a good idea. You're proposing adding a value (global or passed through all the
>> tests) that would get initialized on the first run_single_test()?
>
>I was thinking about a solution similar to get_vendor() that uses a static local
>variable. A global variable could work also.

Oh yes, static local is good too, I'll try that.

>
>> And then the SNC status (if enabled) + a warning if the detection could be wrong
>> (because of the online/present cpus ratio) would happen before the test runs?
>
>I do not think this was part of previous tests, but yes, this is a concern where
>a warning would be helpful.
>
>> On the warning placement I think it should be moved out of being printed only on
>> failure. I did some experiments using "chcpu" to enable/disable cores and then
>> run selftests. They didn't have any problems succeeding even though SNC
>> detection detected different mode every time (I added a printf() around the line
>
>I am not surprised here since there has not been much tuning of the CAT test.
>
>> that cache size is modified to show what SNC mode is detected). While I
>> understand these tests shouldn't fail since they just use a different portion of
>> the cache I think the user should be informed it's not really NUMA aware if the
>> detection was wrong:
>
>Seems like there are two warnings to consider:
>(a) SNC detection may be wrong.
>(b) If SNC is enabled and kernel does not support SNC then the tests may fail.
>
>For (a) I think that it is possible to know when SNC detection may be wrong. A test
>similar to the kernel test that compares the "online" and "present" CPUs [1] can
>be used. The /sys/devices/system/cpu/online and /sys/devices/system/cpu/present
>files are available for this. A simpler way may be to just print a warning if
>/sys/devices/system/cpu/offline is not empty and set the number of SNC nodes
>to 1. Instead, a new "snc_unreliable" global can be set that can be used
>to print additional information during test failure.

The empty offline file is a cool idea, less files to open :)

>
>I do think that it is fair to print all the SNC details during detection but
>I am concerned that those messages will be out of sight during test failures
>and I thus do think it is useful to have extra information during test
>failure.

Yeah, I've been thinking about what is the best way to display these for a
while. Maybe you're right that messages at the top will be lost. What about this
set of messages:

1. First run of run_single_test()
	1.1. For all tests:
		- detected snc mode (if > 1)
			- check if cpu/offline file is empty, set the global
			  variable and print a message saying snc mode might be
			  wrong

2. At the end of tests
	2.1. For CMT, CAT, MBM, MBA:
		- test failed
		- snc detection reports it's enabled
		- kernel version doesn't support snc

	2.2. Additional message for CMT, CAT (since the cache size is divided):
		- test failed or succeeded
		- snc detection reports the offline file is not empty
		- kernel version supports snc

The 1. message will be printed at the top since it's more informational (what is
the SNC mode?) and then 2. messages will deal with possible issues / failures
and will be nicely visible at the end. What do you think about this?

>
>Reinette
>
>[1] https://lore.kernel.org/lkml/20240621223859.43471-18-tony.luck@intel.com/

-- 
Kind regards
Maciej Wieczór-Retman

Powered by blists - more mailing lists