Hi Reinette,
On 5/9/24 16:11, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/25/2024 1:17 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.mo...@amd.com>
>> ---
>> tools/testing/selftests/resctrl/resctrl_val.c | 67 ++++++++++++++++---
>> 1 file changed, 59 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
>> b/tools/testing/selftests/resctrl/resctrl_val.c
>> index e3b09128ec3d..d90d3196d7b5 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"
>> @@ -146,6 +147,47 @@ static int open_perf_event(int i, int cpu_no, int j)
>> return 0;
>> }
>>
>> +/* Get type and config (read and write) of an UMC counter */
>> +static int read_from_umc_dir(char *umc_dir, int count)
>> +{
>> + char umc_counter_type[PATH_MAX];
>> + FILE *fp;
>> +
>> + /* Get type of iMC counter */
>
> iMC counter?
Yes. Will fix it.
>
>> + sprintf(umc_counter_type, "%s%s", umc_dir, "type");
>> + fp = fopen(umc_counter_type, "r");
>> + if (!fp) {
>> + ksft_perror("Failed to open imc counter type file");
>
> Why go through effort of changing to generic names and then follow
> by using Intel naming in AMD specific code?
My bad.
>
>> + return -1;
>> + }
>> +
>> + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) {
>> + ksft_perror("Could not get imc type");
>
> Same here.
Sure.
>
>> + fclose(fp);
>> + return -1;
>> + }
>> +
>> + fclose(fp);
>> +
>> + imc_counters_config[count][WRITE].type =
>> + imc_counters_config[count][READ].type;
>> +
>
> Up to here seems to be a copy&paste of read_from_imc_dir(). Could you
> instead split read_from_imc_dir() so that AMD and Intel can share the
> code to determine type?
Sure. Will address this in next revision.
>
> Reinette
>
--
Thanks
Babu Moger
