linux-kernel - Re: [PATCH v2 3/4] selftests/resctrl: Add support for MBM and MBA tests on AMD

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]

Date: Thu, 30 May 2024 11:08:20 -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 v2 3/4] selftests/resctrl: Add support for MBM and MBA
 tests on AMD

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.moger@....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