diff mbox series

[v2,4/5] selftests/resctrl: Change default limited time to 120 seconds for resctrl_tests

Message ID 20211213100154.180599-5-tan.shaopeng@jp.fujitsu.com (mailing list archive)
State New
Headers show
Series selftests/resctrl: Add resctrl_tests into kselftest set | expand

Commit Message

Shaopeng Tan Dec. 13, 2021, 10:01 a.m. UTC
This commit changed the default limited time(45s) to 120 seconds for
resctrl_tests by adding "setting" file.  Since 45 seconds may not be
enough on some machine environments.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/settings | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 tools/testing/selftests/resctrl/settings

Comments

Reinette Chatre Jan. 6, 2022, 11:49 p.m. UTC | #1
Hi Shaopeng Tan,

On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> This commit changed the default limited time(45s) to 120 seconds for

(This commit)

> resctrl_tests by adding "setting" file.  Since 45 seconds may not be
> enough on some machine environments.

This sounds very imprecise when you say "may not be enough". From what I
understand you have already encountered systems where this is not enough
so you have established that 45 seconds is too short. The next question is,
how was 120 seconds determined and how do you know it will be enough?

> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/settings | 1 +
>  1 file changed, 1 insertion(+)
>  create mode 100644 tools/testing/selftests/resctrl/settings
> 
> diff --git a/tools/testing/selftests/resctrl/settings b/tools/testing/selftests/resctrl/settings
> new file mode 100644
> index 000000000000..6091b45d226b
> --- /dev/null
> +++ b/tools/testing/selftests/resctrl/settings
> @@ -0,0 +1 @@
> +timeout=120

Reinette
Shaopeng Tan (Fujitsu) Jan. 21, 2022, 7:59 a.m. UTC | #2
Hi Reinette,

> On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> > This commit changed the default limited time(45s) to 120 seconds for
> 
> (This commit)
> 
> > resctrl_tests by adding "setting" file.  Since 45 seconds may not be
> > enough on some machine environments.
> 
> This sounds very imprecise when you say "may not be enough". From what I
> understand you have already encountered systems where this is not enough so
> you have established that 45 seconds is too short. The next question is, how
> was 120 seconds determined and how do you know it will be enough?

It took about 68 seconds in my environment (Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz). 
This test accesses a fixed size(250M) memory by default settings 
which is used when run in kselftest framework. 
I think that the execution time does not change significantly in different environments.
So, I roughly doubled the execution time and get 120s, which I set to the limited time. 
I think it is enough for any environments.
If 120s are also not enough, user can freely set the limited time by "setting" file.

Thanks for your feedback of this patch series,
I will correct them in a few days.

> > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > ---
> >  tools/testing/selftests/resctrl/settings | 1 +
> >  1 file changed, 1 insertion(+)
> >  create mode 100644 tools/testing/selftests/resctrl/settings
> >
> > diff --git a/tools/testing/selftests/resctrl/settings
> > b/tools/testing/selftests/resctrl/settings
> > new file mode 100644
> > index 000000000000..6091b45d226b
> > --- /dev/null
> > +++ b/tools/testing/selftests/resctrl/settings
> > @@ -0,0 +1 @@
> > +timeout=120

Best regards,
Tan Shaopeng
Reinette Chatre Jan. 21, 2022, 6:06 p.m. UTC | #3
Hi Tan Shaopeng,

On 1/20/2022 11:59 PM, tan.shaopeng@fujitsu.com wrote:
> Hi Reinette,
> 
>> On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
>>> This commit changed the default limited time(45s) to 120 seconds for
>>
>> (This commit)
>>
>>> resctrl_tests by adding "setting" file.  Since 45 seconds may not be
>>> enough on some machine environments.
>>
>> This sounds very imprecise when you say "may not be enough". From what I
>> understand you have already encountered systems where this is not enough so
>> you have established that 45 seconds is too short. The next question is, how
>> was 120 seconds determined and how do you know it will be enough?
> 
> It took about 68 seconds in my environment (Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz). 
> This test accesses a fixed size(250M) memory by default settings 
> which is used when run in kselftest framework. 
> I think that the execution time does not change significantly in different environments.
> So, I roughly doubled the execution time and get 120s, which I set to the limited time. 
> I think it is enough for any environments.
> If 120s are also not enough, user can freely set the limited time by "setting" file.
> 

How about something like:

When testing on a Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz the resctrl selftests fail
due to timeout after exceeding the default time limit of 45 seconds. On this system
the test takes about 68 seconds. Since the failing test by default accesses a fixed
size of memory the execution time should not vary significantly between different
environment. A new default of 120 seconds should be sufficient yet easy to customize
with the introduction of the "settings" file for reference. 

Reinette
Shaopeng Tan (Fujitsu) Jan. 24, 2022, 8:07 a.m. UTC | #4
Hi Reinette,

> On 1/20/2022 11:59 PM, tan.shaopeng@fujitsu.com wrote:
> > Hi Reinette,
> >
> >> On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> >>> This commit changed the default limited time(45s) to 120 seconds for
> >>
> >> (This commit)
> >>
> >>> resctrl_tests by adding "setting" file.  Since 45 seconds may not be
> >>> enough on some machine environments.
> >>
> >> This sounds very imprecise when you say "may not be enough". From
> >> what I understand you have already encountered systems where this is
> >> not enough so you have established that 45 seconds is too short. The
> >> next question is, how was 120 seconds determined and how do you know it
> will be enough?
> >
> > It took about 68 seconds in my environment (Intel(R) Xeon(R) Gold 6254 CPU
> @ 3.10GHz).
> > This test accesses a fixed size(250M) memory by default settings which
> > is used when run in kselftest framework.
> > I think that the execution time does not change significantly in different
> environments.
> > So, I roughly doubled the execution time and get 120s, which I set to the
> limited time.
> > I think it is enough for any environments.
> > If 120s are also not enough, user can freely set the limited time by "setting"
> file.
> >
> 
> How about something like:
> 
> When testing on a Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz the resctrl
> selftests fail
> due to timeout after exceeding the default time limit of 45 seconds. On this
> system
> the test takes about 68 seconds. Since the failing test by default accesses a
> fixed
> size of memory the execution time should not vary significantly between
> different
> environment. A new default of 120 seconds should be sufficient yet easy to
> customize
> with the introduction of the "settings" file for reference.

Thanks for your feedback. 
I will use it for commit log in next version.

Best regards,
Tan Shaopeng
diff mbox series

Patch

diff --git a/tools/testing/selftests/resctrl/settings b/tools/testing/selftests/resctrl/settings
new file mode 100644
index 000000000000..6091b45d226b
--- /dev/null
+++ b/tools/testing/selftests/resctrl/settings
@@ -0,0 +1 @@ 
+timeout=120