Re: [PATCH v2 3/4] selftests/resctrl: Split validate_resctrl_feature_request()

[Maciej Wieczór-Retman]

Hi!

On 2024-01-08 at 14:38:45 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote:
>> validate_resctrl_feature_request() is used to test both if a resource is
>> present in the info directory, and if a passed monitoring feature is
>> present in the mon_features file. There exists a different way to
>> represent feature support and that is by the presence of 0 or 1 in
>> single file in the info/resource directory. In this case the filename
>> represents what feature support is being indicated.
>> 
>> Split validate_resctrl_feature_request() into three smaller functions
>> that each accomplish one check:
>> - Resource directory presence in the /sys/fs/resctrl/info directory.
>> - Feature name presence in the /sys/fs/resctrl/info/RESOURCE/mon_features file.
>> - Feature file presence in a given /sys/fs/resctrl/info/RESOURCE directory.
>
>Please present refactoring of existing code and introduction of new
>feature as separate patches. 

Sure, will do.

>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> ---
>> Changelog v2:
>> - Added this patch.
>> 
>>  tools/testing/selftests/resctrl/cat_test.c  |  2 -
>>  tools/testing/selftests/resctrl/cmt_test.c  |  4 +-
>>  tools/testing/selftests/resctrl/mba_test.c  |  5 +-
>>  tools/testing/selftests/resctrl/mbm_test.c  |  6 +--
>>  tools/testing/selftests/resctrl/resctrl.h   |  6 ++-
>>  tools/testing/selftests/resctrl/resctrlfs.c | 59 +++++++++++++++++----
>>  6 files changed, 63 insertions(+), 19 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 39fc9303b8e8..7dc7206b3b99 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -1,9 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  /*
>>   * Cache Allocation Technology (CAT) test
>> - *
>>   * Copyright (C) 2018 Intel Corporation
>> - *
>>   * Authors:
>>   *    Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx>,
>>   *    Fenghua Yu <fenghua.yu@xxxxxxxxx>
>
>Some unrelated changes here. 

Oops, sorry, I'll remove these.

>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index 70333440ff2f..8546421f0940 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -725,6 +721,25 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
>>  	if (stat(res_path, &statbuf))
>>  		return false;
>>  
>> +	return true;
>> +}
>> +
>> +/*
>> + * resctrl_mon_feature_exists - Check if requested feature is valid.
>> + * @resource:	Required resource (e.g., MB, L3, L2, L3_MON, etc.)
>
>If this is intended for the monitoring resource and L3_MON (per below) is the
>only valid resource then @resource cannot be all of the examples shown. Why is
>the @resource argument needed?

I see now that these functions turned out rather silly. I'll redo them according
to all your other comments. Thanks for pointing these out!

>> + * @feature:	Required monitor feature (in mon_features file). Can only be
>> + *		set for L3_MON. Must be NULL for all other resources.
>
>Which other resources?

I'll remove it and just put L3_MON in the path.

>> + *
>> + * Return: True if the resource/feature is supported, else false. False is
>> + *         also returned if resctrl FS is not mounted.
>> + */
>> +bool resctrl_mon_feature_exists(const char *resource,
>> +				const char *feature)
>> +{
>> +	char res_path[PATH_MAX];
>> +	char *res;
>> +	FILE *inf;
>> +
>>  	if (!feature)
>>  		return true;
>
>Doesn't this mean that resctrl_mon_feature_exists(NULL, NULL) will return true? 

I'll get rid of this check.

>>  
>> @@ -740,9 +755,35 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
>>  	return !!res;
>>  }
>>  
>> +/*
>> + * resctrl_cache_feature_exists - Check if a file that indicates a
>> + * cache related feature support is present.
>
>Seems like this is not really specific to a cache ... it can
>check for any info file related to any resource.

Right, I'll rewrite this.

>
>> + * @resource:	Required cache resource (L3 or L2)
>> + * @feature:	Required cache feature.
>
>This seems to assume some usage of this utility. What if it
>is, for example, resource_info_file_exists() or resource_info_file_readable()?

Yes, I think resource_info_file_exists() fits better.

>> + *
>> + * Return: True if the feature is supported, else false.
>> + */
>> +bool resctrl_cache_feature_exists(const char *resource,
>> +				  const char *feature)
>> +{
>> +	char res_path[PATH_MAX];
>> +	struct stat statbuf;
>> +
>> +	if (!feature)
>> +		return true;
>
>resctrl_cache_feature_exists(NULL, NULL) will return true, no?

I'll remove this check.

>> +
>> +	snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
>> +		 feature);
>> +
>> +	if (stat(res_path, &statbuf))
>> +		return false;
>
>I think it will be more robust to look at statbuf to learn if the file type
>is correct and the file is actually readable.

Could that file be unreadable or of wrong type?

Also I thought that since read_info_res_file() opens and reads that file any
errors should be handled there. Shouldn't this part of the test only return
whether the file is there or not since that indicates if something is supported
in the kernel?

>> +
>> +	return true;
>> +}
>> +
>>  bool test_resource_feature_check(const struct resctrl_test *test)
>>  {
>> -	return validate_resctrl_feature_request(test->resource, NULL);
>> +	return resctrl_resource_exists(test->resource);
>>  }
>>  
>>  int filter_dmesg(void)
>
>Reinette

-- 
Kind regards
Maciej Wieczór-Retman