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: Thu, 14 Sep 2023 14:06:54 +0300 (EEST)	[thread overview]
Message-ID: <dfc53e-3f92-82e4-6af-d1a28e8c199a@linux.intel.com> (raw)
In-Reply-To: <8eb32195-102b-cbf5-3ea0-88550cc81de6@intel.com>

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

On Wed, 13 Sep 2023, Reinette Chatre wrote:
> 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.

I'll just make that change part of this series and use also it as 
dependency. Making an intermediate step just for stable that is going to 
immediately removed in mainline would just causing the code to diverge 
unnecessarily, IMO.

There's also a small risk for some other bug that does not cause compile 
to fail due to differences because of a late define for _GNU_SOURCE. I 
don't find it very likely but seems possible due to differences in some 
constant values (not that the resctrl selftest code is very good at using 
those defined constants in the first place, there are plenty of literals 
still to cleanup).

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

Thanks, I'll do that.

-- 
 i.