[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6d4eb508-f551-4c12-2e15-34ab9b1dc49f@intel.com>
Date: Wed, 1 Dec 2021 16:18:47 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: "tan.shaopeng@...itsu.com" <tan.shaopeng@...itsu.com>,
Fenghua Yu <fenghua.yu@...el.com>,
Shuah Khan <shuah@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using
kselftest framework
Hi Shaopeng Tan,
On 11/30/2021 6:36 PM, tan.shaopeng@...itsu.com wrote:
> Hi Reinette
>
>> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
>>> From: "Tan, Shaopeng" <tan.shaopeng@...fujitsu.com>
>>>
>>> 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.
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?
When logical changes are isolated into separate patches it really makes
the patches easier to understand.
>>> 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.
Reinette
Powered by blists - more mailing lists