Re: [PATCH v6 1/2] selftests/resctrl: Adjust effective L3 cache size with SNC enabled

[Maciej Wieczor-Retman]

On 2024-12-03 at 15:26:41 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 12/2/24 3:08 AM, Maciej Wieczor-Retman wrote:
>
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index d38d6dd90be4..50561993d37c 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -13,6 +13,8 @@
>>  
>>  #include "resctrl.h"
>>  
>> +int snc_unreliable;
>> +
>>  static int find_resctrl_mount(char *buffer)
>>  {
>>  	FILE *mounts;
>> @@ -156,6 +158,90 @@ int get_domain_id(const char *resource, int cpu_no, int *domain_id)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Count number of CPUs in a /sys bitmap
>> + */
>> +static unsigned int count_sys_bitmap_bits(char *name)
>> +{
>> +	FILE *fp = fopen(name, "r");
>> +	int count = 0, c;
>> +
>> +	if (!fp)
>> +		return 0;
>> +
>> +	while ((c = fgetc(fp)) != EOF) {
>> +		if (!isxdigit(c))
>> +			continue;
>> +		switch (c) {
>> +		case 'f':
>> +			count++;
>> +		case '7': case 'b': case 'd': case 'e':
>> +			count++;
>> +		case '3': case '5': case '6': case '9': case 'a': case 'c':
>> +			count++;
>> +		case '1': case '2': case '4': case '8':
>> +			count++;
>> +		}
>> +	}
>> +	fclose(fp);
>> +
>
>running this through a syntax checker triggers a couple of complaints due to the
>missing break statements. I think this can be made more robust by making use of
>"fallthrough" and "break". It looks like this can be obtained by including
>linux/compiler.h ... but from what I can tell care should be taken to set
>the include directory _after_ includink lib.mk so that top_srcdir is set
>correctly.

Sure, I'll look into it

>
>> +	return count;
>> +}
>> +
>> +static bool cpus_offline_empty(void)
>> +{
>> +	char offline_cpus_str[64];
>> +	FILE *fp;
>> +
>> +	fp = fopen("/sys/devices/system/cpu/offline", "r");
>
>Please check fp before use.

Will do

>
>> +	if (fscanf(fp, "%s", offline_cpus_str) < 0) {
>
>This needs something equivalent to 
>46058430fc5d ("selftests/resctrl: Protect against array overflow when reading strings")

Thanks, I'll add that protection here.

>
>> +		if (!errno) {
>> +			fclose(fp);
>> +			return 1;
>> +		}
>> +		ksft_perror("Could not read /sys/devices/system/cpu/offline");
>> +	}
>> +
>> +	fclose(fp);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Detect SNC by comparing #CPUs in node0 with #CPUs sharing LLC with CPU0.
>> + * If any CPUs are offline declare the detection as unreliable and skip the
>> + * tests.
>
>nit: "and skip the tests" can be dropped since the function need not make
>assumption about how callers will use it.

Right, sorry, will remove it.

>
>> + */
>> +int snc_nodes_per_l3_cache(void)
>> +{
>> +	int node_cpus, cache_cpus;
>> +	static int snc_mode;
>> +
>> +	if (!snc_mode) {
>> +		snc_mode = 1;
>> +		if (!cpus_offline_empty()) {
>> +			ksft_print_msg("Runtime SNC detection unreliable due to offline CPUs.\n");
>> +			ksft_print_msg("Setting SNC mode to disabled.\n");
>> +			snc_unreliable = 1;
>> +			return snc_mode;
>> +		}
>> +		node_cpus = count_sys_bitmap_bits("/sys/devices/system/node/node0/cpumap");
>> +		cache_cpus = count_sys_bitmap_bits("/sys/devices/system/cpu/cpu0/cache/index3/shared_cpu_map");
>> +
>> +		if (!node_cpus || !cache_cpus) {
>> +			ksft_print_msg("Could not determine Sub-NUMA Cluster mode.\n");
>> +			snc_unreliable = 1;
>> +			return snc_mode;
>> +		}
>> +		snc_mode = cache_cpus / node_cpus;
>> +
>> +		if (snc_mode > 1)
>> +			ksft_print_msg("SNC-%d mode discovered.\n", snc_mode);
>> +	}
>> +
>> +	return snc_mode;
>> +}
>> +
>>  /*
>>   * get_cache_size - Get cache size for a specified CPU
>>   * @cpu_no:	CPU number
>> @@ -211,6 +297,17 @@ int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size
>>  			break;
>>  	}
>>  
>> +	/*
>> +	 * The amount of cache represented by each bit in the masks
>> +	 * in the schemata file is reduced by a factor equal to SNC
>> +	 * nodes per L3 cache.
>> +	 * E.g. on a SNC-2 system with a 100MB L3 cache a test that
>> +	 * allocates memory from its local SNC node (default behavior
>> +	 * without using libnuma) will only see 50 MB llc_occupancy
>> +	 * with a fully populated L3 mask in the schemata file.
>> +	 */
>> +	if (cache_num == 3)
>> +		*cache_size /= snc_nodes_per_l3_cache();
>>  	return 0;
>>  }
>>  
>
>Reinette
>

-- 
Kind regards
Maciej Wieczór-Retman