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

[tan.shaopeng@xxxxxxxxxxx]

Hi Reinette,
 
> On 11/30/2021 6:36 PM, tan.shaopeng@xxxxxxxxxxx wrote:
> > Hi Reinette
> >
> >> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> >>> From: "Tan, Shaopeng" <tan.shaopeng@xxxxxxxxxxxxxx>
> >>>
> >>> This commit enables kselftest to be built/run in resctrl framework.
> >>> Build/run resctrl_tests by build/run all tests of kselftest, or by
> >>> using the "TARGETS" variable on the make command line to specify
> >> resctrl_tests.
> >>> To make resctrl_tests run using kselftest framework, this commit
> >>> modified the Makefile of kernel kselftest set and the Makefile of
> >> resctrl_tests.
> >>
> >> The above sentence mentions that changes were made to the resctrl
> >> selftest Makefile but it does not describe what the change accomplish
> >> or why they are needed. Could you please elaborate?
> >
> > Before these changes of Makefile, when we run resctrl test, we need to
> > goto tools/testing/selftests/resctrl/ directory, run "make" to build
> > executable file "resctrl_tests", and run "sudo ./resctrl_tests" to
> > execute the test.
> >
> > With this patch, we can resctrl test in selftest framwork as follow.
> > Run all tests include resctrl:
> >   $ make -C tools/testing/selftests run_tests Run a subset(resctrl) of
> > selftests:
> >   $ make -C tools/testing/selftests TARGETS=resctrl run_tests
> >
> > Linux Kernel Selftests :
> > https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html
> 
> Understood and this is a reasonable change. What you wrote above would be a
> great addition to the changelog. I think it is also important to add to the
> changelog that the changes do not change the existing behavior and users can
> continue to build and run the tests as before.

Thanks for your advice.

> Also, please follow the guidance found in "Separate your changes" in
> Documentation/process/submitting-patches.rst: Logical changes should be
> in separate patches. This patch does too many things. Please consider:
> 1) the license addition is not relevant to this work
> 2) the new comment seems unnecessary as it states the obvious
> 3) where is the "LDLIBS += -lnuma" change coming from and why is it
> needed?

I'm sorry, I made a mistake.
"LDLIBs += -lnuma" is dependent on the following patch.
I will reorganize patch series to include the following patch and
separate Makefile changes appropriately.

https://lore.kernel.org/lkml/20211110082734.3184985-1-tan.shaopeng@xxxxxxxxxxxxxx/
[PATCH] selftests/resctrl: Skip MBM&CMT tests when Intel Sub-NUMA

> When logical changes are isolated into separate patches it really makes the
> patches easier to understand.

Thanks for your advice, I will separate patches.

> >>> 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. 


Regards,
Shaopeng Tan