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