| Message ID | 7e3e4b91f5786a489e68eecda21e1d8049b60181.1583657204.git.sai.praneeth.prakhya@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Miscellaneous fixes for resctrl selftests | expand |
Hi Sai, On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > From: Reinette Chatre <reinette.chatre@intel.com> > > The intention of the resctrl selftests is to only run the tests > associated with the feature(s) supported by the platform. Through > parsing of the feature flags found in /proc/cpuinfo it is possible > to learn which features are supported by the plaform. > > There are currently two issues with the platform feature detection that > together result in tests always being run, whether the platform supports > a feature or not. First, the parsing of the the feature flags loads the > line containing the flags in a buffer that is too small (256 bytes) to > always contain all flags. The consequence is that the flags of the features > being tested for may not be present in the buffer. Second, the actual > test for presence of a feature has an error in the logic, negating the > test for a particular feature flag instead of testing for the presence of a > particular feature flag. > > These two issues combined results in all tests being run on all > platforms, whether the feature is supported or not. > > Fix these issue by (1) increasing the buffer size being used to parse > the feature flags, and (2) change the logic to test for presence of the > feature being tested for. > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> > --- > tools/testing/selftests/resctrl/resctrlfs.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > index 19c0ec4045a4..226dd7fdcfb1 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -596,11 +596,11 @@ bool check_resctrlfs_support(void) > > char *fgrep(FILE *inf, const char *str) > { > - char line[256]; > int slen = strlen(str); > + char line[2048]; > > while (!feof(inf)) { > - if (!fgets(line, 256, inf)) > + if (!fgets(line, 2048, inf)) > break; > if (strncmp(line, str, slen)) > continue; > @@ -631,7 +631,7 @@ bool validate_resctrl_feature_request(char *resctrl_val) > if (res) { > char *s = strchr(res, ':'); > > - found = s && !strstr(s, resctrl_val); > + found = s && strstr(s, resctrl_val); > free(res); > } > fclose(inf); > Please note that this is only a partial fix. The current feature detection relies on the feature flags found in /proc/cpuinfo. Quirks and kernel boot parameters are not taken into account. This fix only addresses the parsing of feature flags. If a feature has been disabled via kernel boot parameter or quirk then the resctrl tests would still attempt to run the test for it. Reinette
Hi Reinette, > -----Original Message----- > From: Reinette Chatre <reinette.chatre@intel.com> > Sent: Monday, March 9, 2020 2:45 PM > To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>; > shuah@kernel.org; linux-kselftest@vger.kernel.org > Cc: tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; Luck, Tony > <tony.luck@intel.com>; babu.moger@amd.com; james.morse@arm.com; > Shankar, Ravi V <ravi.v.shankar@intel.com>; Yu, Fenghua > <fenghua.yu@intel.com>; x86@kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH V1 01/13] selftests/resctrl: Fix feature detection > > Hi Sai, > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > > From: Reinette Chatre <reinette.chatre@intel.com> > > > > The intention of the resctrl selftests is to only run the tests > > associated with the feature(s) supported by the platform. Through > > parsing of the feature flags found in /proc/cpuinfo it is possible to > > learn which features are supported by the plaform. > > > > There are currently two issues with the platform feature detection > > that together result in tests always being run, whether the platform > > supports a feature or not. First, the parsing of the the feature flags > > loads the line containing the flags in a buffer that is too small (256 > > bytes) to always contain all flags. The consequence is that the flags > > of the features being tested for may not be present in the buffer. > > Second, the actual test for presence of a feature has an error in the > > logic, negating the test for a particular feature flag instead of > > testing for the presence of a particular feature flag. > > > > These two issues combined results in all tests being run on all > > platforms, whether the feature is supported or not. > > > > Fix these issue by (1) increasing the buffer size being used to parse > > the feature flags, and (2) change the logic to test for presence of > > the feature being tested for. > > > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> > > --- > > tools/testing/selftests/resctrl/resctrlfs.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c > > b/tools/testing/selftests/resctrl/resctrlfs.c > > index 19c0ec4045a4..226dd7fdcfb1 100644 > > --- a/tools/testing/selftests/resctrl/resctrlfs.c > > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > > @@ -596,11 +596,11 @@ bool check_resctrlfs_support(void) > > > > char *fgrep(FILE *inf, const char *str) { > > - char line[256]; > > int slen = strlen(str); > > + char line[2048]; > > > > while (!feof(inf)) { > > - if (!fgets(line, 256, inf)) > > + if (!fgets(line, 2048, inf)) > > break; > > if (strncmp(line, str, slen)) > > continue; > > @@ -631,7 +631,7 @@ bool validate_resctrl_feature_request(char > *resctrl_val) > > if (res) { > > char *s = strchr(res, ':'); > > > > - found = s && !strstr(s, resctrl_val); > > + found = s && strstr(s, resctrl_val); > > free(res); > > } > > fclose(inf); > > > > Please note that this is only a partial fix. The current feature detection relies on > the feature flags found in /proc/cpuinfo. Quirks and kernel boot parameters are > not taken into account. This fix only addresses the parsing of feature flags. If a > feature has been disabled via kernel boot parameter or quirk then the resctrl > tests would still attempt to run the test for it. That's a good point and makes sense to me. I think we could fix it in two ways 1. grep for strings in dmesg but that will still leave ambiguity in deciding b/w mbm and cqm because kernel prints "resctrl: L3 monitoring detected" for both the features 2. Check in "info" directory a. For cat_l3, we could search for info/L3 b. For mba, we could search for info/MB c. For cqm and mbm, we could search for specified string in info/L3_MON/mon_features I think option 2 might be better because it can handle all cases, please let me know what you think. Regards, Sai
Hi Sai, On 3/9/2020 3:22 PM, Prakhya, Sai Praneeth wrote: > Hi Reinette, > >> -----Original Message----- >> From: Reinette Chatre <reinette.chatre@intel.com> >> Sent: Monday, March 9, 2020 2:45 PM >> On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: [SNIP] >> Please note that this is only a partial fix. The current feature detection relies on >> the feature flags found in /proc/cpuinfo. Quirks and kernel boot parameters are >> not taken into account. This fix only addresses the parsing of feature flags. If a >> feature has been disabled via kernel boot parameter or quirk then the resctrl >> tests would still attempt to run the test for it. > > That's a good point and makes sense to me. I think we could fix it in two ways > 1. grep for strings in dmesg but that will still leave ambiguity in deciding b/w mbm and cqm because kernel prints "resctrl: L3 monitoring detected" for both the features > 2. Check in "info" directory > a. For cat_l3, we could search for info/L3 > b. For mba, we could search for info/MB > c. For cqm and mbm, we could search for specified string in info/L3_MON/mon_features > > I think option 2 might be better because it can handle all cases, please let me know what you think. I agree. For the reasons you mention and also that (1) may not be possible if the loglevel prevents those lines from being printed. Reinette
Hi Reinette, > -----Original Message----- > From: Reinette Chatre <reinette.chatre@intel.com> > Sent: Monday, March 9, 2020 3:34 PM [SNIP] > > That's a good point and makes sense to me. I think we could fix it in > > two ways 1. grep for strings in dmesg but that will still leave > > ambiguity in deciding b/w mbm and cqm because kernel prints "resctrl: L3 > monitoring detected" for both the features 2. Check in "info" directory > > a. For cat_l3, we could search for info/L3 > > b. For mba, we could search for info/MB > > c. For cqm and mbm, we could search for specified string in > > info/L3_MON/mon_features > > > > I think option 2 might be better because it can handle all cases, please let me > know what you think. > > I agree. For the reasons you mention and also that (1) may not be possible if the > loglevel prevents those lines from being printed. Makes sense. I will work on the fix. Regards, Sai
Hi Sai, On 3/9/2020 3:51 PM, Prakhya, Sai Praneeth wrote: >> -----Original Message----- >> From: Reinette Chatre <reinette.chatre@intel.com> >> Sent: Monday, March 9, 2020 3:34 PM > > [SNIP] > >>> That's a good point and makes sense to me. I think we could fix it in >>> two ways 1. grep for strings in dmesg but that will still leave >>> ambiguity in deciding b/w mbm and cqm because kernel prints "resctrl: L3 >> monitoring detected" for both the features 2. Check in "info" directory >>> a. For cat_l3, we could search for info/L3 >>> b. For mba, we could search for info/MB >>> c. For cqm and mbm, we could search for specified string in >>> info/L3_MON/mon_features >>> >>> I think option 2 might be better because it can handle all cases, please let me >> know what you think. >> >> I agree. For the reasons you mention and also that (1) may not be possible if the >> loglevel prevents those lines from being printed. > > Makes sense. I will work on the fix. One more note about this ... from what I can tell the test for a feature currently fails if the platform does not support the feature. Would it be possible to just skip the test in this case instead? Reinette
Hi Reinette, On Wed, 2020-03-11 at 11:06 -0700, Reinette Chatre wrote: > Hi Sai, > > On 3/9/2020 3:51 PM, Prakhya, Sai Praneeth wrote: > > > -----Original Message----- > > > From: Reinette Chatre <reinette.chatre@intel.com> > > > Sent: Monday, March 9, 2020 3:34 PM > > > > [SNIP] > > > > > > That's a good point and makes sense to me. I think we could fix it in > > > > two ways 1. grep for strings in dmesg but that will still leave > > > > ambiguity in deciding b/w mbm and cqm because kernel prints "resctrl: > > > > L3 > > > monitoring detected" for both the features 2. Check in "info" directory > > > > a. For cat_l3, we could search for info/L3 > > > > b. For mba, we could search for info/MB > > > > c. For cqm and mbm, we could search for specified string in > > > > info/L3_MON/mon_features > > > > > > > > I think option 2 might be better because it can handle all cases, > > > > please let me > > > know what you think. > > > > > > I agree. For the reasons you mention and also that (1) may not be > > > possible if the > > > loglevel prevents those lines from being printed. > > > > Makes sense. I will work on the fix. > > One more note about this ... from what I can tell the test for a feature > currently fails if the platform does not support the feature. Would it > be possible to just skip the test in this case instead? That's because the output of the test should be just "ok" or "not ok". I can change it to something like "# Skip <test_name> because platform doesn't support the feature", but not really sure if it complies with TAP 13 protocol. Regards, Sai
Hi Sai, On 3/11/2020 11:22 AM, Sai Praneeth Prakhya wrote: > Hi Reinette, > > On Wed, 2020-03-11 at 11:06 -0700, Reinette Chatre wrote: >> Hi Sai, >> >> On 3/9/2020 3:51 PM, Prakhya, Sai Praneeth wrote: >>>> -----Original Message----- >>>> From: Reinette Chatre <reinette.chatre@intel.com> >>>> Sent: Monday, March 9, 2020 3:34 PM >>> >>> [SNIP] >>> >>>>> That's a good point and makes sense to me. I think we could fix it in >>>>> two ways 1. grep for strings in dmesg but that will still leave >>>>> ambiguity in deciding b/w mbm and cqm because kernel prints "resctrl: >>>>> L3 >>>> monitoring detected" for both the features 2. Check in "info" directory >>>>> a. For cat_l3, we could search for info/L3 >>>>> b. For mba, we could search for info/MB >>>>> c. For cqm and mbm, we could search for specified string in >>>>> info/L3_MON/mon_features >>>>> >>>>> I think option 2 might be better because it can handle all cases, >>>>> please let me >>>> know what you think. >>>> >>>> I agree. For the reasons you mention and also that (1) may not be >>>> possible if the >>>> loglevel prevents those lines from being printed. >>> >>> Makes sense. I will work on the fix. >> >> One more note about this ... from what I can tell the test for a feature >> currently fails if the platform does not support the feature. Would it >> be possible to just skip the test in this case instead? > > That's because the output of the test should be just "ok" or "not ok". The output could be something like: ok MBA # SKIP MBA is not supported > I can change it to something like "# Skip <test_name> because platform doesn't > support the feature", but not really sure if it complies with TAP 13 protocol. Please consider the "skip" directive at https://testanything.org/tap-version-13-specification.html Reinette
Hi Reinette, On Wed, 2020-03-11 at 11:45 -0700, Reinette Chatre wrote: > Hi Sai, > > On 3/11/2020 11:22 AM, Sai Praneeth Prakhya wrote: > > Hi Reinette, > > > > On Wed, 2020-03-11 at 11:06 -0700, Reinette Chatre wrote: > > > Hi Sai, > > > > > > On 3/9/2020 3:51 PM, Prakhya, Sai Praneeth wrote: > > > > > -----Original Message----- > > > > > From: Reinette Chatre <reinette.chatre@intel.com> > > > > > Sent: Monday, March 9, 2020 3:34 PM > > > > > > > > [SNIP] > > > > > > > > > > That's a good point and makes sense to me. I think we could fix it > > > > > > in > > > > > > two ways 1. grep for strings in dmesg but that will still leave > > > > > > ambiguity in deciding b/w mbm and cqm because kernel prints > > > > > > "resctrl: > > > > > > L3 > > > > > monitoring detected" for both the features 2. Check in "info" > > > > > directory > > > > > > a. For cat_l3, we could search for info/L3 > > > > > > b. For mba, we could search for info/MB > > > > > > c. For cqm and mbm, we could search for specified string in > > > > > > info/L3_MON/mon_features > > > > > > > > > > > > I think option 2 might be better because it can handle all cases, > > > > > > please let me > > > > > know what you think. > > > > > > > > > > I agree. For the reasons you mention and also that (1) may not be > > > > > possible if the > > > > > loglevel prevents those lines from being printed. > > > > > > > > Makes sense. I will work on the fix. > > > > > > One more note about this ... from what I can tell the test for a feature > > > currently fails if the platform does not support the feature. Would it > > > be possible to just skip the test in this case instead? > > > > That's because the output of the test should be just "ok" or "not ok". > > The output could be something like: > > ok MBA # SKIP MBA is not supported Makes sense.. I will fix it. > > I can change it to something like "# Skip <test_name> because platform > > doesn't > > support the feature", but not really sure if it complies with TAP 13 > > protocol. > > Please consider the "skip" directive at > https://testanything.org/tap-version-13-specification.html Sure! thanks for the link :) Regards, Sai
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 19c0ec4045a4..226dd7fdcfb1 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -596,11 +596,11 @@ bool check_resctrlfs_support(void) char *fgrep(FILE *inf, const char *str) { - char line[256]; int slen = strlen(str); + char line[2048]; while (!feof(inf)) { - if (!fgets(line, 256, inf)) + if (!fgets(line, 2048, inf)) break; if (strncmp(line, str, slen)) continue; @@ -631,7 +631,7 @@ bool validate_resctrl_feature_request(char *resctrl_val) if (res) { char *s = strchr(res, ':'); - found = s && !strstr(s, resctrl_val); + found = s && strstr(s, resctrl_val); free(res); } fclose(inf);