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
