[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Petr Vorel]

While it's good to increase the default LTP_TIMEOUT_MUL value, give user
a chance to change it.

Fixes: 877b445c9 ("memcg_stress_test.sh: ported to newlib")

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
index 5b19cc292..0d3ded112 100755
--- a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
+++ b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
@@ -17,7 +17,7 @@ TST_NEEDS_CMDS="mount umount cat kill mkdir rmdir grep awk cut"
 
 # Each test case runs for 900 secs when everything fine
 # therefore the default 5 mins timeout is not enough.
-LTP_TIMEOUT_MUL=7
+LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-7}
 
 . cgroup_lib.sh
 
-- 
2.22.1

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Li Wang]

On Fri, Aug 30, 2019 at 2:12 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> While it's good to increase the default LTP_TIMEOUT_MUL value, give user
> a chance to change it.

It's a good proposal, but one thing we need to consider that there is
possible to pass a small timeout value(<5mins) from the user. So what
about set a condition judgment which only accepts time value which >=
7?

>  # Each test case runs for 900 secs when everything fine
>  # therefore the default 5 mins timeout is not enough.

Here the code comments reminder this.

-- 
Regards,
Li Wang

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Petr Vorel]

Hi Li,

Good point. Something like this could do it:
-LTP_TIMEOUT_MUL=7
+min_timeout=7
+[ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout

Unless we test only integers:
+[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout

But that'd require using only integers, while C allows to use floating point
numbers :(. We can
1) either live with the limitation of integers for shell (+ document it)
2) or use awk or bc (but that's external dependency for shell tests (currently
tst_test.sh requires: cut, tr, wc; tst_net.sh requires awk and ip; so I'd be for
awk dependency; dependencies should be documented as well)
3) write simple utility (tst_float_cmp.c) to compare strings for us

Of course, we can test only integers:
+[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout

Also, C code requires LTP_TIMEOUT_MUL > 1 in tst_set_timeout().
We don't have this check. Again, adding it brings problem with float number.

Kind regards,
Petr

> On Fri, Aug 30, 2019 at 2:12 AM Petr Vorel <pvorel@suse.cz> wrote:

> > While it's good to increase the default LTP_TIMEOUT_MUL value, give user
> > a chance to change it.

> It's a good proposal, but one thing we need to consider that there is
> possible to pass a small timeout value(<5mins) from the user. So what
> about set a condition judgment which only accepts time value which >=
> 7?

> >  # Each test case runs for 900 secs when everything fine
> >  # therefore the default 5 mins timeout is not enough.

> Here the code comments reminder this.

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Cristian Marussi]

Hi

On 30/08/2019 09:50, Petr Vorel wrote:
> Hi Li,
> 
> Good point. Something like this could do it:
> -LTP_TIMEOUT_MUL=7
> +min_timeout=7
> +[ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout
> 
> Unless we test only integers:
> +[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout
> 

I would certainly introduce a check on the minimum allowed test-timeout and just stick to integers.
(is it really needed to worry for float multipliers ?)

I also wonder if it is worth somehow put this minimum-enforce mechanism inside the framework itself
instead that hardcoding it in this specific test (unless you already mean to do it this way...
and I misunderstood)

So that, roughly, in the test

LTP_TIMEOUT_MUL_MIN=7
LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-7}

and somewhere in framework test initialization you enforce it (maybe with a warning for the user when overriding its setup)

[ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $LTP_TIMEOUT_MUL_MIN ] && LTP_TIMEOUT_MUL=$LTP_TIMEOUT_MUL_MIN

(but my LTP framework memories are a bit blurred now...so feel free to ignore if it is not feasible or practical)

Thanks

Cristian

> But that'd require using only integers, while C allows to use floating point
> numbers :(. We can
> 1) either live with the limitation of integers for shell (+ document it)
> 2) or use awk or bc (but that's external dependency for shell tests (currently
> tst_test.sh requires: cut, tr, wc; tst_net.sh requires awk and ip; so I'd be for
> awk dependency; dependencies should be documented as well)
> 3) write simple utility (tst_float_cmp.c) to compare strings for us
> 
> Of course, we can test only integers:
> +[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout
> 
> Also, C code requires LTP_TIMEOUT_MUL > 1 in tst_set_timeout().
> We don't have this check. Again, adding it brings problem with float number.
> 
> Kind regards,
> Petr
> 
>> On Fri, Aug 30, 2019 at 2:12 AM Petr Vorel <pvorel@suse.cz> wrote:
> 
>>> While it's good to increase the default LTP_TIMEOUT_MUL value, give user
>>> a chance to change it.
> 
>> It's a good proposal, but one thing we need to consider that there is
>> possible to pass a small timeout value(<5mins) from the user. So what
>> about set a condition judgment which only accepts time value which >=
>> 7?
> 
>>>  # Each test case runs for 900 secs when everything fine
>>>  # therefore the default 5 mins timeout is not enough.
> 
>> Here the code comments reminder this.
> 

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Petr Vorel]

Hi Cristian,

> On 30/08/2019 09:50, Petr Vorel wrote:
> > Hi Li,

> > Good point. Something like this could do it:
> > -LTP_TIMEOUT_MUL=7
> > +min_timeout=7
> > +[ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout

> > Unless we test only integers:
> > +[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout


> I would certainly introduce a check on the minimum allowed test-timeout and just stick to integers.
> (is it really needed to worry for float multipliers ?)
Not sure, but it'd be good to have it same for C and for shell. Otherwise
working variable for C would fail on shell.

> I also wonder if it is worth somehow put this minimum-enforce mechanism inside the framework itself
> instead that hardcoding it in this specific test (unless you already mean to do it this way...
> and I misunderstood)
Yes, I was thinking about it as well.
LTP_TIMEOUT_MUL should be reserved for users, tests should use LTP_TIMEOUT_MUL_MIN,
check for LTP_TIMEOUT_MUL being higher than LTP_TIMEOUT_MUL_MIN would be in
_tst_setup_timer(). Similar mechanism I introduced in 9d6a960d9 (VIRT_PERF_THRESHOLD_MIN).

I wonder if it'd be useful to have some functionality in C (ltp_timeout_mul_min
as a struct tst_test, default 1).

> So that, roughly, in the test

> LTP_TIMEOUT_MUL_MIN=7
> LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-7}

> and somewhere in framework test initialization you enforce it (maybe with a warning for the user when overriding its setup)

> [ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $LTP_TIMEOUT_MUL_MIN ] && LTP_TIMEOUT_MUL=$LTP_TIMEOUT_MUL_MIN
+1. Feel free to send a patch.

> (but my LTP framework memories are a bit blurred now...so feel free to ignore if it is not feasible or practical)

> Thanks

> Cristian

Kind regards,
Petr

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Li Wang]

On Fri, Aug 30, 2019 at 6:46 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Cristian,
>
> > On 30/08/2019 09:50, Petr Vorel wrote:
> > > Hi Li,
>
> > > Good point. Something like this could do it:
> > > -LTP_TIMEOUT_MUL=7
> > > +min_timeout=7
> > > +[ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout
>
> > > Unless we test only integers:
> > > +[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout
>
>
> > I would certainly introduce a check on the minimum allowed test-timeout and just stick to integers.
> > (is it really needed to worry for float multipliers ?)

No, I guess the integer division in the shell/C is enough.

> Not sure, but it'd be good to have it same for C and for shell. Otherwise
> working variable for C would fail on shell.
>
> > I also wonder if it is worth somehow put this minimum-enforce mechanism inside the framework itself
> > instead that hardcoding it in this specific test (unless you already mean to do it this way...
> > and I misunderstood)
> Yes, I was thinking about it as well.
> LTP_TIMEOUT_MUL should be reserved for users, tests should use LTP_TIMEOUT_MUL_MIN,
> check for LTP_TIMEOUT_MUL being higher than LTP_TIMEOUT_MUL_MIN would be in
> _tst_setup_timer(). Similar mechanism I introduced in 9d6a960d9 (VIRT_PERF_THRESHOLD_MIN).

+1 agree.

>
> I wonder if it'd be useful to have some functionality in C (ltp_timeout_mul_min
> as a struct tst_test, default 1).

Yes. But seems no need to involve a new field in struct tst_test,
maybe we could complete that in the function tst_set_timeout(int
timeout).

>
> > So that, roughly, in the test
>
> > LTP_TIMEOUT_MUL_MIN=7
> > LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-7}
>
> > and somewhere in framework test initialization you enforce it (maybe with a warning for the user when overriding its setup)
>
> > [ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $LTP_TIMEOUT_MUL_MIN ] && LTP_TIMEOUT_MUL=$LTP_TIMEOUT_MUL_MIN
> +1. Feel free to send a patch.

Agree.

-- 
Regards,
Li Wang

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Clemens Famulla-Conrad]

On Mon, 2019-09-02 at 10:34 +0800, Li Wang wrote:
> On Fri, Aug 30, 2019 at 6:46 PM Petr Vorel <pvorel@suse.cz> wrote:
> > 
> > Hi Cristian,
> > 
> > > On 30/08/2019 09:50, Petr Vorel wrote:
> > > > Hi Li,
> > > > Good point. Something like this could do it:
> > > > -LTP_TIMEOUT_MUL=7
> > > > +min_timeout=7
> > > > +[ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout
> > > > ] && LTP_TIMEOUT_MUL=$min_timeout
> > > > Unless we test only integers:
> > > > +[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt
> > > > $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout
> > 
> > 
> > > I would certainly introduce a check on the minimum allowed test-
> > > timeout and just stick to integers.
> > > (is it really needed to worry for float multipliers ?)
> 
> No, I guess the integer division in the shell/C is enough.
> 
> > Not sure, but it'd be good to have it same for C and for shell.
> > Otherwise
> > working variable for C would fail on shell.
> > 
> > > I also wonder if it is worth somehow put this minimum-enforce
> > > mechanism inside the framework itself
> > > instead that hardcoding it in this specific test (unless you
> > > already mean to do it this way...
> > > and I misunderstood)
> > 
> > Yes, I was thinking about it as well.
> > LTP_TIMEOUT_MUL should be reserved for users, tests should use
> > LTP_TIMEOUT_MUL_MIN,
> > check for LTP_TIMEOUT_MUL being higher than LTP_TIMEOUT_MUL_MIN
> > would be in
> > _tst_setup_timer(). Similar mechanism I introduced in 9d6a960d9
> > (VIRT_PERF_THRESHOLD_MIN).
> 
> +1 agree.

I have a general question. What do we try to get with
LTP_TIMEOUT_MUL_MIN? From my perspective, we try to set a minimum
timeout value. Isn't it the value (struct tst_test*)->timeout ?

I'm missing such configuration value for shell. Is there one?

Or do we need to increase timeout in smaller steps and that is why we
need this LTP_TIMEOUT_MUL_MIN?

> 
> > 
> > I wonder if it'd be useful to have some functionality in C
> > (ltp_timeout_mul_min
> > as a struct tst_test, default 1).
> 
> Yes. But seems no need to involve a new field in struct tst_test,
> maybe we could complete that in the function tst_set_timeout(int
> timeout).
> 
> > 
> > > So that, roughly, in the test
> > > LTP_TIMEOUT_MUL_MIN=7
> > > LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-7}
> > > and somewhere in framework test initialization you enforce it
> > > (maybe with a warning for the user when overriding its setup)
> > > [ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt
> > > $LTP_TIMEOUT_MUL_MIN ] && LTP_TIMEOUT_MUL=$LTP_TIMEOUT_MUL_MIN
> > 
> > +1. Feel free to send a patch.
> 
> Agree.
> 
> -- 
> Regards,
> Li Wang
> 

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Cristian Marussi]

Hi Clemens

On 12/09/2019 10:04, Clemens Famulla-Conrad wrote:
> On Mon, 2019-09-02 at 10:34 +0800, Li Wang wrote:
>> On Fri, Aug 30, 2019 at 6:46 PM Petr Vorel <pvorel@suse.cz> wrote:
>>>
>>> Hi Cristian,
>>>
>>>> On 30/08/2019 09:50, Petr Vorel wrote:
>>>>> Hi Li,
>>>>> Good point. Something like this could do it:
>>>>> -LTP_TIMEOUT_MUL=7
>>>>> +min_timeout=7
>>>>> +[ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt $min_timeout
>>>>> ] && LTP_TIMEOUT_MUL=$min_timeout
>>>>> Unless we test only integers:
>>>>> +[ is_int "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt
>>>>> $min_timeout ] && LTP_TIMEOUT_MUL=$min_timeout
>>>
>>>
>>>> I would certainly introduce a check on the minimum allowed test-
>>>> timeout and just stick to integers.
>>>> (is it really needed to worry for float multipliers ?)
>>
>> No, I guess the integer division in the shell/C is enough.
>>
>>> Not sure, but it'd be good to have it same for C and for shell.
>>> Otherwise
>>> working variable for C would fail on shell.
>>>
>>>> I also wonder if it is worth somehow put this minimum-enforce
>>>> mechanism inside the framework itself
>>>> instead that hardcoding it in this specific test (unless you
>>>> already mean to do it this way...
>>>> and I misunderstood)
>>>
>>> Yes, I was thinking about it as well.
>>> LTP_TIMEOUT_MUL should be reserved for users, tests should use
>>> LTP_TIMEOUT_MUL_MIN,
>>> check for LTP_TIMEOUT_MUL being higher than LTP_TIMEOUT_MUL_MIN
>>> would be in
>>> _tst_setup_timer(). Similar mechanism I introduced in 9d6a960d9
>>> (VIRT_PERF_THRESHOLD_MIN).
>>
>> +1 agree.
> 
> I have a general question. What do we try to get with
> LTP_TIMEOUT_MUL_MIN? From my perspective, we try to set a minimum
> timeout value. Isn't it the value (struct tst_test*)->timeout ?
> 
> I'm missing such configuration value for shell. Is there one?
> 
> Or do we need to increase timeout in smaller steps and that is why we
> need this LTP_TIMEOUT_MUL_MIN?
> 

My understanding was that the idea was to allow the user to select its own
LTP_TIMEOUT_MUL at will, while enforcing a minimum per-test LTP_TIMEOUT_MUL_MIN
where needed, since, as an example for this test, reducing LTP_TIMEOUT_MUL < 7
will cause it to fail straight away

Cristian

>>>
>>> I wonder if it'd be useful to have some functionality in C
>>> (ltp_timeout_mul_min
>>> as a struct tst_test, default 1).
>>
>> Yes. But seems no need to involve a new field in struct tst_test,
>> maybe we could complete that in the function tst_set_timeout(int
>> timeout).
>>
>>>
>>>> So that, roughly, in the test
>>>> LTP_TIMEOUT_MUL_MIN=7
>>>> LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-7}
>>>> and somewhere in framework test initialization you enforce it
>>>> (maybe with a warning for the user when overriding its setup)
>>>> [ -z "$LTP_TIMEOUT_MUL" -o "$LTP_TIMEOUT_MUL" -lt
>>>> $LTP_TIMEOUT_MUL_MIN ] && LTP_TIMEOUT_MUL=$LTP_TIMEOUT_MUL_MIN
>>>
>>> +1. Feel free to send a patch.
>>
>> Agree.
>>
>> -- 
>> Regards,
>> Li Wang
>>
> 

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Li Wang]

> > > > I also wonder if it is worth somehow put this minimum-enforce
> > > > mechanism inside the framework itself
> > > > instead that hardcoding it in this specific test (unless you
> > > > already mean to do it this way...
> > > > and I misunderstood)
> > >
> > > Yes, I was thinking about it as well.
> > > LTP_TIMEOUT_MUL should be reserved for users, tests should use
> > > LTP_TIMEOUT_MUL_MIN,
> > > check for LTP_TIMEOUT_MUL being higher than LTP_TIMEOUT_MUL_MIN
> > > would be in
> > > _tst_setup_timer(). Similar mechanism I introduced in 9d6a960d9
> > > (VIRT_PERF_THRESHOLD_MIN).
> >
> > +1 agree.
>
> I have a general question. What do we try to get with
> LTP_TIMEOUT_MUL_MIN? From my perspective, we try to set a minimum
> timeout value. Isn't it the value (struct tst_test*)->timeout ?
>

Well, the (struct tst_test*)->timeout is the default minimum value to set a
timeout, but for some test case(e.g memcg_stress_test.sh), they required
time should be higher than the default. So as we discussed in the above
mails, we're planning to introduce a new variable LTP_TIMEOUT_MUL_MIN to
set as a new minimum value for test timeout. The operation will be
encapsulate in function  _tst_setup_timer().



>
> I'm missing such configuration value for shell. Is there one?
>

No, we don't have it so far.


>
> Or do we need to increase timeout in smaller steps and that is why we
> need this LTP_TIMEOUT_MUL_MIN?
>

Hmm, what we want to do is:

If a testcase needs timeout value is larger than the default (300 sec), we
could only define a variable LTP_TIMEOUT_MUL_MIN in the test, then the
_tst_setup_timer() will detect if LTP_TIMEOUT_MUL_MIN is valid and reset
the minimum time for the test.

@Petr and @Cristian, If I misunderstand anything, please correct me.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190912/37dfd516/attachment.htm>

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Clemens Famulla-Conrad]

On Thu, 2019-09-12 at 17:34 +0800, Li Wang wrote:
> > > > > I also wonder if it is worth somehow put this minimum-enforce
> > > > > mechanism inside the framework itself
> > > > > instead that hardcoding it in this specific test (unless you
> > > > > already mean to do it this way...
> > > > > and I misunderstood)
> > > > 
> > > > Yes, I was thinking about it as well.
> > > > LTP_TIMEOUT_MUL should be reserved for users, tests should use
> > > > LTP_TIMEOUT_MUL_MIN,
> > > > check for LTP_TIMEOUT_MUL being higher than LTP_TIMEOUT_MUL_MIN
> > > > would be in
> > > > _tst_setup_timer(). Similar mechanism I introduced in 9d6a960d9
> > > > (VIRT_PERF_THRESHOLD_MIN).
> > > 
> > > +1 agree.
> > 
> > I have a general question. What do we try to get with
> > LTP_TIMEOUT_MUL_MIN? From my perspective, we try to set a minimum
> > timeout value. Isn't it the value (struct tst_test*)->timeout ?
> > 
> 
> Well, the (struct tst_test*)->timeout is the default minimum value to
> set a
> timeout, but for some test case(e.g memcg_stress_test.sh), they
> required
> time should be higher than the default. So as we discussed in the
> above
> mails, we're planning to introduce a new variable LTP_TIMEOUT_MUL_MIN
> to
> set as a new minimum value for test timeout. The operation will be
> encapsulate in function  _tst_setup_timer().
> 
> 
> 
> > 
> > I'm missing such configuration value for shell. Is there one?
> > 
> 
> No, we don't have it so far.
> 
> 
> > 
> > Or do we need to increase timeout in smaller steps and that is why
> > we
> > need this LTP_TIMEOUT_MUL_MIN?
> > 
> 
> Hmm, what we want to do is:
> 
> If a testcase needs timeout value is larger than the default (300
> sec), we
> could only define a variable LTP_TIMEOUT_MUL_MIN in the test, then
> the
> _tst_setup_timer() will detect if LTP_TIMEOUT_MUL_MIN is valid and
> reset
> the minimum time for the test.
> 
> @Petr and @Cristian, If I misunderstand anything, please correct me.

So from what I understood now, we need to specify a minimum timeout and
not a minimum timeout multiplier.
And we already have it for c, but only miss it in shell, or?

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Cristian Marussi]

Hi

On 12/09/2019 10:34, Li Wang wrote:
>>>>> I also wonder if it is worth somehow put this minimum-enforce
>>>>> mechanism inside the framework itself
>>>>> instead that hardcoding it in this specific test (unless you
>>>>> already mean to do it this way...
>>>>> and I misunderstood)
>>>>
>>>> Yes, I was thinking about it as well.
>>>> LTP_TIMEOUT_MUL should be reserved for users, tests should use
>>>> LTP_TIMEOUT_MUL_MIN,
>>>> check for LTP_TIMEOUT_MUL being higher than LTP_TIMEOUT_MUL_MIN
>>>> would be in
>>>> _tst_setup_timer(). Similar mechanism I introduced in 9d6a960d9
>>>> (VIRT_PERF_THRESHOLD_MIN).
>>>
>>> +1 agree.
>>
>> I have a general question. What do we try to get with
>> LTP_TIMEOUT_MUL_MIN? From my perspective, we try to set a minimum
>> timeout value. Isn't it the value (struct tst_test*)->timeout ?
>>
> 
> Well, the (struct tst_test*)->timeout is the default minimum value to set a
> timeout, but for some test case(e.g memcg_stress_test.sh), they required
> time should be higher than the default. So as we discussed in the above
> mails, we're planning to introduce a new variable LTP_TIMEOUT_MUL_MIN to
> set as a new minimum value for test timeout. The operation will be
> encapsulate in function  _tst_setup_timer().
> 
> 
> 
>>
>> I'm missing such configuration value for shell. Is there one?
>>
> 
> No, we don't have it so far.
> 
> 
>>
>> Or do we need to increase timeout in smaller steps and that is why we
>> need this LTP_TIMEOUT_MUL_MIN?
>>
> 
> Hmm, what we want to do is:
> 
> If a testcase needs timeout value is larger than the default (300 sec), we
> could only define a variable LTP_TIMEOUT_MUL_MIN in the test, then the
> _tst_setup_timer() will detect if LTP_TIMEOUT_MUL_MIN is valid and reset
> the minimum time for the test.
> 
> @Petr and @Cristian, If I misunderstand anything, please correct me.

my understanding was that:

- we should already be able to set a non default per-test timeout using
  the existing global LTP_TIMEOUT_MUL (and we are)

- in this test we hardcoded such LTP_TIMEOUT_MUL to 7 because is the minimum sane
  value for this test (less than 7 and it fails 100%)

- we want to allow again the user to specify its own LTP_TIMEOUT_MUL if he wants
  BUT also being able to enforce on a test by test basis a MINIMUM allowed value:
  so we would define LTP_TIMEOUT_MUL_MIN=7 here, and then a user would be free to 
  run LTP with a different global LTP_TIMEOUT_MUL but when running this test
  
  + if LTP_TIMEOUT_MUL < LTP_TIMEOUT_MUL_MIN ===> use local LTP_TIMEOUT_MUL_MIN
  + if LTP_TIMEOUT_MUL >= LTP_TIMEOUT_MUL_MIN  ===> use global LTP_TIMEOUT_MUL

This way you don't break specific tests' needs while allowing the user to global reduce
run-time....now basically the user cannot enforce a higher timeout on this test
using the global LTP_TIMEOUT_MUL even if it should be allowed to since this wouldn't
break the test.

...unless I misunderstood too o_O :D


Thanks

Cristian





> 

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Clemens Famulla-Conrad]

On Thu, 2019-09-12 at 10:55 +0100, Cristian Marussi wrote:
> Hi
> 
> > Hmm, what we want to do is:
> > 
> > If a testcase needs timeout value is larger than the default (300
> > sec), we
> > could only define a variable LTP_TIMEOUT_MUL_MIN in the test, then
> > the
> > _tst_setup_timer() will detect if LTP_TIMEOUT_MUL_MIN is valid and
> > reset
> > the minimum time for the test.
> > 
> > @Petr and @Cristian, If I misunderstand anything, please correct
> > me.
> 
> my understanding was that:
> 
> - we should already be able to set a non default per-test timeout
> using
>   the existing global LTP_TIMEOUT_MUL (and we are)
> 
> - in this test we hardcoded such LTP_TIMEOUT_MUL to 7 because is the
> minimum sane
>   value for this test (less than 7 and it fails 100%)
> 
> - we want to allow again the user to specify its own LTP_TIMEOUT_MUL
> if he wants
>   BUT also being able to enforce on a test by test basis a MINIMUM
> allowed value:
>   so we would define LTP_TIMEOUT_MUL_MIN=7 here, and then a user
> would be free to 
>   run LTP with a different global LTP_TIMEOUT_MUL but when running
> this test
>   
>   + if LTP_TIMEOUT_MUL < LTP_TIMEOUT_MUL_MIN ===> use local
> LTP_TIMEOUT_MUL_MIN
>   + if LTP_TIMEOUT_MUL >= LTP_TIMEOUT_MUL_MIN  ===> use global
> LTP_TIMEOUT_MUL
> 
> This way you don't break specific tests' needs while allowing the
> user to global reduce
> run-time....now basically the user cannot enforce a higher timeout on
> this test
> using the global LTP_TIMEOUT_MUL even if it should be allowed to
> since this wouldn't
> break the test.
> 
> ...unless I misunderstood too o_O :D

Thanks for explaining. That's how I understood the idea of
LTP_TIMEOUT_MUL_MIN, too.

But what I understood from current "c" approach is:
We have a fixed (minimal) timeout value, specified in (struct
tst_test*)->timeout, which can be adjusted by user with environment
variable TST_TIMEOUT_MUL.
This behavior is missing in shell.

And if we now introduce a LTP_TIMEOUT_MUL_MIN, it doesn't make much
sense, cause we have already a timeout min. So I think, we only need
something to specify the default minimum timeout in seconds for shell
(like we already do in c) and we are done.

Thanks
Clemens

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Petr Vorel]

Hi,

> > @Petr and @Cristian, If I misunderstand anything, please correct me.

> my understanding was that:

> - we should already be able to set a non default per-test timeout using
>   the existing global LTP_TIMEOUT_MUL (and we are)

> - in this test we hardcoded such LTP_TIMEOUT_MUL to 7 because is the minimum sane
>   value for this test (less than 7 and it fails 100%)

> - we want to allow again the user to specify its own LTP_TIMEOUT_MUL if he wants
>   BUT also being able to enforce on a test by test basis a MINIMUM allowed value:
>   so we would define LTP_TIMEOUT_MUL_MIN=7 here, and then a user would be free to 
>   run LTP with a different global LTP_TIMEOUT_MUL but when running this test

>   + if LTP_TIMEOUT_MUL < LTP_TIMEOUT_MUL_MIN ===> use local LTP_TIMEOUT_MUL_MIN
>   + if LTP_TIMEOUT_MUL >= LTP_TIMEOUT_MUL_MIN  ===> use global LTP_TIMEOUT_MUL

LTP_TIMEOUT_MUL is only for user, LTP_TIMEOUT_MUL_MIN is only for library.
It's similar to way which is used in virt_lib.sh (VIRT_PERF_THRESHOLD_MIN).
See
https://patchwork.ozlabs.org/patch/1155460/

I'll probably sent this patch today although so you can base the work on it.
Is that ok?

Kind regards,
Petr

> This way you don't break specific tests' needs while allowing the user to global reduce
> run-time....now basically the user cannot enforce a higher timeout on this test
> using the global LTP_TIMEOUT_MUL even if it should be allowed to since this wouldn't
> break the test.

> ...unless I misunderstood too o_O :D

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Clemens Famulla-Conrad]

On Thu, 2019-09-12 at 17:28 +0200, Petr Vorel wrote:
> Hi,
> 
> LTP_TIMEOUT_MUL is only for user, LTP_TIMEOUT_MUL_MIN is only for
> library.
> It's similar to way which is used in virt_lib.sh
> (VIRT_PERF_THRESHOLD_MIN).

Agree that LTP_TIMEOUT_MUL is for the user and the initial timeout
value comes from library.
I only say, that a LTP_TIMEOUT_MUL_MIN isn't needed from my
perspective, if we allow to set a absolute timeout value like
TST_TIMEOUT (as we already do in c). Because it has the same effect,
setting a minimum timeout value which the user cannot reduce.


> See
> https://patchwork.ozlabs.org/patch/1155460/
> 
> I'll probably sent this patch today although so you can base the work
> on it.
> Is that ok?
sure it is.

[LTP] [PATCH] memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user

[Petr Vorel]

Hi Clements,

> On Thu, 2019-09-12 at 17:28 +0200, Petr Vorel wrote:
> > Hi,

> > LTP_TIMEOUT_MUL is only for user, LTP_TIMEOUT_MUL_MIN is only for
> > library.
> > It's similar to way which is used in virt_lib.sh
> > (VIRT_PERF_THRESHOLD_MIN).

> Agree that LTP_TIMEOUT_MUL is for the user and the initial timeout
> value comes from library.
> I only say, that a LTP_TIMEOUT_MUL_MIN isn't needed from my
> perspective, if we allow to set a absolute timeout value like
> TST_TIMEOUT (as we already do in c). Because it has the same effect,
> setting a minimum timeout value which the user cannot reduce.
OK, replace LTP_TIMEOUT_MUL_MIN with TST_TIMEOUT set by test, make sense.
Thanks for a hint :).


> > See
> > https://patchwork.ozlabs.org/patch/1155460/

> > I'll probably sent this patch today although so you can base the work
> > on it.
> > Is that ok?
> sure it is.
:)

Kind regards,
Petr