Hi Reinette, > 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). Thanks for your advice. I will separate the part about default limited time to a new patch. In order to get some opinions about change default limited time, I will add a description in the cover letter, when posting the next version of this patch. Regards, Shaopeng Tan
