Re: [PATCH v2 4/9] selftests/resctrl: Support multiple events associated with iMC

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Ilpo,

On 3/6/26 2:18 AM, Ilpo Järvinen wrote:
> On Tue, 3 Mar 2026, Reinette Chatre wrote:
> 
>> The resctrl selftests discover needed parameters to perf_event_open() via
>> sysfs. The PMU associated with every memory controller (iMC) is discovered
>> via the /sys/bus/event_source/devices/uncore_imc_N/type file while
>> the read memory bandwidth event type and umask is discovered via
>> /sys/bus/event_source/devices/uncore_imc_N/events/cas_count_read.
>>
>> Newer systems may have multiple events that expose read memory bandwidth.
>> For example,
>>  /sys/bus/event_source/devices/uncore_imc_N/events/cas_count_read_sch0
>>  /sys/bus/event_source/devices/uncore_imc_N/events/cas_count_read_sch1
>>
>> Support parsing of iMC PMU properties when the PMU may have multiple events
>> to measure read memory bandwidth. The PMU only needs to be discovered once.
>> Split the parsing of event details from actual PMU discovery in order to
>> loop over all events associated with the PMU. Match all events with the
>> cas_count_read prefix instead of requiring there to be one file with that
>> name.
>>
>> Make the parsing code more robust. With strings passed around to create
>> needed paths, use snprintf() instead of sprintf() to ensure there is
>> always enough space to create the path. Ensure there is enough room in
>> imc_counters_config[] before attempting to add an entry.
>>
>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>> Reviewed-by: Zide Chen <zide.chen@intel.com>
>> ---
>> Changes since v1:
>> - Add Zide Chen's RB tag.
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 112 ++++++++++++++----
>>  1 file changed, 90 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 25c8101631e0..7aae0cc5aee9 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -11,10 +11,10 @@
>>  #include "resctrl.h"
>>  
>>  #define UNCORE_IMC		"uncore_imc"
>> -#define READ_FILE_NAME		"events/cas_count_read"
>> +#define READ_FILE_NAME		"cas_count_read"
>>  #define DYN_PMU_PATH		"/sys/bus/event_source/devices"
>>  #define SCALE			0.00006103515625
>> -#define MAX_IMCS		20
>> +#define MAX_IMCS		40
>>  #define MAX_TOKENS		5
>>  
>>  #define CON_MBM_LOCAL_BYTES_PATH		\
>> @@ -109,21 +109,102 @@ static int open_perf_read_event(int i, int cpu_no)
>>  	return 0;
>>  }
>>  
>> +static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>> +				    unsigned int *count)
>> +{
>> +	char imc_events[1024], imc_counter_cfg[1024], cas_count_cfg[1024];
> 
> The first two are paths, right? PATH_MAX should be used instead of the 
> literals.

Yes, they are paths. Thanks. Will use PATH_MAX.

> 
>> +	unsigned int org_count = *count;
> 
> orig_count is less ambiguous name.

Sure.

> 
>> +	struct dirent *ep;
>> +	int path_len;
>> +	int ret = -1;
>> +	FILE *fp;
>> +	DIR *dp;
>> +
>> +	path_len = snprintf(imc_events, sizeof(imc_events), "%sevents", imc_dir);
>> +	if (path_len >= sizeof(imc_events)) {
>> +		ksft_print_msg("Unable to create path to %sevents\n", imc_dir);
>> +		return -1;
>> +	}
>> +	dp = opendir(imc_events);
>> +	if (dp) {
>> +		while ((ep = readdir(dp))) {
>> +			/*
>> +			 * Parse all event files with READ_FILE_NAME
>> +			 * prefix that contain the event number and umask.
>> +			 * Skip files containing "." that contain unused
>> +			 * properties of event.
>> +			 */
>> +			if (!strstr(ep->d_name, READ_FILE_NAME) ||
>> +			    strchr(ep->d_name, '.'))
>> +				continue;
>> +
>> +			path_len = snprintf(imc_counter_cfg, sizeof(imc_counter_cfg),
>> +					    "%s/%s", imc_events, ep->d_name);
>> +			if (path_len >= sizeof(imc_counter_cfg)) {
>> +				ksft_print_msg("Unable to create path to %s/%s\n",
>> +					       imc_events, ep->d_name);
>> +				goto out_close;
>> +			}
>> +			fp = fopen(imc_counter_cfg, "r");
>> +			if (!fp) {
>> +				ksft_perror("Failed to open iMC config file");
>> +				goto out_close;
>> +			}
>> +			if (fscanf(fp, "%1023s", cas_count_cfg) <= 0) {
>> +				ksft_perror("Could not get iMC cas count read");
>> +				fclose(fp);
>> +				goto out_close;
>> +			}
>> +			fclose(fp);
> 
> I'd prefer:
> 
> 	xx = fscanf(...);
> 	fclose(fp);
> 	if (xx) {
> 		...
> 
> ...But it is up to you ("ret" cannot be used as xx as is).

ok. I do not really like to use ret for xx since it generates a bit of churn to
reset on success as well as failure paths after the fscanf(). I can introduce a new
local variable that should help with readability.

> 
>> +			if (*count >= MAX_IMCS) {
>> +				ksft_print_msg("Maximum iMC count exceeded\n");
>> +				goto out_close;
>> +			}
>> +
>> +			imc_counters_config[*count].type = type;
>> +			get_read_event_and_umask(cas_count_cfg, *count);
>> +			/* Do not fail after incrementing *count. */
>> +			*count += 1;
>> +		}
>> +		if (*count == org_count) {
>> +			ksft_print_msg("Unable to find events in %s\n", imc_events);
>> +			goto out_close;
>> +		}
>> +	} else {
>> +		ksft_perror("Unable to open PMU events directory");
>> +		goto out;
> 
> Reverse the logic (handle error first), it reduces the indentation level 
> of the loop.

Right. Will do.

> 
>> +	}
>> +	ret = 0;
>> +out_close:
>> +	closedir(dp);
>> +out:
>> +	return ret;
>> +}
>> +
>>  /* Get type and config of an iMC counter's read event. */
>>  static int read_from_imc_dir(char *imc_dir, unsigned int *count)
>>  {
>> -	char cas_count_cfg[1024], imc_counter_cfg[1024], imc_counter_type[1024];
>> +	char imc_counter_type[1024];

I'll also change imc_counter_type to PATH_MAX.


Reinette