| Message ID | 20221101094341.3383073-6-tan.shaopeng@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some improvements of resctrl selftest | expand |
On 11/1/22 03:43, Shaopeng Tan wrote: > Before exiting each test function(run_cmt/cat/mbm/mba_test()), > test results("ok","not ok") are printed by ksft_test_result() and then > temporary result files are cleaned by function > cmt/cat/mbm/mba_test_cleanup(). > However, before running ksft_test_result(), > function cmt/cat/mbm/mba_test_cleanup() > has been run in each test function as follows: > cmt_resctrl_val() > cat_perf_miss_val() > mba_schemata_change() > mbm_bw_change() > > Remove duplicate codes that clear each test result file. This isn't making much sense to me. Please include test report before and after this change in the change log. > > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > --- thanks, -- Shuah
Hi Shaopeng, On 11/1/2022 2:43 AM, Shaopeng Tan wrote: > Before exiting each test function(run_cmt/cat/mbm/mba_test()), > test results("ok","not ok") are printed by ksft_test_result() and then > temporary result files are cleaned by function > cmt/cat/mbm/mba_test_cleanup(). > However, before running ksft_test_result(), > function cmt/cat/mbm/mba_test_cleanup() > has been run in each test function as follows: > cmt_resctrl_val() > cat_perf_miss_val() > mba_schemata_change() > mbm_bw_change() > > Remove duplicate codes that clear each test result file. > > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > --- > tools/testing/selftests/resctrl/resctrl_tests.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index df0d8d8526fc..8732cf736528 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span, > ksft_test_result(!res, "MBM: bw change\n"); > if ((get_vendor() == ARCH_INTEL) && res) > ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n"); > - mbm_test_cleanup(); > } > From what I can tell this still seem to suffer from the problem where the test files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup is now expected to be done in mbm_bw_change(). Note that: mbm_bw_change() { ... ret = resctrl_val(benchmark_cmd, ¶m); if (ret) return ret; /* Test results stored in file */ ret = check_results(span); if (ret) return ret; <== Return without cleaning test result file mbm_test_cleanup(); <== Test result file cleaned only when test passed. return 0; } > static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, > @@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, > sprintf(benchmark_cmd[1], "%d", span); > res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd); > ksft_test_result(!res, "MBA: schemata change\n"); > - mba_test_cleanup(); > } mba_schemata_change() has the same pattern as mbm_bw_change() so test result files may linger when test fails. > > static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) > @@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) > ksft_test_result(!res, "CMT: test\n"); > if ((get_vendor() == ARCH_INTEL) && res) > ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n"); > - cmt_test_cleanup(); > } Same pattern again. > > static void run_cat_test(int cpu_no, int no_of_bits) > @@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits) > > res = cat_perf_miss_val(cpu_no, no_of_bits, "L3"); > ksft_test_result(!res, "CAT: test\n"); > - cat_test_cleanup(); > } Patch 4 makes this work. Thanks for doing that. Reinette
Hi Shuah and Reinette, > On 11/1/2022 2:43 AM, Shaopeng Tan wrote: > > Before exiting each test function(run_cmt/cat/mbm/mba_test()), > > test results("ok","not ok") are printed by ksft_test_result() and then > > temporary result files are cleaned by function > > cmt/cat/mbm/mba_test_cleanup(). > > However, before running ksft_test_result(), function > > cmt/cat/mbm/mba_test_cleanup() has been run in each test function as > > follows: > > cmt_resctrl_val() > > cat_perf_miss_val() > > mba_schemata_change() > > mbm_bw_change() > > > > Remove duplicate codes that clear each test result file. > > This isn't making much sense to me. Please include test report before and after > this change in the change log. With or without this patch, there is no effect on the result message. These functions were executed twice, in brief, it runs as follows: - cmt/cat/mbm/mba_test_cleanup() - ksft_test_result() - cmt/cat/mbm/mba_test_cleanup() So, I deleted once. > From what I can tell this still seem to suffer from the problem where the test > files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup > is now expected to be done in mbm_bw_change(). > > Note that: > > mbm_bw_change() > { > ... > > ret = resctrl_val(benchmark_cmd, ¶m); > if (ret) > return ret; > > /* Test results stored in file */ > > ret = check_results(span); > if (ret) > return ret; <== Return without cleaning test result file > > mbm_test_cleanup(); <== Test result file cleaned only when test > passed. > > return 0; > } I intend to avoid this problem through the following codes. mbm_bw_change() { ret = resctrl_val(benchmark_cmd, ¶m); if (ret) - return ret; + goto out; ret = check_results(span); if (ret) - return ret; + goto out; +out: mbm_test_cleanup(); - return 0; + return ret; } Best regards, Shaopeng Tan
Hi Shaopeng, On 11/8/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote: > Hi Shuah and Reinette, > >> On 11/1/2022 2:43 AM, Shaopeng Tan wrote: >>> Before exiting each test function(run_cmt/cat/mbm/mba_test()), >>> test results("ok","not ok") are printed by ksft_test_result() and then >>> temporary result files are cleaned by function >>> cmt/cat/mbm/mba_test_cleanup(). >>> However, before running ksft_test_result(), function >>> cmt/cat/mbm/mba_test_cleanup() has been run in each test function as >>> follows: >>> cmt_resctrl_val() >>> cat_perf_miss_val() >>> mba_schemata_change() >>> mbm_bw_change() >>> >>> Remove duplicate codes that clear each test result file. >> >> This isn't making much sense to me. Please include test report before and after >> this change in the change log. > > With or without this patch, there is no effect on the result message. > These functions were executed twice, in brief, it runs as follows: > - cmt/cat/mbm/mba_test_cleanup() > - ksft_test_result() > - cmt/cat/mbm/mba_test_cleanup() > So, I deleted once. > >> From what I can tell this still seem to suffer from the problem where the test >> files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup >> is now expected to be done in mbm_bw_change(). >> >> Note that: >> >> mbm_bw_change() >> { >> ... >> >> ret = resctrl_val(benchmark_cmd, ¶m); >> if (ret) >> return ret; >> >> /* Test results stored in file */ >> >> ret = check_results(span); >> if (ret) >> return ret; <== Return without cleaning test result file >> >> mbm_test_cleanup(); <== Test result file cleaned only when test >> passed. >> >> return 0; >> } > > I intend to avoid this problem through the following codes. > > mbm_bw_change() > { > ret = resctrl_val(benchmark_cmd, ¶m); > if (ret) > - return ret; > + goto out; > > ret = check_results(span); > if (ret) > - return ret; > + goto out; > > +out: > mbm_test_cleanup(); > > - return 0; > + return ret; > } > Yes, even though file removal may now encounter ENOENT this does seem the most robust route and the possible error is ok since mbm_test_cleanup() does not check the return code. Could you please replicate this pattern to the other functions (mba_schemata_change() and cmt_resctrl_val()) also? Reinette
Hi Reinette, > On 11/8/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote: > > Hi Shuah and Reinette, > > > >> On 11/1/2022 2:43 AM, Shaopeng Tan wrote: > >>> Before exiting each test function(run_cmt/cat/mbm/mba_test()), > >>> test results("ok","not ok") are printed by ksft_test_result() and > >>> then temporary result files are cleaned by function > >>> cmt/cat/mbm/mba_test_cleanup(). > >>> However, before running ksft_test_result(), function > >>> cmt/cat/mbm/mba_test_cleanup() has been run in each test function as > >>> follows: > >>> cmt_resctrl_val() > >>> cat_perf_miss_val() > >>> mba_schemata_change() > >>> mbm_bw_change() > >>> > >>> Remove duplicate codes that clear each test result file. > >> > >> This isn't making much sense to me. Please include test report before > >> and after this change in the change log. > > > > With or without this patch, there is no effect on the result message. > > These functions were executed twice, in brief, it runs as follows: > > - cmt/cat/mbm/mba_test_cleanup() > > - ksft_test_result() > > - cmt/cat/mbm/mba_test_cleanup() > > So, I deleted once. > > > >> From what I can tell this still seem to suffer from the problem where > >> the test files may not be cleaned. With the removal of > >> mbm_test_cleanup() the cleanup is now expected to be done in > mbm_bw_change(). > >> > >> Note that: > >> > >> mbm_bw_change() > >> { > >> ... > >> > >> ret = resctrl_val(benchmark_cmd, ¶m); > >> if (ret) > >> return ret; > >> > >> /* Test results stored in file */ > >> > >> ret = check_results(span); > >> if (ret) > >> return ret; <== Return without cleaning test result file > >> > >> mbm_test_cleanup(); <== Test result file cleaned only when test > >> passed. > >> > >> return 0; > >> } > > > > I intend to avoid this problem through the following codes. > > > > mbm_bw_change() > > { > > ret = resctrl_val(benchmark_cmd, ¶m); > > if (ret) > > - return ret; > > + goto out; > > > > ret = check_results(span); > > if (ret) > > - return ret; > > + goto out; > > > > +out: > > mbm_test_cleanup(); > > > > - return 0; > > + return ret; > > } > > > > Yes, even though file removal may now encounter ENOENT this does seem the > most robust route and the possible error is ok since mbm_test_cleanup() does > not check the return code. > Could you please replicate this pattern to the other functions > (mba_schemata_change() and cmt_resctrl_val()) also? This is an example for MBM, I intended to replicate this pattern to other tests. Best regard, Shaopeng Tan
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index df0d8d8526fc..8732cf736528 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span, ksft_test_result(!res, "MBM: bw change\n"); if ((get_vendor() == ARCH_INTEL) && res) ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n"); - mbm_test_cleanup(); } static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, @@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, sprintf(benchmark_cmd[1], "%d", span); res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd); ksft_test_result(!res, "MBA: schemata change\n"); - mba_test_cleanup(); } static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) @@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) ksft_test_result(!res, "CMT: test\n"); if ((get_vendor() == ARCH_INTEL) && res) ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n"); - cmt_test_cleanup(); } static void run_cat_test(int cpu_no, int no_of_bits) @@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits) res = cat_perf_miss_val(cpu_no, no_of_bits, "L3"); ksft_test_result(!res, "CAT: test\n"); - cat_test_cleanup(); } int main(int argc, char **argv)
Before exiting each test function(run_cmt/cat/mbm/mba_test()), test results("ok","not ok") are printed by ksft_test_result() and then temporary result files are cleaned by function cmt/cat/mbm/mba_test_cleanup(). However, before running ksft_test_result(), function cmt/cat/mbm/mba_test_cleanup() has been run in each test function as follows: cmt_resctrl_val() cat_perf_miss_val() mba_schemata_change() mbm_bw_change() Remove duplicate codes that clear each test result file. Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> --- tools/testing/selftests/resctrl/resctrl_tests.c | 4 ---- 1 file changed, 4 deletions(-)