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

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