RE: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

[Shaopeng Tan (Fujitsu)]

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