Re: [PATCH v3 2/4] selftests/resctrl: Pass sysfs controller name of the vendor

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 6/14/24 13:38, Reinette Chatre wrote:
> Hi Babu,
> 
> Subject and changelog mentions how controller name is "passed" but the
> patch does not seem to "pass" anything new.

Sure. Will change the subject to this. Also will update the commit message.

selftests/resctrl: Dynamically detect sysfs controller name of the vendor

> 
> On 6/5/24 3:45 PM, Babu Moger wrote:
>> The test detects number of memory controllers by looking at the sysfs
>> file system. Detect the vendor to pass the controller name appropriately.
>> So, that rest of the code will be generic and can be used to support other
>> vendors.
>>
>> Also change the search to look for the full string "uncore_imc_". Replace
>> the sizeof with strlen.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v3: Change the search string to "uncore_imc_".
>>
>> v2: No changes
>> ---
>>   tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++++-------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
>> b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 2d5e961b3a68..23c0e0a1d845 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -10,7 +10,7 @@
>>    */
>>   #include "resctrl.h"
>>   -#define UNCORE_IMC        "uncore_imc"
>> +#define UNCORE_IMC        "uncore_imc_"
>>   #define READ_FILE_NAME        "events/cas_count_read"
>>   #define WRITE_FILE_NAME        "events/cas_count_write"
>>   #define DYN_PMU_PATH        "/sys/bus/event_source/devices"
>> @@ -206,24 +206,30 @@ static int num_of_mem_controllers(void)
>>       char mc_dir[512], *temp;
>>       unsigned int count = 0;
>>       struct dirent *ep;
>> -    int ret;
>> +    char *sysfs_name;
>> +    int ret, vendor;
>>       DIR *dp;
>>   +    vendor = get_vendor();
> 
> get_vendor() is already optimized to only need to do actual detection once.
> I thus do not see a need for a local variable.

Sure.
> 
>> +    if (vendor == ARCH_INTEL) {
>> +        sysfs_name = UNCORE_IMC;
>> +    } else {
>> +        ksft_perror("Unsupported vendor!\n");
> 
> ksft_perror() also adds a "\n" so adding it here is not necessary. Also
> please drop the exclamation.

Sure.

> 
>> +        return -1;
>> +    }
>> +
>>       dp = opendir(DYN_PMU_PATH);
>>       if (dp) {
>>           while ((ep = readdir(dp))) {
>> -            temp = strstr(ep->d_name, UNCORE_IMC);
>> +            temp = strstr(ep->d_name, sysfs_name);
>>               if (!temp)
>>                   continue;
>>                 /*
>>                * imc counters are named as "uncore_imc_<n>", hence
>> -             * increment the pointer to point to <n>. Note that
>> -             * sizeof(UNCORE_IMC) would count for null character as
>> -             * well and hence the last underscore character in
>> -             * uncore_imc'_' need not be counted.
>> +             * increment the pointer to point to <n>.
>>                */
>> -            temp = temp + sizeof(UNCORE_IMC);
>> +            temp = temp + strlen(sysfs_name);
>>                 /*
>>                * Some directories under "DYN_PMU_PATH" could have
> 
> Reinette
> 

-- 
Thanks
Babu Moger