| Message ID | 20220207040447.2803113-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | getrusage03: skip on small RAM system | expand |
Hi Li, > Notes: > Ps. I also think we might need .min_mem_[avai|total] field but > not sure if it is really necessary to add that. Is it just a single test? Than maybe not worth of it, but generally I'm for adding tags like this into the library - it encapsulates code and gives us a documentation in docparse. ... > +static void setup(void) > +{ > + long long mem_avail = tst_available_mem(); > + > + if (mem_avail < 512L*1024) > + tst_brk(TCONF, "Needed > 512MB availabe, only have: %ld kB", mem_avail); typo: s/availabe/available/ Reviewed-by: Petr Vorel <pvorel@suse.cz> Thanks! Kind regards, Petr
On Mon, Feb 7, 2022 at 5:05 AM Li Wang <liwang@redhat.com> wrote: > > It is easy to get VmSwap increase with a small system, here is > run with a 1G RAM kvm guest and TBROK: > > 7 tst_test.c:1365: TINFO: Timeout per run is 0h 05m 00s > 8 getrusage03.c:43: TPASS: initial.self ~= child.self > 9 getrusage03.c:57: TPASS: initial.children ~= 100MB > 10 getrusage03.c:66: TPASS: child.children == 0 > 11 getrusage03.c:84: TPASS: child.children ~= 300MB > 12 getrusage03.c:104: TPASS: initial.children ~= pre_wait.children > 13 getrusage03.c:112: TPASS: post_wait.children ~= 400MB > 14 getrusage03.h:25: TBROK: VmSwap is not zero > 15 getrusage03.c:133: TPASS: initial.children ~= after_zombie.children > 16 getrusage03.h:25: TBROK: VmSwap is not zero > 17 getrusage03_child.c:57: TPASS: initial.self ~= exec.self > 18 getrusage03_child.c:62: TPASS: initial.children ~= exec.children > > Signed-off-by: Li Wang <liwang@redhat.com> Acked-by: Jan Stancek <jstancek@redhat.com> This should fix the immediate failure, just wondering, would there be any downside of replacing that TBROK with TCONF? > --- > > Notes: > Ps. I also think we might need .min_mem_[avai|total] field but > not sure if it is really necessary to add that. > > testcases/kernel/syscalls/getrusage/getrusage03.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/testcases/kernel/syscalls/getrusage/getrusage03.c b/testcases/kernel/syscalls/getrusage/getrusage03.c > index bf5127483..5aa0b2326 100644 > --- a/testcases/kernel/syscalls/getrusage/getrusage03.c > +++ b/testcases/kernel/syscalls/getrusage/getrusage03.c > @@ -173,6 +173,14 @@ static void run(unsigned int i) > } > } > > +static void setup(void) > +{ > + long long mem_avail = tst_available_mem(); > + > + if (mem_avail < 512L*1024) > + tst_brk(TCONF, "Needed > 512MB availabe, only have: %ld kB", mem_avail); > +} > + > static struct tst_test test = { > .forks_child = 1, > .child_needs_reinit = 1, > @@ -182,6 +190,7 @@ static struct tst_test test = { > {"linux-git", "1f10206cf8e9"}, > {} > }, > + .setup = setup, > .test = run, > .tcnt = ARRAY_SIZE(testfunc_list), > }; > -- > 2.31.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp >
On Mon, Feb 7, 2022 at 3:41 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > Notes: > > Ps. I also think we might need .min_mem_[avai|total] field but > > not sure if it is really necessary to add that. > Is it just a single test? Than maybe not worth of it, but generally I'm for > By now I only see sporadic tests have this requirement. (hugeshmat04.c, swapping01.c, getrusage03.c) > adding tags like this into the library - it encapsulates code and gives us > a > documentation in docparse. > Agree, maybe just holding as a future plan for adding that. > ... > > +static void setup(void) > > +{ > > + long long mem_avail = tst_available_mem(); > > + > > + if (mem_avail < 512L*1024) > > + tst_brk(TCONF, "Needed > 512MB availabe, only have: %ld > kB", mem_avail); > typo: s/availabe/available/ > Thanks. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Thanks! > > Kind regards, > Petr > >
Hi Li, > On Mon, Feb 7, 2022 at 3:41 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, > > > Notes: > > > Ps. I also think we might need .min_mem_[avai|total] field but > > > not sure if it is really necessary to add that. > > Is it just a single test? Than maybe not worth of it, but generally I'm for > By now I only see sporadic tests have this requirement. > (hugeshmat04.c, swapping01.c, getrusage03.c) > > adding tags like this into the library - it encapsulates code and gives us > > a > > documentation in docparse. > Agree, maybe just holding as a future plan for adding that. IMHO more than two are enough to put it into library, but others might think differently. Also sure, feel free to merge this fix, library support can be done any time later. Kind regards, Petr
On Mon, Feb 7, 2022 at 5:16 PM Jan Stancek <jstancek@redhat.com> wrote: > On Mon, Feb 7, 2022 at 5:05 AM Li Wang <liwang@redhat.com> wrote: > > > > It is easy to get VmSwap increase with a small system, here is > > run with a 1G RAM kvm guest and TBROK: > > > > 7 tst_test.c:1365: TINFO: Timeout per run is 0h 05m 00s > > 8 getrusage03.c:43: TPASS: initial.self ~= child.self > > 9 getrusage03.c:57: TPASS: initial.children ~= 100MB > > 10 getrusage03.c:66: TPASS: child.children == 0 > > 11 getrusage03.c:84: TPASS: child.children ~= 300MB > > 12 getrusage03.c:104: TPASS: initial.children ~= pre_wait.children > > 13 getrusage03.c:112: TPASS: post_wait.children ~= 400MB > > 14 getrusage03.h:25: TBROK: VmSwap is not zero > > 15 getrusage03.c:133: TPASS: initial.children ~= > after_zombie.children > > 16 getrusage03.h:25: TBROK: VmSwap is not zero > > 17 getrusage03_child.c:57: TPASS: initial.self ~= exec.self > > 18 getrusage03_child.c:62: TPASS: initial.children ~= exec.children > > > > Signed-off-by: Li Wang <liwang@redhat.com> > > Acked-by: Jan Stancek <jstancek@redhat.com> > > This should fix the immediate failure, just wondering, > would there be any downside of replacing that TBROK with TCONF? > I'm not quite sure. Seems it trying to test without swap happened to guarantee less disturbing for the ’->ru_maxrss‘ counting. TCONF looks a bit weak to attention if the regression occurs ATM.
Hi! > > Signed-off-by: Li Wang <liwang@redhat.com> > > Acked-by: Jan Stancek <jstancek@redhat.com> > > This should fix the immediate failure, just wondering, > would there be any downside of replacing that TBROK with TCONF? I guess that we may end up in a state where the test would TCONF on every run and it would end up being ingnored. Can't we just change the consume_mb to lock the memory with mlock() in the consume_mb() instead? As long as we add the check for a free memory it should be fine I guess.
Hi! > --- > > Notes: > Ps. I also think we might need .min_mem_[avai|total] field but > not sure if it is really necessary to add that. Actually this would be very useful from the long term perspective. If we ever manage to run tests in parallel the scheduller should make sure that we do not hit OOM due to running more than one processe that consume significant amount of memory at the same time. > diff --git a/testcases/kernel/syscalls/getrusage/getrusage03.c b/testcases/kernel/syscalls/getrusage/getrusage03.c > index bf5127483..5aa0b2326 100644 > --- a/testcases/kernel/syscalls/getrusage/getrusage03.c > +++ b/testcases/kernel/syscalls/getrusage/getrusage03.c > @@ -173,6 +173,14 @@ static void run(unsigned int i) > } > } > > +static void setup(void) > +{ > + long long mem_avail = tst_available_mem(); > + > + if (mem_avail < 512L*1024) > + tst_brk(TCONF, "Needed > 512MB availabe, only have: %ld kB", mem_avail); > +} > + > static struct tst_test test = { > .forks_child = 1, > .child_needs_reinit = 1, > @@ -182,6 +190,7 @@ static struct tst_test test = { > {"linux-git", "1f10206cf8e9"}, > {} > }, > + .setup = setup, > .test = run, > .tcnt = ARRAY_SIZE(testfunc_list), > }; Looks good: Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Cyril Hrubis <chrubis@suse.cz> wrote: > > Notes: > > Ps. I also think we might need .min_mem_[avai|total] field but > > not sure if it is really necessary to add that. > > Actually this would be very useful from the long term perspective. If we > ever manage to run tests in parallel the scheduller should make sure > that we do not hit OOM due to running more than one processe that > consume significant amount of memory at the same time. > Well, sounds useful to avoid OOM in the parallel run. Hence the .min_mem should gives the requirement of minimal MemAvailable(but not MemTotal) size on test system. I will view the whole test to add that when I get a chance. > Looks good: > > Reviewed-by: Cyril Hrubis <chrubis@suse.cz> > Pushed (with the typo fix).
On Mon, Feb 7, 2022 at 9:14 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > Signed-off-by: Li Wang <liwang@redhat.com> > > > > Acked-by: Jan Stancek <jstancek@redhat.com> > > > > This should fix the immediate failure, just wondering, > > would there be any downside of replacing that TBROK with TCONF? > > I guess that we may end up in a state where the test would TCONF on > every run and it would end up being ingnored. > > Can't we just change the consume_mb to lock the memory with mlock() in > the consume_mb() instead? As long as we add the check for a free memory > it should be fine I guess. > Yes, we can, mlock() should be helpful on that. I prefer to do that in a separate patch unless Jan has a different perspective.
diff --git a/testcases/kernel/syscalls/getrusage/getrusage03.c b/testcases/kernel/syscalls/getrusage/getrusage03.c index bf5127483..5aa0b2326 100644 --- a/testcases/kernel/syscalls/getrusage/getrusage03.c +++ b/testcases/kernel/syscalls/getrusage/getrusage03.c @@ -173,6 +173,14 @@ static void run(unsigned int i) } } +static void setup(void) +{ + long long mem_avail = tst_available_mem(); + + if (mem_avail < 512L*1024) + tst_brk(TCONF, "Needed > 512MB availabe, only have: %ld kB", mem_avail); +} + static struct tst_test test = { .forks_child = 1, .child_needs_reinit = 1, @@ -182,6 +190,7 @@ static struct tst_test test = { {"linux-git", "1f10206cf8e9"}, {} }, + .setup = setup, .test = run, .tcnt = ARRAY_SIZE(testfunc_list), };
It is easy to get VmSwap increase with a small system, here is run with a 1G RAM kvm guest and TBROK: 7 tst_test.c:1365: TINFO: Timeout per run is 0h 05m 00s 8 getrusage03.c:43: TPASS: initial.self ~= child.self 9 getrusage03.c:57: TPASS: initial.children ~= 100MB 10 getrusage03.c:66: TPASS: child.children == 0 11 getrusage03.c:84: TPASS: child.children ~= 300MB 12 getrusage03.c:104: TPASS: initial.children ~= pre_wait.children 13 getrusage03.c:112: TPASS: post_wait.children ~= 400MB 14 getrusage03.h:25: TBROK: VmSwap is not zero 15 getrusage03.c:133: TPASS: initial.children ~= after_zombie.children 16 getrusage03.h:25: TBROK: VmSwap is not zero 17 getrusage03_child.c:57: TPASS: initial.self ~= exec.self 18 getrusage03_child.c:62: TPASS: initial.children ~= exec.children Signed-off-by: Li Wang <liwang@redhat.com> --- Notes: Ps. I also think we might need .min_mem_[avai|total] field but not sure if it is really necessary to add that. testcases/kernel/syscalls/getrusage/getrusage03.c | 9 +++++++++ 1 file changed, 9 insertions(+)