Re: [PATCH v2 1/3] selftests/resctrl: Add CPU vendor detection for Hygon

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 12/9/25 3:42 PM, Luck, Tony wrote:
> On Tue, Dec 09, 2025 at 03:02:14PM -0800, Reinette Chatre wrote:
> 
>> I suggest this be simplified to not have the vendor ID be used both as a value and as a state.
>> Here is some pseudo-code that should be able to accomplish this:
>>
>>
>> 	unsigned int detect_vendor(void)
>> 	{
>> 		static bool initialized = false;
>> 		static unsigned int vendor_id;
>> 		...
>> 		FILE *inf;
>>
>>
>> 		if (initialized)
>> 			return vendor_id;
>>
>> 		inf = fopen("/proc/cpuinfo", "r");
>> 		if (!inf) {
>> 			vendor_id = 0;
>> 			initialized = true;
>> 			return vendor_id;
>> 		}
>>
>> 		/* initialize vendor_id from /proc/cpuinfo */
>>
>> 		initialized = true;
>> 		return vendor_id;
>> 	}
>>
>> 	unsigned int get_vendor(void)
>> 	{
>> 		unsigned int vendor;
>> 		
>> 		vendor = detect_vendor();
>>
>> 		if (vendor == 0)
>> 			ksft_print_msg(...);
>>
>> 		return vendor;
>> 	}
> 
> If detect_vendor() failed, this you'd get the ksft_print_msg() for every
> call to get_vendor().

Right. This is intended to match existing behavior. The goal is to
only do the work of querying the vendor information once. The tests are
independent so to avoid the failure message about obtaining vendor information
to only be in the test that did the original query it is printed in every
test's output.

> 
> Why not split completly.
> 
> static unsigned int vendor_id;
> 
> void detect_vendor(void)
> {
> 	FILE *inf = fopen("/proc/cpuinfo", "r");
> 
> 	if (!inf) {
> 		... warning unable to get vendor id ...
> 	}
> 
> 	... initialize from /proc/cpuinfo ...
> 
> 	... warn if doesn't find a known vendor ...
> }
> 
> Call detect_vendor() at the beginning of main() in each test.

This will repeat the vendor detection for every (currently six) test?
This seems unnecessary work to me considering this only needs to be done
once.

> 
> Then just use "vendor_id" whenever you need to test for some vendor
> specific feature.

Including the warning within detect_vendor() and calling it in each test
does address the goal of including any vendor detection failure message in log of
every test that depends on it. Even so, from what I can tell this separates the message
from where test actually fails though, making things harder to debug, and I expect will
result in more code duplication: the duplicated calls to detect_vendor() and likely
tests needing to duplicate current get_vendor() (more below):

If I understand correctly this would look something like:

	some_function()
	{
		if (vendor_id == ARCH_TBD)
			/* Do something */
	}


	resctrl_test::run_test(...)
	{
		/* prep */
		detect_vendor(); /* may print warning */
		/*
		 * Do not fail the test if vendor detection fails since not all
		 * flows may depend on vendor information.
		 */
		
		/* run actual test */
		/* do some things that result in messages in test log */
		some_function();
		/* do some things that result in messages in test log */
	}
	
In above the test log will show early that vendor detection failed but it is not
clear in the test log what the impact on the test is by being clear where in the
test the failed vendor detection is needed/used.

I anticipate that tests will start to be defensive and do things like below that
would create unnecessary duplication that is currently handled by get_vendor().
	some_function()
	{
		if (vendor_id == 0) {
			ksft_print_msg("Vendor invalid, cannot do <something>\n");
			return;
		}

		if (vendor_id == ARCH_TBD)
			/* Do something */
	}

Just failing tests if the vendor cannot be determined may be an easy solution but
since not all tests depend on vendor information this seems too severe.

Reinette