lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e2755213-0ea1-4b77-886c-294068940bd1@amd.com>
Date: Thu, 20 Jun 2024 11:18:40 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, fenghua.yu@...el.com,
 shuah@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 ilpo.jarvinen@...ux.intel.com, maciej.wieczor-retman@...el.com,
 peternewman@...gle.com, eranian@...gle.com
Subject: Re: [PATCH v3 3/4] selftests/resctrl: Add support for MBM and MBA
 tests on AMD

Hi Reinette,

On 6/14/24 13:39, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/5/24 3:45 PM, Babu Moger wrote:
>> Add support to read UMC (Unified Memory Controller) perf events to compare
>> the numbers with QoS monitor for AMD.
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v3: Made read_from_mc_dir function generic to both AMD and Intel.
>>      Rest are mostly related to rebase.
>>
>> v2: Replace perror with ksft_perror.
>> ---
>>   tools/testing/selftests/resctrl/resctrl_val.c | 80 ++++++++++++-------
>>   1 file changed, 50 insertions(+), 30 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
>> b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 23c0e0a1d845..ffacafb535cd 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -11,6 +11,7 @@
>>   #include "resctrl.h"
>>     #define UNCORE_IMC        "uncore_imc_"
>> +#define AMD_UMC            "amd_umc_"
>>   #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"
>> @@ -128,7 +129,7 @@ static int open_perf_event(int i, int cpu_no, int j)
>>   }
>>     /* Get type and config (read and write) of an MC counter */
>> -static int read_from_mc_dir(char *mc_dir, int count)
>> +static int read_from_mc_dir(char *mc_dir, int count, int vendor)
>>   {
>>       char cas_count_cfg[1024], mc_counter_cfg[1024],
>> mc_counter_type[1024];
>>       FILE *fp;
>> @@ -152,41 +153,56 @@ static int read_from_mc_dir(char *mc_dir, int count)
>>       mc_counters_config[count][WRITE].type =
>>                   mc_counters_config[count][READ].type;
>>   -    /* Get read config */
>> -    sprintf(mc_counter_cfg, "%s%s", mc_dir, READ_FILE_NAME);
>> -    fp = fopen(mc_counter_cfg, "r");
>> -    if (!fp) {
>> -        ksft_perror("Failed to open MC config file");
>> +    if (vendor == ARCH_AMD) {
>> +        /*
>> +         * Setup the event and umasks for UMC events
>> +         * Number of CAS commands sent for reads:
>> +         * EventCode = 0x0a, umask = 0x1
>> +         * Number of CAS commands sent for writes:
>> +         * EventCode = 0x0a, umask = 0x2
>> +         */
>> +        mc_counters_config[count][READ].event = 0xa;
>> +        mc_counters_config[count][READ].umask = 0x1;
>>   -        return -1;
>> -    }
>> -    if (fscanf(fp, "%s", cas_count_cfg) <= 0) {
>> -        ksft_perror("Could not get MC cas count read");
>> +        mc_counters_config[count][WRITE].event = 0xa;
>> +        mc_counters_config[count][WRITE].umask = 0x2;
>> +    } else {
>> +        /* Get read config */
>> +        sprintf(mc_counter_cfg, "%s%s", mc_dir, READ_FILE_NAME);
>> +        fp = fopen(mc_counter_cfg, "r");
>> +        if (!fp) {
>> +            ksft_perror("Failed to open MC config file");
>> +
>> +            return -1;
>> +        }
>> +        if (fscanf(fp, "%s", cas_count_cfg) <= 0) {
>> +            ksft_perror("Could not get MC cas count read");
>> +            fclose(fp);
>> +
>> +            return -1;
>> +        }
>>           fclose(fp);
>>   -        return -1;
>> -    }
>> -    fclose(fp);
>> +        get_event_and_umask(cas_count_cfg, count, READ);
>>   -    get_event_and_umask(cas_count_cfg, count, READ);
>> +        /* Get write config */
>> +        sprintf(mc_counter_cfg, "%s%s", mc_dir, WRITE_FILE_NAME);
>> +        fp = fopen(mc_counter_cfg, "r");
>> +        if (!fp) {
>> +            ksft_perror("Failed to open MC config file");
>>   -    /* Get write config */
>> -    sprintf(mc_counter_cfg, "%s%s", mc_dir, WRITE_FILE_NAME);
>> -    fp = fopen(mc_counter_cfg, "r");
>> -    if (!fp) {
>> -        ksft_perror("Failed to open MC config file");
>> +            return -1;
>> +        }
>> +        if  (fscanf(fp, "%s", cas_count_cfg) <= 0) {
>> +            ksft_perror("Could not get MC cas count write");
>> +            fclose(fp);
>>   -        return -1;
>> -    }
>> -    if  (fscanf(fp, "%s", cas_count_cfg) <= 0) {
>> -        ksft_perror("Could not get MC cas count write");
>> +            return -1;
>> +        }
>>           fclose(fp);
>>   -        return -1;
>> +        get_event_and_umask(cas_count_cfg, count, WRITE);
>>       }
>> -    fclose(fp);
>> -
>> -    get_event_and_umask(cas_count_cfg, count, WRITE);
>>         return 0;
>>   }
>> @@ -213,6 +229,8 @@ static int num_of_mem_controllers(void)
>>       vendor = get_vendor();
>>       if (vendor == ARCH_INTEL) {
>>           sysfs_name = UNCORE_IMC;
>> +    } else if (vendor == ARCH_AMD) {
>> +        sysfs_name = AMD_UMC;
>>       } else {
>>           ksft_perror("Unsupported vendor!\n");
>>           return -1;
>> @@ -228,6 +246,7 @@ static int num_of_mem_controllers(void)
>>               /*
>>                * imc counters are named as "uncore_imc_<n>", hence
>>                * increment the pointer to point to <n>.
>> +             * For AMD, it will be amd_umc_<n>.
>>                */
>>               temp = temp + strlen(sysfs_name);
>>   @@ -239,7 +258,7 @@ static int num_of_mem_controllers(void)
>>               if (temp[0] >= '0' && temp[0] <= '9') {
>>                   sprintf(mc_dir, "%s/%s/", DYN_PMU_PATH,
>>                       ep->d_name);
>> -                ret = read_from_mc_dir(mc_dir, count);
>> +                ret = read_from_mc_dir(mc_dir, count, vendor);
>>                   if (ret) {
>>                       closedir(dp);
>>   @@ -250,8 +269,9 @@ static int num_of_mem_controllers(void)
>>           }
>>           closedir(dp);
>>           if (count == 0) {
>> -            ksft_print_msg("Unable to find MC counters\n");
>> -
>> +            ksft_print_msg("Unable to find iMC/UMC counters\n");
>> +            if (vendor == ARCH_AMD)
>> +                ksft_print_msg("Try loading amd_uncore module\n");
>>               return -1;
>>           }
>>       } else {
> 
> Can all the vendor checking be contained in num_of_mem_controllers()
> instead of
> scattered through multiple layers? There can be two vendor specific
> functions to
> initialize mc_counters_config[][]. Only the type setting code ends up
> being shared so that can be split into a function that is called by both
> vendor functions?

Yes, We can do that. Will add it in v4.
-- 
Thanks
Babu Moger

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ