Message-ID: <1ce7ea26-6e97-4640-86df-c8dd3e623002@open-hieco.net>
Date: Tue, 9 Dec 2025 14:10:02 +0800
From: Xiaochen Shen <shenxiaochen@...n-hieco.net>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Fenghua Yu <fenghuay@...dia.com>, tony.luck@...el.com, bp@...en8.de,
 shuah@...nel.org, skhan@...uxfoundation.org
Cc: babu.moger@....com, james.morse@....com, Dave.Martin@....com,
 x86@...nel.org, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, shenxiaochen@...n-hieco.net
Subject: Re: [PATCH v2 1/3] selftests/resctrl: Add CPU vendor detection for
 Hygon

Hi Reinette,

On 12/9/2025 1:57 AM, Reinette Chatre wrote:
>> Thank you for the suggestion. How about using BIT_U8() instead of BIT()?
>> In my opinion, 8-bits type "unsigned int" is enough for "vendor id".
> BIT() is fine here. I prefer that types used by selftests are consistent, that is, not
> a mix of user space and kernel types. 
> There may be good motivation to switch to kernel types but then it needs to be
> throughout the resctrl selftests, which is not something this work needs to take on.

Thank you. I will keep BIT() here.


>> Should I split the code changes (using BIT_xx(), updates of type 'unsigned int') into a separate patch?	
> I agree this would be better as a separate patch.

Sure. I will add a prerequisite patch in this series.


>> The patch may look like:
>> -----------------------------
>> commit baaabb7bd3a3e45a8093422b576383da20488aca
>> Author: Xiaochen Shen <shenxiaochen@...n-hieco.net>
>> Date:   Mon Dec 8 14:26:45 2025 +0800
>>
>>     selftests/resctrl: Improve type definitions of CPU vendor IDs
> Instead of a generic "Improve" it can just be specific about what it does:
> "selftests/resctrl: Define CPU vendor IDs as bits to match usage"

Thank you for the suggestion. The subject of the patch looks much better.


>>   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.
    ...
--------------------------------------

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

Thank you!

Best regards,
Xiaochen Shen

Powered by blists - more mailing lists