| Message ID | 20211216034125.255907-2-liwang@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] lib: add functions to adjust oom score | expand |
Hi Li,
Not really expert on memory, but this LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
Hi, On 16. 12. 21 4:41, Li Wang wrote: > diff --git a/lib/tst_test.c b/lib/tst_test.c > index ce2b8239d..f3ae48240 100644 > --- a/lib/tst_test.c > +++ b/lib/tst_test.c > @@ -1441,11 +1441,15 @@ static int fork_testrun(void) > > SAFE_SIGNAL(SIGINT, sigint_handler); > > + tst_enable_oom_protection(getppid()); this is exactly what you should *NOT* do because then the OOM protection will also be inherited by all non-LTP processes executed by the same shell (or whatever the parent process is). > + tst_enable_oom_protection(getpid()); > + > test_pid = fork(); > if (test_pid < 0) > tst_brk(TBROK | TERRNO, "fork()"); > > if (!test_pid) { > + tst_cancel_oom_protection(getpid()); > SAFE_SIGNAL(SIGALRM, SIG_DFL); > SAFE_SIGNAL(SIGUSR1, SIG_DFL); > SAFE_SIGNAL(SIGINT, SIG_DFL); >
On Thu, Dec 16, 2021 at 5:56 PM Martin Doucha <mdoucha@suse.cz> wrote: > Hi, > > On 16. 12. 21 4:41, Li Wang wrote: > > diff --git a/lib/tst_test.c b/lib/tst_test.c > > index ce2b8239d..f3ae48240 100644 > > --- a/lib/tst_test.c > > +++ b/lib/tst_test.c > > @@ -1441,11 +1441,15 @@ static int fork_testrun(void) > > > > SAFE_SIGNAL(SIGINT, sigint_handler); > > > > + tst_enable_oom_protection(getppid()); > > this is exactly what you should *NOT* do because then the OOM protection > will also be inherited by all non-LTP processes executed by the same > shell (or whatever the parent process is). > You are right! I previously thought the parent process is only ltp-pan and we only need to cancel the protection in fork_testrun's children. But obviously, one thing I neglected is that some shell tests will still under the affected. And furthermore if run LTP test manually the parent will be the shell, non-LTP process also inherits the score. Thanks for pointing out this, I will remove this line in V2.
diff --git a/lib/tst_test.c b/lib/tst_test.c index ce2b8239d..f3ae48240 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -1441,11 +1441,15 @@ static int fork_testrun(void) SAFE_SIGNAL(SIGINT, sigint_handler); + tst_enable_oom_protection(getppid()); + tst_enable_oom_protection(getpid()); + test_pid = fork(); if (test_pid < 0) tst_brk(TBROK | TERRNO, "fork()"); if (!test_pid) { + tst_cancel_oom_protection(getpid()); SAFE_SIGNAL(SIGALRM, SIG_DFL); SAFE_SIGNAL(SIGUSR1, SIG_DFL); SAFE_SIGNAL(SIGINT, SIG_DFL);
Here invoke OOM protection in fork_testrun, since it is the key point to distiguish many process branches. We do protect ltp test harness($PPID) and main ($PID) process from killing by OOM Killer, hope this can help to get the completed correct report for all of LTP tests. Fundamental principle: (oom protection) ltp test harness --> library process (oom protection) main --> tst_run_tcases --> ... --> fork_testrun (cancel protection) testrun --> run_tests --> ... --> testname() child_test --> ... --> end Note: there might be still argument on doing this protection for test harness, because it will affect all common testcases (I mean none oom tests), but I slightly think it is safe as there seems no much system load during perform them. Signed-off-by: Li Wang <liwang@redhat.com> --- lib/tst_test.c | 4 ++++ 1 file changed, 4 insertions(+)