[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
>>
>