[V2,14/19] selftests/resctrl: Skip the test if requested resctrl feature is not supported
diff mbox series

Message ID 485f834d4f1188056b306263d800bffbc0c43430.1589835155.git.sai.praneeth.prakhya@intel.com
State New
Headers show
Series
  • Miscellaneous fixes for resctrl selftests
Related show

Commit Message

Prakhya, Sai Praneeth May 18, 2020, 10:08 p.m. UTC
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(-)

Comments

Reinette Chatre May 20, 2020, 11:46 p.m. UTC | #1
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
Prakhya, Sai Praneeth May 21, 2020, 5:12 p.m. UTC | #2
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

Patch
diff mbox series

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, &param);
 	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, &param);
 	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++;