Re: [PATCH 3/5] selftests/resctrl: Refactor feature check to use resource and feature name

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

Hi Ilpo,

On 9/13/2023 4:02 AM, Ilpo Järvinen wrote:
> On Tue, 12 Sep 2023, Reinette Chatre wrote:
>> On 9/11/2023 4:19 AM, Ilpo Järvinen wrote:
>>> Feature check in validate_resctrl_feature_request() takes in the test
>>> name string and maps that to what to check per test.
>>>
>>> Pass resource and feature names to validate_resctrl_feature_request()
>>> directly rather than deriving them from the test name inside the
>>> function which makes the feature check easier to extend for new test
>>> cases.
>>>
>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> Cc: <stable@vger.kernel.org>
>>
>> This does not seem to be stable material.
> 
> Alone it isn't, but both 2/5 and this 3/5 are prerequisites for 4/5 as 
> shown by the tags there.
> 
>>> ---
>>>  tools/testing/selftests/resctrl/resctrl.h     |  6 +-
>>>  .../testing/selftests/resctrl/resctrl_tests.c | 10 +--
>>>  tools/testing/selftests/resctrl/resctrlfs.c   | 69 ++++++++-----------
>>>  3 files changed, 34 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>>> index dd07463cdf48..89ced4152933 100644
>>> --- a/tools/testing/selftests/resctrl/resctrl.h
>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
> 
>>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>>> index bd36ee206602..bd547a10791c 100644
>>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>>> @@ -10,6 +10,8 @@
>>>   */
>>>  #include "resctrl.h"
>>>  
>>> +#include <limits.h>
>>> +
>>
>> Could you please include <limits.h> before the local resctrl.h?
> 
> Believe me I tried that first but it did not work. So this intentionally 
> in the current order as resctrl.h defines _GNU_SOURCE which is among 
> things that tends to alter many things. If I reorder them, the build gives 
> me these issues:
> 
> resctrlfs.c: In function ‘taskset_benchmark’:
> resctrlfs.c:284:2: warning: implicit declaration of function ‘CPU_ZERO’; 
> did you mean ‘FP_ZERO’? [-Wimplicit-function-declaration]
>   284 |  CPU_ZERO(&my_set);
>       |  ^~~~~~~~
>       |  FP_ZERO
> resctrlfs.c:285:2: warning: implicit declaration of function ‘CPU_SET’ 
> [-Wimplicit-function-declaration]
>   285 |  CPU_SET(cpu_no, &my_set);
>       |  ^~~~~~~
> resctrlfs.c:287:6: warning: implicit declaration of function 
> ‘sched_setaffinity’ [-Wimplicit-function-declaration]
>   287 |  if (sched_setaffinity(bm_pid, sizeof(cpu_set_t), &my_set)) {
>       |      ^~~~~~~~~~~~~~~~~
> 
> It might be useful to move _GNU_SOURCE define into Makefile though to 
> avoid these kind of issues (but that's not material for this patch).

How about a #define _GNU_SOURCE in this file as an intermediate step?
I did see your patch making this change but cannot see how it is
coordinated with fixing the include order in this file.

> 
>>>  static int find_resctrl_mount(char *buffer)
>>>  {
>>>  	FILE *mounts;
>>> @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str)
>>>  
>>>  /*
>>>   * validate_resctrl_feature_request - Check if requested feature is valid.
>>> - * @resctrl_val:	Requested feature
>>> + * @resource:	Required resource (e.g., MB, L3, L2, L3_MON, etc.)
>>> + * @feature:	Feature to be checked under resource (can be NULL). This path
>>> + *		is relative to the resource path.
>>
>> I do not think "this path" is accurate. @feature is not a path but an entry
>> within the mon_features file.
> 
> Yes, agreed.
> 
>> Also please note that mon_features only exists for L3_MON, none of the other
>> listed resources have an associated mon_features file in resctrl. This
>> function is created to be generic has specific requirements on what
>> valid (never checked) parameters should be. This may be ok with the usage
>> but it should not pretend to be generic.
> 
> So are you recommending I split this function into two where the new one 
> would do the mon_features check?

No need to split the function. That seems overkill considering its
captive usage. I think a snippet making its usage clear will be helpful.
Something like:

	@feature: <description>. Can only be set for L3_MON. Must be
		  NULL for all other resources.

Please feel free to improve.


> 
>>>  	char *res;
>>>  	FILE *inf;
>>>  	int ret;
>>>  
>>> -	if (!resctrl_val)
>>> +	if (!resource)
>>>  		return false;
>>>  
>>>  	ret = find_resctrl_mount(NULL);
>>>  	if (ret)
>>>  		return false;
>>>  
>>> -	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
>>> -		if (!stat(L3_PATH, &statbuf))
>>> -			return true;
>>> -	} else if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
>>> -		if (!stat(MB_PATH, &statbuf))
>>> -			return true;
>>> -	} else if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
>>> -		   !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
>>> -		if (!stat(L3_MON_PATH, &statbuf)) {
>>> -			inf = fopen(L3_MON_FEATURES_PATH, "r");
>>> -			if (!inf)
>>> -				return false;
>>> -
>>> -			if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
>>> -				res = fgrep(inf, "llc_occupancy");
>>> -				if (res) {
>>> -					found = true;
>>> -					free(res);
>>> -				}
>>> -			}
>>> -
>>> -			if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
>>> -				res = fgrep(inf, "mbm_total_bytes");
>>> -				if (res) {
>>> -					free(res);
>>> -					res = fgrep(inf, "mbm_local_bytes");
>>> -					if (res) {
>>> -						found = true;
>>> -						free(res);
>>> -					}
>>> -				}
>>> -			}
>>> -			fclose(inf);
>>> -		}
>>> -	}
>>> +	snprintf(res_path, sizeof(res_path), "%s/%s", INFO_PATH, resource);
>>> +
>>> +	if (stat(res_path, &statbuf))
>>> +		return false;
>>> +
>>> +	if (!feature)
>>> +		return true;
>>> +
>>> +	snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource);
>>> +	inf = fopen(res_path, "r");
>>> +	if (!inf)
>>> +		return false;
>>> +
>>> +	res = fgrep(inf, feature);
>>> +	free(res);
>>> +	fclose(inf);
>>>  
>>> -	return found;
>>> +	return res;
>>
>> This is unexpected. Function should return bool but instead returns a char * that
>> has been freed?
> 
> Okay, I understand it looks confusing when relying on implicit conversion 
> to boolean (despite being functionally correct).
> 
> I can do this instead to make it more obvious the intention is to convert 
> the result to boolean and not return a pointer:
> 	return !!res;
> 

Looks good to me. 

Reinette