| Message ID | 485f834d4f1188056b306263d800bffbc0c43430.1589835155.git.sai.praneeth.prakhya@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Miscellaneous fixes for resctrl selftests | expand |
Hi Sai, On 5/18/2020 3:08 PM, Sai Praneeth Prakhya wrote: > Presently, if a requested resctrl feature is not supported by H/W or is > disabled by user through kernel command line, the test suite treats it as > an error and hence would print something like "not ok MBA: schemata > change". But, not supporting a feature isn't a test error and hence > shouldn't printed as a failure. > > So, instead of treating it as an error, use the SKIP directive of TAP > protocol and print it as below i.e. don't report it as test failure. > > Sample o/p if CAT isn't supported: > "ok CAT # SKIP Hardware does not support CAT or CAT is disabled" > > Suggested-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> > --- ... > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index fb7703413be7..d45ae004ed77 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -170,6 +170,10 @@ int main(int argc, char **argv) > > if (!is_amd && mbm_test) { > printf("# Starting MBM BW change ...\n"); > + if (!validate_resctrl_feature_request("mbm")) { > + printf("ok MBM # SKIP Hardware does not support MBM or MBM is disabled\n"); > + goto test_mba; > + } > if (!has_ben) > sprintf(benchmark_cmd[5], "%s", "mba"); > res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd); > @@ -178,8 +182,13 @@ int main(int argc, char **argv) > tests_run++; > } > > +test_mba: I think this particular usage of goto could make the flow of the code harder to trace. Could the tests perhaps be moved to functions to avoid needing to jump like this? Perhaps there could be a new function per test, like run_mbm_test(), run_mba_test(), etc. with each test called when requested by user and with the test exiting cleanly if feature is not supported by the hardware. Reinette
Hi Reinette, > -----Original Message----- > From: Reinette Chatre <reinette.chatre@intel.com> > Sent: Wednesday, May 20, 2020 4:46 PM > To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>; > shuah@kernel.org; skhan@linuxfoundation.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; > dan.carpenter@oracle.com; dcb314@hotmail.com > Subject: Re: [PATCH V2 14/19] selftests/resctrl: Skip the test if requested resctrl > feature is not supported > > Hi Sai, [SNIP] > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c > > b/tools/testing/selftests/resctrl/resctrl_tests.c > > index fb7703413be7..d45ae004ed77 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > > @@ -170,6 +170,10 @@ int main(int argc, char **argv) > > > > if (!is_amd && mbm_test) { > > printf("# Starting MBM BW change ...\n"); > > + if (!validate_resctrl_feature_request("mbm")) { > > + printf("ok MBM # SKIP Hardware does not support > MBM or MBM is disabled\n"); > > + goto test_mba; > > + } > > if (!has_ben) > > sprintf(benchmark_cmd[5], "%s", "mba"); > > res = mbm_bw_change(span, cpu_no, bw_report, > benchmark_cmd); @@ > > -178,8 +182,13 @@ int main(int argc, char **argv) > > tests_run++; > > } > > > > +test_mba: > > I think this particular usage of goto could make the flow of the code harder to > trace. Could the tests perhaps be moved to functions to avoid needing to jump > like this? Perhaps there could be a new function per test, like run_mbm_test(), > run_mba_test(), etc. with each test called when requested by user and with the > test exiting cleanly if feature is not supported by the hardware. Makes sense. I will change it as suggested. Regards, Sai
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 1bce84e23783..a18a37ce626c 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -132,9 +132,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) if (ret) return ret; - if (!validate_resctrl_feature_request("cat")) - return -1; - /* Get default cbm mask for L3/L2 cache */ ret = get_cbm_mask(cache_type); if (ret) diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 282ba7fcf17c..119ae65abec7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -122,9 +122,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) if (ret) return ret; - if (!validate_resctrl_feature_request("cmt")) - return -1; - ret = get_cbm_mask("L3"); if (ret) return ret; diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index ba0234d4829e..6f09d46a5424 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -156,9 +156,6 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd) remove(RESULT_FILE_NAME); - if (!validate_resctrl_feature_request("mba")) - return -1; - ret = resctrl_val(benchmark_cmd, ¶m); if (ret) return ret; diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index ca610c3ebc8c..cb3113cb3b10 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -129,9 +129,6 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd) remove(RESULT_FILE_NAME); - if (!validate_resctrl_feature_request("mbm")) - return -1; - ret = resctrl_val(benchmark_cmd, ¶m); if (ret) return ret; diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index fb7703413be7..d45ae004ed77 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -170,6 +170,10 @@ int main(int argc, char **argv) if (!is_amd && mbm_test) { printf("# Starting MBM BW change ...\n"); + if (!validate_resctrl_feature_request("mbm")) { + printf("ok MBM # SKIP Hardware does not support MBM or MBM is disabled\n"); + goto test_mba; + } if (!has_ben) sprintf(benchmark_cmd[5], "%s", "mba"); res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd); @@ -178,8 +182,13 @@ int main(int argc, char **argv) tests_run++; } +test_mba: if (!is_amd && mba_test) { printf("# Starting MBA Schemata change ...\n"); + if (!validate_resctrl_feature_request("mba")) { + printf("ok MBA # SKIP Hardware does not support MBA or MBA is disabled\n"); + goto test_cmt; + } if (!has_ben) sprintf(benchmark_cmd[1], "%d", span); res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd); @@ -188,8 +197,13 @@ int main(int argc, char **argv) tests_run++; } +test_cmt: if (cmt_test) { printf("# Starting CMT test ...\n"); + if (!validate_resctrl_feature_request("cmt")) { + printf("ok CMT # SKIP Hardware does not support CMT or CMT is disabled\n"); + goto test_cat; + } if (!has_ben) sprintf(benchmark_cmd[5], "%s", "cmt"); res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd); @@ -198,8 +212,13 @@ int main(int argc, char **argv) tests_run++; } +test_cat: if (cat_test) { printf("# Starting CAT test ...\n"); + if (!validate_resctrl_feature_request("cat")) { + printf("ok CAT # SKIP Hardware does not support CAT or CAT is disabled\n"); + goto out; + } res = cat_perf_miss_val(cpu_no, no_of_bits, "L3"); printf("%sok CAT: test\n", res ? "not " : ""); tests_run++;
Presently, if a requested resctrl feature is not supported by H/W or is disabled by user through kernel command line, the test suite treats it as an error and hence would print something like "not ok MBA: schemata change". But, not supporting a feature isn't a test error and hence shouldn't printed as a failure. So, instead of treating it as an error, use the SKIP directive of TAP protocol and print it as below i.e. don't report it as test failure. Sample o/p if CAT isn't supported: "ok CAT # SKIP Hardware does not support CAT or CAT is disabled" Suggested-by: Reinette Chatre <reinette.chatre@intel.com> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> --- tools/testing/selftests/resctrl/cat_test.c | 3 --- tools/testing/selftests/resctrl/cmt_test.c | 3 --- tools/testing/selftests/resctrl/mba_test.c | 3 --- tools/testing/selftests/resctrl/mbm_test.c | 3 --- .../testing/selftests/resctrl/resctrl_tests.c | 19 +++++++++++++++++++ 5 files changed, 19 insertions(+), 12 deletions(-)