| Message ID | 20190829181146.20261-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | memcg_stress_test.sh: Respect LTP_TIMEOUT_MUL set by user | expand |
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.
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.
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. >
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
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.
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 >
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 >> >
> > > > 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.
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?
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 >
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
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
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.
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
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
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(-)