Re: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Reinette Chatre]

Hi Shaopeng Tan,

On 12/2/2021 11:21 PM, tan.shaopeng@xxxxxxxxxxx wrote:
> > On 11/30/2021 6:36 PM, tan.shaopeng@xxxxxxxxxxx wrote:
> > > > On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> > > > > From: "Tan, Shaopeng" <tan.shaopeng@xxxxxxxxxxxxxx>

> > > > > To ensure the resctrl_tests finish in limited time, this commit
> > > > > changed the default limited time(45s) to 120 seconds for
> > > > > resctrl_tests by adding "setting" file.
> > > >
> > > > How is changing the timeout related to the resctrl framework changes?
> > > > Is it not a separate change?
> > >
> > > In selftest framwork, the default limited time of all tests is 45
> > > seconds which is specified by common file
> > tools/testing/selftests/kselftest/runner.sh.
> > > Each test can change the limited time individually by adding a "setting"
> > > file into its own directory. I changed the limited time of resctrl
> > > to120s because 45s was not enough in my environment.
> >
> > Understood. My question was if this can be a separate change with its own
> > patch? It seems to me that this deserves its own patch ... but actually it also
> > raises an important issue that the resctrl tests are taking a long time.
> >
> > I do see a rule for tests in Documentation/dev-tools/kselftest.rst:
> > "Don't take too long". This may be a motivation _not_ to include the resctrl tests
> > in the regular kselftest targets because when a user wants to run all tests on a
> > system it needs to be quick and the resctrl tests are not quick.
>
> I think 120s is not long, there are 6 tests with a limited time over 120s,
> for example, the limited time of net test is set 300s.

I am not familiar with the specific kselftest requirements in this 
regard but the test duration is surely something that needs to be kept 
in mind. Consider the systems performing integration testing on kernels 
everywhere - running the kselftest framework is a reasonable thing to do 
and test delays that may seem palatable on an individual run may not be 
appropriate for all test infrastructures.

Needing to almost triple the needed time from the default time is a red 
flag and really deserves to be in its own patch with a motivation. I 
would also recommend highlighting this change in the cover letter. This 
will bring the issue to the attention of the kselftest audience who will 
provide a better informed opinion (whether they want a long running test 
as part of the default framework or not).

Reinette