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