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

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

Hi Xiaochen,

On 12/8/25 10:10 PM, Xiaochen Shen wrote:
> On 12/9/2025 1:57 AM, Reinette Chatre wrote:

...

 
>>>   In file resctrl.h:
>>>     -----------------
>>>       /*
>>>        * CPU vendor IDs
>>>        *
>>>        * Define as bits because they're used for vendor_specific bitmask in
>>>        * the struct resctrl_test.
>>>        */
>>>       #define ARCH_INTEL     1
>>>       #define ARCH_AMD       2
>>>     -----------------
>>>
>>>     The comment before the CPU vendor IDs defines attempts to provide
>>>     guidance but it is clearly still quite subtle that these values are
>> I wrote "clearly" in response to the earlier  patch that did not follow the quoted
>> documentation, implying that the documentation was not sufficient. I do not
>> think "clearly" applies here. This can just be specific about how these values
>> are used ... which this paragraph duplicates from the quoted comment so either this
>> paragraph or the code quote could be dropped?
> 
> Thank you for the suggestion.
> The revised patch description as below:
> --------------------------------------
>     The CPU vendor IDs are required to be unique bits because they're used
>     for vendor_specific bitmask in the struct resctrl_test.
>     Consider for example their usage in test_vendor_specific_check():
>             return get_vendor() & test->vendor_specific
> 
>     However, the definitions of CPU vendor IDs in file resctrl.h is quite
>     subtle as a bitmask value:
>       #define ARCH_INTEL     1
>       #define ARCH_AMD       2
> 
>     A clearer and more maintainable approach is to define these CPU vendor
>     IDs using BIT(). This ensures each vendor corresponds to a distinct bit
>     and makes it obvious when adding new vendor IDs.

Thank you. Looks good to me.

>     ...
> --------------------------------------
> 
>>
>>>     required to be unique bits. Consider for example their usage in
>>>     test_vendor_specific_check():
>>>             return get_vendor() & test->vendor_specific
>>> -int get_vendor(void)
>>> +unsigned int get_vendor(void)
>>>  {
>>> -       static int vendor = -1;
>>> +       static unsigned int vendor;
>>>
>>> -       if (vendor == -1)
>>> +       if (vendor == 0)
>>>                 vendor = detect_vendor();
>>> +
>>> +       /* detect_vendor() returns invalid vendor id */
>>>         if (vendor == 0)
>>>                 ksft_print_msg("Can not get vendor info...\n");
>> detect_vendor() returns 0 if it cannot detect the vendor. Using "0" as well as
>> return value of detect_vendor() to indicate that detect_vendor() should be run will
>> thus cause detect_vendor() to always be called on failure even though it will keep
>> failing.
> 
> Thank you.
> I got it. In original code, "static int vendor = -1;" does it intentionally.
> 
> 
>>
>> Can vendor be kept as int and just cast it on return? This may be introducing the
>> risky type conversion that the changelog claims to avoid though .... 
> 
> This is really a dilemma.
> I could keep vendor as int, even thought the code doesn't look graceful. I will try to add a comment for it.
> The code changes may look like:
> -------------------------------
> -int get_vendor(void)
> +unsigned int get_vendor(void)
>  {
>         static int vendor = -1;
> 
> +       /*
> +        * Notes on vendor:
> +        *  -1: initial value, detect_vendor() is not called yet.
> +        *   0: detect_vendor() returns 0 if it cannot detect the vendor.
> +        * > 0: detect_vendor() returns valid vendor id.
> +        *
> +        * The return type of detect_vendor() is 'unsigned int'.
> +        * Cast vendor from 'int' to 'unsigned int' on return.
> +        */
>         if (vendor == -1)
>                 vendor = detect_vendor();
> +
>         if (vendor == 0)
>                 ksft_print_msg("Can not get vendor info...\n");
> 
> -       return vendor;
> +       return (unsigned int) vendor;
>  }

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;
	}

Reinette