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

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: "Shuah Khan" <shuah@kernel.org>,
	"Shuah Khan" <skhan@linuxfoundation.org>,
	linux-kselftest@vger.kernel.org,
	"Maciej Wieczór-Retman" <maciej.wieczor-retman@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH 3/5] selftests/resctrl: Refactor feature check to use resource and feature name
Date: Wed, 13 Sep 2023 14:02:58 +0300 (EEST)	[thread overview]
Message-ID: <eac7deb6-3593-7a59-7df8-208392254f7@linux.intel.com> (raw)
In-Reply-To: <b5a36b8a-c5c7-84a9-270e-bef4478d4bff@intel.com>

[-- Attachment #1: Type: text/plain, Size: 5802 bytes --]

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).

> >  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?

> >  	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;

-- 
 i.