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 <[email protected]> >> 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
