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 <[email protected]>
---
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();
}
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)
--
2.27.0
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 <[email protected]>
> ---
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 <[email protected]>
> ---
> 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