* [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
* [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 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 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
* 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
` (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.