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@xxxxxxxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
>
> 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.
