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 <[email protected]>
>>> Cc: <[email protected]>
>>
>> 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
