* [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score @ 2021-12-20 9:54 Li Wang 2021-12-20 9:54 ` [LTP] [PATCH v3 2/3] lib: enable OOM protection for ltp lib process Li Wang ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Li Wang @ 2021-12-20 9:54 UTC (permalink / raw) To: ltp This introduces function to LTP for adjusting the oom_score_adj of target process, which may be helpful in OOM tests to prevent kernel killing the main or lib process during test running. The exported global tst_enable_oom_protection function can be used at anywhere you want to protect, but please remember that if you do enable protection on a process($PID) that all the children will inherit its score and be ignored by OOM Killer as well. So that's why tst_disable_oom_protection is recommended to combination in use. Signed-off-by: Li Wang <liwang@redhat.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> --- Notes: v2 --> v3 * rename to tst_disable_oom_protection * support set PID as 0 to protect current process include/tst_memutils.h | 20 ++++++++++++++++++++ lib/tst_memutils.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/include/tst_memutils.h b/include/tst_memutils.h index f605f544e..a5265423d 100644 --- a/include/tst_memutils.h +++ b/include/tst_memutils.h @@ -25,4 +25,24 @@ void tst_pollute_memory(size_t maxsize, int fillchar); */ long long tst_available_mem(void); +/* + * Enable OOM protection to prevent process($PID) being killed by OOM Killer. + * echo -1000 >/proc/$PID/oom_score_adj + * If the pid is 0 which means it will set on current(self) process. + * + * Note: + * This exported tst_enable_oom_protection function can be used at anywhere + * you want to protect, but please remember that if you do enable protection + * on a process($PID) that all the children will inherit its score and be + * ignored by OOM Killer as well. So that's why tst_disable_oom_protection + * is recommended to combination in use. + */ +void tst_enable_oom_protection(pid_t pid); + +/* + * Disable the OOM protection for the process($PID). + * echo 0 >/proc/$PID/oom_score_adj + */ +void tst_disable_oom_protection(pid_t pid); + #endif /* TST_MEMUTILS_H__ */ diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c index bd09cf6fa..0757fe654 100644 --- a/lib/tst_memutils.c +++ b/lib/tst_memutils.c @@ -3,6 +3,7 @@ * Copyright (c) 2020 SUSE LLC <mdoucha@suse.cz> */ +#include <stdio.h> #include <unistd.h> #include <limits.h> #include <sys/sysinfo.h> @@ -91,3 +92,34 @@ long long tst_available_mem(void) return mem_available; } + +static void set_oom_score_adj(pid_t pid, int value) +{ + int val; + char score_path[64]; + + if (access("/proc/self/oom_score_adj", F_OK) == -1) { + tst_res(TINFO, "Warning: oom_score_adj does not exist"); + return; + } + + if (pid == 0) + sprintf(score_path, "/proc/self/oom_score_adj"); + else + sprintf(score_path, "/proc/%d/oom_score_adj", pid); + + SAFE_FILE_PRINTF(score_path, "%d", value); + SAFE_FILE_SCANF(score_path, "%d", &val); + if (val != value) + tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val, value); +} + +void tst_enable_oom_protection(pid_t pid) +{ + set_oom_score_adj(pid, -1000); +} + +void tst_disable_oom_protection(pid_t pid) +{ + set_oom_score_adj(pid, 0); +} -- 2.31.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [LTP] [PATCH v3 2/3] lib: enable OOM protection for ltp lib process 2021-12-20 9:54 [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Li Wang @ 2021-12-20 9:54 ` Li Wang 2021-12-20 13:26 ` Cyril Hrubis 2021-12-20 9:54 ` [LTP] [PATCH v3 3/3] oom: enable OOM protection for mem " Li Wang ` (3 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Li Wang @ 2021-12-20 9:54 UTC (permalink / raw) To: ltp We do protect ltp-lib ($PID) process from killing by OOM Killer, hope this can help to get the completed correct report for all of LTP tests. This achieve by invoking tst_enable_oom_protection in tst_run_tcases, at the same time, we purposely disabling the protection for children in fork_testrun, to avoid the oom score inherit by testcase. Fundamental principle: ltp test harness --> library process (enable protection) main --> tst_run_tcases --> ... --> fork_testrun (disable protection) testrun --> run_tests --> ... --> testname child_test --> ... --> end Signed-off-by: Li Wang <liwang@redhat.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> --- lib/tst_test.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/tst_test.c b/lib/tst_test.c index ce2b8239d..51f438d06 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -1446,6 +1446,7 @@ static int fork_testrun(void) tst_brk(TBROK | TERRNO, "fork()"); if (!test_pid) { + tst_disable_oom_protection(0); SAFE_SIGNAL(SIGALRM, SIG_DFL); SAFE_SIGNAL(SIGUSR1, SIG_DFL); SAFE_SIGNAL(SIGINT, SIG_DFL); @@ -1523,6 +1524,7 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self) tst_test = self; do_setup(argc, argv); + tst_enable_oom_protection(lib_pid); SAFE_SIGNAL(SIGALRM, alarm_handler); SAFE_SIGNAL(SIGUSR1, heartbeat_handler); -- 2.31.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 2/3] lib: enable OOM protection for ltp lib process 2021-12-20 9:54 ` [LTP] [PATCH v3 2/3] lib: enable OOM protection for ltp lib process Li Wang @ 2021-12-20 13:26 ` Cyril Hrubis 0 siblings, 0 replies; 18+ messages in thread From: Cyril Hrubis @ 2021-12-20 13:26 UTC (permalink / raw) To: Li Wang; +Cc: ltp Hi! Reviewed-by: Cyril Hrubis <chrubis@suse.cz> -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* [LTP] [PATCH v3 3/3] oom: enable OOM protection for mem lib process 2021-12-20 9:54 [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Li Wang 2021-12-20 9:54 ` [LTP] [PATCH v3 2/3] lib: enable OOM protection for ltp lib process Li Wang @ 2021-12-20 9:54 ` Li Wang 2021-12-20 13:27 ` Cyril Hrubis 2021-12-20 13:26 ` [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Cyril Hrubis ` (2 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Li Wang @ 2021-12-20 9:54 UTC (permalink / raw) To: ltp Just simply invoke oom protection on mem library to make it can collect full state of children. Signed-off-by: Li Wang <liwang@redhat.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> --- testcases/kernel/mem/lib/mem.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testcases/kernel/mem/lib/mem.c b/testcases/kernel/mem/lib/mem.c index ac890491c..a4bede0df 100644 --- a/testcases/kernel/mem/lib/mem.c +++ b/testcases/kernel/mem/lib/mem.c @@ -129,8 +129,11 @@ void oom(int testcase, int lite, int retcode, int allow_sigkill) pid_t pid; int status, threads; + tst_enable_oom_protection(0); + switch (pid = SAFE_FORK()) { case 0: + tst_disable_oom_protection(0); threads = MAX(1, tst_ncpus() - 1); child_alloc(testcase, lite, threads); default: -- 2.31.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 3/3] oom: enable OOM protection for mem lib process 2021-12-20 9:54 ` [LTP] [PATCH v3 3/3] oom: enable OOM protection for mem " Li Wang @ 2021-12-20 13:27 ` Cyril Hrubis 0 siblings, 0 replies; 18+ messages in thread From: Cyril Hrubis @ 2021-12-20 13:27 UTC (permalink / raw) To: Li Wang; +Cc: ltp Hi! Reviewed-by: Cyril Hrubis <chrubis@suse.cz> -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-20 9:54 [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Li Wang 2021-12-20 9:54 ` [LTP] [PATCH v3 2/3] lib: enable OOM protection for ltp lib process Li Wang 2021-12-20 9:54 ` [LTP] [PATCH v3 3/3] oom: enable OOM protection for mem " Li Wang @ 2021-12-20 13:26 ` Cyril Hrubis 2021-12-20 18:13 ` Petr Vorel 2021-12-20 18:34 ` Petr Vorel 4 siblings, 0 replies; 18+ messages in thread From: Cyril Hrubis @ 2021-12-20 13:26 UTC (permalink / raw) To: Li Wang; +Cc: ltp Hi! > This introduces function to LTP for adjusting the oom_score_adj of ^ to adjust > target process, which may be helpful in OOM tests to prevent kernel > killing the main or lib process during test running. ^ test run. > The exported global tst_enable_oom_protection function can be used > at anywhere you want to protect, but please remember that if you > do enable protection on a process($PID) that all the children will > inherit its score and be ignored by OOM Killer as well. So that's > why tst_disable_oom_protection is recommended to combination in use. ^ to be used in combination. Reviewed-by: Cyril Hrubis <chrubis@suse.cz> -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-20 9:54 [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Li Wang ` (2 preceding siblings ...) 2021-12-20 13:26 ` [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Cyril Hrubis @ 2021-12-20 18:13 ` Petr Vorel 2021-12-20 18:34 ` Petr Vorel 4 siblings, 0 replies; 18+ messages in thread From: Petr Vorel @ 2021-12-20 18:13 UTC (permalink / raw) To: Li Wang; +Cc: ltp Hi Li, > v2 --> v3 > * rename to tst_disable_oom_protection > * support set PID as 0 to protect current process +1 to the changes + to Cyril suggestions for docs. Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-20 9:54 [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Li Wang ` (3 preceding siblings ...) 2021-12-20 18:13 ` Petr Vorel @ 2021-12-20 18:34 ` Petr Vorel 2021-12-21 2:50 ` Li Wang 4 siblings, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-12-20 18:34 UTC (permalink / raw) To: Li Wang; +Cc: ltp Hi Li, > v2 --> v3 > * rename to tst_disable_oom_protection > * support set PID as 0 to protect current process > +static void set_oom_score_adj(pid_t pid, int value) > +{ > + int val; > + char score_path[64]; > + > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { We need to check here also /proc/PID/oom_score_adj, i.e. score_path. Otherwise tests without root fail fail. lib/tst_memutils.c:111: TBROK: Failed to close FILE '/proc/26980/oom_score_adj': EACCES (13) Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-20 18:34 ` Petr Vorel @ 2021-12-21 2:50 ` Li Wang 2021-12-21 8:33 ` Petr Vorel 2021-12-21 9:18 ` Cyril Hrubis 0 siblings, 2 replies; 18+ messages in thread From: Li Wang @ 2021-12-21 2:50 UTC (permalink / raw) To: Petr Vorel; +Cc: LTP List [-- Attachment #1.1: Type: text/plain, Size: 826 bytes --] On Tue, Dec 21, 2021 at 2:34 AM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > v2 --> v3 > > * rename to tst_disable_oom_protection > > * support set PID as 0 to protect current process > > > +static void set_oom_score_adj(pid_t pid, int value) > > +{ > > + int val; > > + char score_path[64]; > > + > > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { > We need to check here also /proc/PID/oom_score_adj, i.e. score_path. > Good catch, I would add a 'W_OK' checking and skip the setting with a reminder message if run without root. how about this? if (access(score_path, W_OK) == -1) { tst_res(TINFO, "Warning: %s cannot be accessed for writing, please check if test run with root user.", score_path); return } -- Regards, Li Wang [-- Attachment #1.2: Type: text/html, Size: 2022 bytes --] [-- Attachment #2: Type: text/plain, Size: 60 bytes --] -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 2:50 ` Li Wang @ 2021-12-21 8:33 ` Petr Vorel 2021-12-21 9:18 ` Cyril Hrubis 1 sibling, 0 replies; 18+ messages in thread From: Petr Vorel @ 2021-12-21 8:33 UTC (permalink / raw) To: Li Wang; +Cc: LTP List Hi Li, > > > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { > > We need to check here also /proc/PID/oom_score_adj, i.e. score_path. > Good catch, I would add a 'W_OK' checking and skip the setting with > a reminder message if run without root. > how about this? > if (access(score_path, W_OK) == -1) { > tst_res(TINFO, "Warning: %s cannot be accessed for writing, > please check if test run with root user.", > score_path); > return > } No, value < 0 obviously requires root, thus access() check is not enough. Try echo -1 > /proc/$$/oom_score_adj Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 2:50 ` Li Wang 2021-12-21 8:33 ` Petr Vorel @ 2021-12-21 9:18 ` Cyril Hrubis 2021-12-21 9:23 ` Li Wang 1 sibling, 1 reply; 18+ messages in thread From: Cyril Hrubis @ 2021-12-21 9:18 UTC (permalink / raw) To: Li Wang; +Cc: LTP List Hi! > > > v2 --> v3 > > > * rename to tst_disable_oom_protection > > > * support set PID as 0 to protect current process > > > > > +static void set_oom_score_adj(pid_t pid, int value) > > > +{ > > > + int val; > > > + char score_path[64]; > > > + > > > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { > > We need to check here also /proc/PID/oom_score_adj, i.e. score_path. > > > > Good catch, I would add a 'W_OK' checking and skip the setting with > a reminder message if run without root. > > how about this? > > if (access(score_path, W_OK) == -1) { > tst_res(TINFO, "Warning: %s cannot be accessed for writing, > please check if test run with root user.", > score_path); Hmm, I guess that we should produce TINFO if the file does not exist and TWARN if we cannot write to it. Something as: if (access(score_path, F_OK)) { tst_res(TINFO, "'%s' does not exist, skipping OOM score adjustement", score_path); return; } if (access(score_path, W_OK)) { tst_res(TWARN, "'%s' not writeable, are you root?", score_path); return; } > return > } > > > -- > Regards, > Li Wang -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 9:18 ` Cyril Hrubis @ 2021-12-21 9:23 ` Li Wang 2021-12-21 9:40 ` Li Wang 2021-12-21 9:59 ` Cyril Hrubis 0 siblings, 2 replies; 18+ messages in thread From: Li Wang @ 2021-12-21 9:23 UTC (permalink / raw) To: Cyril Hrubis; +Cc: LTP List [-- Attachment #1.1: Type: text/plain, Size: 2963 bytes --] On Tue, Dec 21, 2021 at 5:17 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > > v2 --> v3 > > > > * rename to tst_disable_oom_protection > > > > * support set PID as 0 to protect current process > > > > > > > +static void set_oom_score_adj(pid_t pid, int value) > > > > +{ > > > > + int val; > > > > + char score_path[64]; > > > > + > > > > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { > > > We need to check here also /proc/PID/oom_score_adj, i.e. score_path. > > > > > > > Good catch, I would add a 'W_OK' checking and skip the setting with > > a reminder message if run without root. > > > > how about this? > > > > if (access(score_path, W_OK) == -1) { > > tst_res(TINFO, "Warning: %s cannot be accessed for writing, > > please check if test run with root user.", > > score_path); > > Hmm, I guess that we should produce TINFO if the file does not exist and > TWARN if we cannot write to it. Something as: > Not exactly, if someone gives a wrong PID, that also cannot find the score_path. So we shouldn't skip OOM adjustment only with printing the TFINO. > > if (access(score_path, F_OK)) { > tst_res(TINFO, > "'%s' does not exist, skipping OOM score adjustement", > score_path); > return; > } > > if (access(score_path, W_OK)) { > tst_res(TWARN, "'%s' not writeable, are you root?", score_path); > return; > } > As Petr points out, only root user can set a negative value to oom_score_adj, this W_OK is not enough for ordinary users. Consider about situation, I'd suggest go with non-safe macros and add additional check in the last. e.g. --- a/lib/tst_memutils.c +++ b/lib/tst_memutils.c @@ -108,17 +108,21 @@ static void set_oom_score_adj(pid_t pid, int value) else sprintf(score_path, "/proc/%d/oom_score_adj", pid); - if (access(score_path, R_OK | W_OK) == -1) { - tst_res(TINFO, "Warning: %s cannot be accessed for reading/writing, - please check if test run with root user.", - score_path); - return - } - - SAFE_FILE_PRINTF(score_path, "%d", value); - SAFE_FILE_SCANF(score_path, "%d", &val); - if (val != value) + if (access(score_path, F_OK) == -1) + tst_brk(TBROK, "%s does not exist, please check if PID is valid"); + + FILE_PRINTF(score_path, "%d", value); + FILE_SCANF(score_path, "%d", &val); + + if (val != value) { + if (value < 0) { + tst_res(TINFO, "Warning: %s cannot be set to negative value, + please check if test run with root user.", + score_path); + return + } tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val, value); + } } -- Regards, Li Wang [-- Attachment #1.2: Type: text/html, Size: 5040 bytes --] [-- Attachment #2: Type: text/plain, Size: 60 bytes --] -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 9:23 ` Li Wang @ 2021-12-21 9:40 ` Li Wang 2021-12-21 9:59 ` Cyril Hrubis 1 sibling, 0 replies; 18+ messages in thread From: Li Wang @ 2021-12-21 9:40 UTC (permalink / raw) To: Cyril Hrubis; +Cc: LTP List On Tue, Dec 21, 2021 at 5:23 PM Li Wang <liwang@redhat.com> wrote: > Consider about situation, I'd suggest go with non-safe macros and add > additional check in the last. > > e.g. > > --- a/lib/tst_memutils.c > +++ b/lib/tst_memutils.c > @@ -108,17 +108,21 @@ static void set_oom_score_adj(pid_t pid, int value) > else > sprintf(score_path, "/proc/%d/oom_score_adj", pid); > > - if (access(score_path, R_OK | W_OK) == -1) { > - tst_res(TINFO, "Warning: %s cannot be accessed for reading/writing, > - please check if test run with root user.", > - score_path); > - return > - } > - > - SAFE_FILE_PRINTF(score_path, "%d", value); > - SAFE_FILE_SCANF(score_path, "%d", &val); > - if (val != value) > + if (access(score_path, F_OK) == -1) > + tst_brk(TBROK, "%s does not exist, please check if PID is valid"); > + > + FILE_PRINTF(score_path, "%d", value); > + FILE_SCANF(score_path, "%d", &val); > + > + if (val != value) { > + if (value < 0) { > + tst_res(TINFO, "Warning: %s cannot be set to negative value, > + please check if test run with root user.", > + score_path); > + return > + } > tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val, value); > + } > } Sorry, the above patch is based on my private v3.1, plz ignore it. For better read, here give the whole function: +static void set_oom_score_adj(pid_t pid, int value) +{ + int val; + char score_path[64]; + + if (access("/proc/self/oom_score_adj", F_OK) == -1) { + tst_res(TINFO, + "Warning: oom_score_adj does not exist, skipping OOM the adjustement"); + return; + } + + if (pid == 0) + sprintf(score_path, "/proc/self/oom_score_adj"); + else + sprintf(score_path, "/proc/%d/oom_score_adj", pid); + + if (access(score_path, F_OK) == -1) + tst_brk(TBROK, "%s does not exist, please check if PID is valid"); + + FILE_PRINTF(score_path, "%d", value); + FILE_SCANF(score_path, "%d", &val); + + if (val != value) { + if (value < 0) { + tst_res(TINFO, "Warning: %s cannot be set to negative value, + please check if test run with root user.", + score_path); + return + } + tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val, value); + } +} -- Regards, Li Wang -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 9:23 ` Li Wang 2021-12-21 9:40 ` Li Wang @ 2021-12-21 9:59 ` Cyril Hrubis 2021-12-21 10:12 ` Li Wang 1 sibling, 1 reply; 18+ messages in thread From: Cyril Hrubis @ 2021-12-21 9:59 UTC (permalink / raw) To: Li Wang; +Cc: LTP List Hi! > Not exactly, if someone gives a wrong PID, that also cannot find > the score_path. So we shouldn't skip OOM adjustment only > with printing the TFINO. Right, we would have to check if the /proc/$PID/ directory exists first. > > if (access(score_path, F_OK)) { > > tst_res(TINFO, > > "'%s' does not exist, skipping OOM score adjustement", > > score_path); > > return; > > } > > > > if (access(score_path, W_OK)) { > > tst_res(TWARN, "'%s' not writeable, are you root?", score_path); > > return; > > } > > > > As Petr points out, only root user can set a negative value to > oom_score_adj, > this W_OK is not enough for ordinary users. Ah, right this makes it even more complex. > Consider about situation, I'd suggest go with non-safe macros and add > additional check in the last. > > e.g. > > --- a/lib/tst_memutils.c > +++ b/lib/tst_memutils.c > @@ -108,17 +108,21 @@ static void set_oom_score_adj(pid_t pid, int value) > else > sprintf(score_path, "/proc/%d/oom_score_adj", pid); > > - if (access(score_path, R_OK | W_OK) == -1) { > - tst_res(TINFO, "Warning: %s cannot be accessed for > reading/writing, > - please check if test run with root user.", > - score_path); > - return > - } > - > - SAFE_FILE_PRINTF(score_path, "%d", value); > - SAFE_FILE_SCANF(score_path, "%d", &val); > - if (val != value) > + if (access(score_path, F_OK) == -1) > + tst_brk(TBROK, "%s does not exist, please check if PID is > valid"); Maybe we should print the pid in the message here as well. > + > + FILE_PRINTF(score_path, "%d", value); > + FILE_SCANF(score_path, "%d", &val); > + > + if (val != value) { > + if (value < 0) { > + tst_res(TINFO, "Warning: %s cannot be set to > negative value, > + please check if test run with root user.", I would say that we TBROK here, otherwise it could be silently ignored. Also I would shorten the message to something as: "'%s' cannot be set to %i, are you root?", score_path, value); > + score_path); > + return > + } > tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val, > value); > + } > } > > > -- > Regards, > Li Wang -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 9:59 ` Cyril Hrubis @ 2021-12-21 10:12 ` Li Wang 2021-12-21 10:36 ` Cyril Hrubis 0 siblings, 1 reply; 18+ messages in thread From: Li Wang @ 2021-12-21 10:12 UTC (permalink / raw) To: Cyril Hrubis; +Cc: LTP List On Tue, Dec 21, 2021 at 5:57 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Not exactly, if someone gives a wrong PID, that also cannot find > > the score_path. So we shouldn't skip OOM adjustment only > > with printing the TFINO. > > Right, we would have to check if the /proc/$PID/ directory exists > first. Not necessary, because we did check /proc/self/oom_score_adj already, otherwise, we have no chance to get here. > > + if (access(score_path, F_OK) == -1) > > + tst_brk(TBROK, "%s does not exist, please check if PID is > > valid"); > > Maybe we should print the pid in the message here as well. Yes, and the score_path will include the pid, here I forget in the sentence. > > + if (val != value) { > > + if (value < 0) { > > + tst_res(TINFO, "Warning: %s cannot be set to > > negative value, > > + please check if test run with root user.", > > I would say that we TBROK here, otherwise it could be silently ignored. > Also I would shorten the message to something as: Hmm, maybe tst_res(TWARN, ...), is enough, I don't want to let people who run LTP in ordinary users get TBROK here since they might even don't need the oom protection. So, I will push (the improved) code like below, after getting Petr and you ack again: +static void set_oom_score_adj(pid_t pid, int value) +{ + int val; + char score_path[64]; + + if (access("/proc/self/oom_score_adj", F_OK) == -1) { + tst_res(TINFO, "Warning: oom_score_adj does not exist, + skipping the adjustement"); + return; + } + + if (pid == 0) { + sprintf(score_path, "/proc/self/oom_score_adj"); + } else { + sprintf(score_path, "/proc/%d/oom_score_adj", pid); + if (access(score_path, F_OK) == -1) + tst_brk(TBROK, "%s does not exist, please check if PID is valid", score_path); + } + + FILE_PRINTF(score_path, "%d", value); + FILE_SCANF(score_path, "%d", &val); + + if (val != value) { + if (value < 0) { + tst_res(TWARN, "'%s' cannot be set to %i, are you root?", + score_path, value); + return + } + tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val, value); + } +} -- Regards, Li Wang -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 10:12 ` Li Wang @ 2021-12-21 10:36 ` Cyril Hrubis 2021-12-21 10:44 ` Petr Vorel 0 siblings, 1 reply; 18+ messages in thread From: Cyril Hrubis @ 2021-12-21 10:36 UTC (permalink / raw) To: Li Wang; +Cc: LTP List Hi! > So, I will push (the improved) code like below, after getting > Petr and you ack again: > > +static void set_oom_score_adj(pid_t pid, int value) > +{ > + int val; > + char score_path[64]; > + > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { > + tst_res(TINFO, "Warning: oom_score_adj does not exist, > + skipping the adjustement"); I'm not sure about the "Warning:" in this message, I would just dully informed the user that the interface is not available. > + return;` > + } > + > + if (pid == 0) { > + sprintf(score_path, "/proc/self/oom_score_adj"); > + } else { > + sprintf(score_path, "/proc/%d/oom_score_adj", pid); > + if (access(score_path, F_OK) == -1) > + tst_brk(TBROK, "%s does not exist, please > check if PID is valid", score_path); > + } > + > + FILE_PRINTF(score_path, "%d", value); > + FILE_SCANF(score_path, "%d", &val); > + > + if (val != value) { > + if (value < 0) { > + tst_res(TWARN, "'%s' cannot be set to %i, are > you root?", > + score_path, value); > + return > + } > + tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", > val, value); > + } > +} Anyways this version looks good to me: Reviewed-by: Cyril Hrubis <chrubis@suse.cz> -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 10:36 ` Cyril Hrubis @ 2021-12-21 10:44 ` Petr Vorel 2021-12-21 11:32 ` Li Wang 0 siblings, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-12-21 10:44 UTC (permalink / raw) To: Cyril Hrubis; +Cc: LTP List > Hi! > > So, I will push (the improved) code like below, after getting > > Petr and you ack again: > > +static void set_oom_score_adj(pid_t pid, int value) > > +{ > > + int val; > > + char score_path[64]; > > + > > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { > > + tst_res(TINFO, "Warning: oom_score_adj does not exist, > > + skipping the adjustement"); > I'm not sure about the "Warning:" in this message, I would just dully > informed the user that the interface is not available. I'd also prefer not having to print "Warning:" in TINFO message (I know TWARN exit non-zero). Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score 2021-12-21 10:44 ` Petr Vorel @ 2021-12-21 11:32 ` Li Wang 0 siblings, 0 replies; 18+ messages in thread From: Li Wang @ 2021-12-21 11:32 UTC (permalink / raw) To: Petr Vorel; +Cc: LTP List Hi Petr, Cyril, On Tue, Dec 21, 2021 at 6:44 PM Petr Vorel <pvorel@suse.cz> wrote: > > > + if (access("/proc/self/oom_score_adj", F_OK) == -1) { > > > + tst_res(TINFO, "Warning: oom_score_adj does not exist, > > > + skipping the adjustement"); > > > I'm not sure about the "Warning:" in this message, I would just dully > > informed the user that the interface is not available. > > I'd also prefer not having to print "Warning:" in TINFO message > (I know TWARN exit non-zero). Okay, I deleted that keyword and pushed patchset. Thanks a lot for your both review. -- Regards, Li Wang -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-12-21 11:32 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-20 9:54 [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Li Wang 2021-12-20 9:54 ` [LTP] [PATCH v3 2/3] lib: enable OOM protection for ltp lib process Li Wang 2021-12-20 13:26 ` Cyril Hrubis 2021-12-20 9:54 ` [LTP] [PATCH v3 3/3] oom: enable OOM protection for mem " Li Wang 2021-12-20 13:27 ` Cyril Hrubis 2021-12-20 13:26 ` [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score Cyril Hrubis 2021-12-20 18:13 ` Petr Vorel 2021-12-20 18:34 ` Petr Vorel 2021-12-21 2:50 ` Li Wang 2021-12-21 8:33 ` Petr Vorel 2021-12-21 9:18 ` Cyril Hrubis 2021-12-21 9:23 ` Li Wang 2021-12-21 9:40 ` Li Wang 2021-12-21 9:59 ` Cyril Hrubis 2021-12-21 10:12 ` Li Wang 2021-12-21 10:36 ` Cyril Hrubis 2021-12-21 10:44 ` Petr Vorel 2021-12-21 11:32 ` Li Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.