All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
@ 2021-12-21 19:35 Petr Vorel
  2021-12-22  2:25 ` xuyang2018.jy
  0 siblings, 1 reply; 16+ messages in thread
From: Petr Vorel @ 2021-12-21 19:35 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

If needed to set value also for non-root, use set_oom_score_adj().

Fixes: 8a0827766d ("lib: add functions to adjust oom score")

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/tst_memutils.h | 11 ++++++++++-
 lib/tst_memutils.c     |  6 ++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/tst_memutils.h b/include/tst_memutils.h
index 68a6e37714..e6f946ac0c 100644
--- a/include/tst_memutils.h
+++ b/include/tst_memutils.h
@@ -30,11 +30,15 @@ long long tst_available_mem(void);
  *   echo -1000 >/proc/$PID/oom_score_adj
  * If the pid is 0 which means it will set on current(self) process.
  *
+ * WARNING:
+ *  Do nothing for non-root, because setting value < 0 requires root.
+    If you want to set value also for non-root, use set_oom_score_adj().
+ *
  * 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
+ *  ignored by OOM Killer as well. So that's why tst_disable_oom_protection()
  *  to be used in combination.
  */
 void tst_enable_oom_protection(pid_t pid);
@@ -42,6 +46,11 @@ void tst_enable_oom_protection(pid_t pid);
 /*
  * Disable the OOM protection for the process($PID).
  *   echo 0 >/proc/$PID/oom_score_adj
+ *
+ * WARNING:
+ *  Do nothing for non-root, because it's expected to be cleanup after
+ *  tst_enable_oom_protection(). Use set_oom_score_adj(), if you want to set
+ *  value also for non-root.
  */
 void tst_disable_oom_protection(pid_t pid);
 
diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
index 4346508d9a..f0695e026a 100644
--- a/lib/tst_memutils.c
+++ b/lib/tst_memutils.c
@@ -126,10 +126,16 @@ static void set_oom_score_adj(pid_t pid, int value)
 
 void tst_enable_oom_protection(pid_t pid)
 {
+	if (geteuid() != 0)
+		return;
+
 	set_oom_score_adj(pid, -1000);
 }
 
 void tst_disable_oom_protection(pid_t pid)
 {
+	if (geteuid() != 0)
+		return;
+
 	set_oom_score_adj(pid, 0);
 }
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-21 19:35 [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root Petr Vorel
@ 2021-12-22  2:25 ` xuyang2018.jy
  2021-12-22  3:32   ` Li Wang
  0 siblings, 1 reply; 16+ messages in thread
From: xuyang2018.jy @ 2021-12-22  2:25 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Richard Palethorpe, ltp

Hi Petr
> If needed to set value also for non-root, use set_oom_score_adj().
If so, why not rename set_oom_score_adj to tst_set_oom_score_adj and add 
declartion to tst_memutils.h?

ps: also have a word typo in set_oom_score_adj, adjustement => adjustment.

Best Regards
Yang Xu
>
> Fixes: 8a0827766d ("lib: add functions to adjust oom score")
>
> Signed-off-by: Petr Vorel<pvorel@suse.cz>
> ---
>   include/tst_memutils.h | 11 ++++++++++-
>   lib/tst_memutils.c     |  6 ++++++
>   2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/include/tst_memutils.h b/include/tst_memutils.h
> index 68a6e37714..e6f946ac0c 100644
> --- a/include/tst_memutils.h
> +++ b/include/tst_memutils.h
> @@ -30,11 +30,15 @@ long long tst_available_mem(void);
>    *   echo -1000>/proc/$PID/oom_score_adj
>    * If the pid is 0 which means it will set on current(self) process.
>    *
> + * WARNING:
> + *  Do nothing for non-root, because setting value<  0 requires root.
> +    If you want to set value also for non-root, use set_oom_score_adj().
> + *
>    * 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
> + *  ignored by OOM Killer as well. So that's why tst_disable_oom_protection()
>    *  to be used in combination.
>    */
>   void tst_enable_oom_protection(pid_t pid);
> @@ -42,6 +46,11 @@ void tst_enable_oom_protection(pid_t pid);
>   /*
>    * Disable the OOM protection for the process($PID).
>    *   echo 0>/proc/$PID/oom_score_adj
> + *
> + * WARNING:
> + *  Do nothing for non-root, because it's expected to be cleanup after
> + *  tst_enable_oom_protection(). Use set_oom_score_adj(), if you want to set
> + *  value also for non-root.
>    */
>   void tst_disable_oom_protection(pid_t pid);
>
> diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
> index 4346508d9a..f0695e026a 100644
> --- a/lib/tst_memutils.c
> +++ b/lib/tst_memutils.c
> @@ -126,10 +126,16 @@ static void set_oom_score_adj(pid_t pid, int value)
>
>   void tst_enable_oom_protection(pid_t pid)
>   {
> +	if (geteuid() != 0)
> +		return;
> +
>   	set_oom_score_adj(pid, -1000);
>   }
>
>   void tst_disable_oom_protection(pid_t pid)
>   {
> +	if (geteuid() != 0)
> +		return;
> +
>   	set_oom_score_adj(pid, 0);
>   }

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  2:25 ` xuyang2018.jy
@ 2021-12-22  3:32   ` Li Wang
  2021-12-22  6:05     ` xuyang2018.jy
  2021-12-22  8:06     ` Petr Vorel
  0 siblings, 2 replies; 16+ messages in thread
From: Li Wang @ 2021-12-22  3:32 UTC (permalink / raw)
  To: xuyang2018.jy; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 3078 bytes --]

Hi Xu, Petr,

On Wed, Dec 22, 2021 at 10:26 AM xuyang2018.jy@fujitsu.com <
xuyang2018.jy@fujitsu.com> wrote:

> Hi Petr
> > If needed to set value also for non-root, use set_oom_score_adj().
> If so, why not rename set_oom_score_adj to tst_set_oom_score_adj and add
> declartion to tst_memutils.h?
>

Yes, it makes sense to expose this function to users to cover
more oom test scenarios. For instance, set a high (>0) or low (<0)
score in child_alloc() to verify if OOM-Killer still works well.
But so far, we don't have such tests.


> ps: also have a word typo in set_oom_score_adj, adjustement => adjustment.
>
> Best Regards
> Yang Xu
> >
> > Fixes: 8a0827766d ("lib: add functions to adjust oom score")
> >
> > Signed-off-by: Petr Vorel<pvorel@suse.cz>
> > ---
> >   include/tst_memutils.h | 11 ++++++++++-
> >   lib/tst_memutils.c     |  6 ++++++
> >   2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/tst_memutils.h b/include/tst_memutils.h
> > index 68a6e37714..e6f946ac0c 100644
> > --- a/include/tst_memutils.h
> > +++ b/include/tst_memutils.h
> > @@ -30,11 +30,15 @@ long long tst_available_mem(void);
> >    *   echo -1000>/proc/$PID/oom_score_adj
> >    * If the pid is 0 which means it will set on current(self) process.
> >    *
> > + * WARNING:
> > + *  Do nothing for non-root, because setting value<  0 requires root.
> > +    If you want to set value also for non-root, use set_oom_score_adj().
> > + *
> >    * 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
> > + *  ignored by OOM Killer as well. So that's why
> tst_disable_oom_protection()
> >    *  to be used in combination.
> >    */
> >   void tst_enable_oom_protection(pid_t pid);
> > @@ -42,6 +46,11 @@ void tst_enable_oom_protection(pid_t pid);
> >   /*
> >    * Disable the OOM protection for the process($PID).
> >    *   echo 0>/proc/$PID/oom_score_adj
> > + *
> > + * WARNING:
> > + *  Do nothing for non-root, because it's expected to be cleanup after
> > + *  tst_enable_oom_protection(). Use set_oom_score_adj(), if you want
> to set
> > + *  value also for non-root.
> >    */
> >   void tst_disable_oom_protection(pid_t pid);
> >
> > diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
> > index 4346508d9a..f0695e026a 100644
> > --- a/lib/tst_memutils.c
> > +++ b/lib/tst_memutils.c
> > @@ -126,10 +126,16 @@ static void set_oom_score_adj(pid_t pid, int value)
> >
> >   void tst_enable_oom_protection(pid_t pid)
> >   {
> > +     if (geteuid() != 0)
>

This is not working as expected in Github CI. I'm still looking at the
problem.
https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true

And the worth mentioning, maybe better to do this check
in set_oom_score_adj() if we do not decide to expose
that function to user.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 4899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  3:32   ` Li Wang
@ 2021-12-22  6:05     ` xuyang2018.jy
  2021-12-22  8:14       ` Petr Vorel
  2021-12-22  8:06     ` Petr Vorel
  1 sibling, 1 reply; 16+ messages in thread
From: xuyang2018.jy @ 2021-12-22  6:05 UTC (permalink / raw)
  To: Li Wang; +Cc: Richard Palethorpe, ltp

Hi Li
> Hi Xu, Petr,
>
> On Wed, Dec 22, 2021 at 10:26 AM xuyang2018.jy@fujitsu.com
> <mailto:xuyang2018.jy@fujitsu.com> <xuyang2018.jy@fujitsu.com
> <mailto:xuyang2018.jy@fujitsu.com>> wrote:
>
>     Hi Petr
>      > If needed to set value also for non-root, use set_oom_score_adj().
>     If so, why not rename set_oom_score_adj to tst_set_oom_score_adj and
>     add
>     declartion to tst_memutils.h?
>
>
> Yes, it makes sense to expose this function to users to cover
> more oom test scenarios. For instance, set a high (>0) or low (<0)
> score in child_alloc() to verify if OOM-Killer still works well.
> But so far, we don't have such tests.
>
>
>     ps: also have a word typo in set_oom_score_adj, adjustement =>
>     adjustment.
>
>     Best Regards
>     Yang Xu
>      >
>      > Fixes: 8a0827766d ("lib: add functions to adjust oom score")
>      >
>      > Signed-off-by: Petr Vorel<pvorel@suse.cz <mailto:pvorel@suse.cz>>
>      > ---
>      > include/tst_memutils.h | 11 ++++++++++-
>      > lib/tst_memutils.c | 6 ++++++
>      > 2 files changed, 16 insertions(+), 1 deletion(-)
>      >
>      > diff --git a/include/tst_memutils.h b/include/tst_memutils.h
>      > index 68a6e37714..e6f946ac0c 100644
>      > --- a/include/tst_memutils.h
>      > +++ b/include/tst_memutils.h
>      > @@ -30,11 +30,15 @@ long long tst_available_mem(void);
>      > * echo -1000>/proc/$PID/oom_score_adj
>      > * If the pid is 0 which means it will set on current(self) process.
>      > *
>      > + * WARNING:
>      > + * Do nothing for non-root, because setting value< 0 requires root.
>      > + If you want to set value also for non-root, use
>     set_oom_score_adj().
>      > + *
>      > * 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
>      > + * ignored by OOM Killer as well. So that's why
>     tst_disable_oom_protection()
>      > * to be used in combination.
>      > */
>      > void tst_enable_oom_protection(pid_t pid);
>      > @@ -42,6 +46,11 @@ void tst_enable_oom_protection(pid_t pid);
>      > /*
>      > * Disable the OOM protection for the process($PID).
>      > * echo 0>/proc/$PID/oom_score_adj
>      > + *
>      > + * WARNING:
>      > + * Do nothing for non-root, because it's expected to be cleanup
>     after
>      > + * tst_enable_oom_protection(). Use set_oom_score_adj(), if you
>     want to set
>      > + * value also for non-root.
>      > */
>      > void tst_disable_oom_protection(pid_t pid);
>      >
>      > diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
>      > index 4346508d9a..f0695e026a 100644
>      > --- a/lib/tst_memutils.c
>      > +++ b/lib/tst_memutils.c
>      > @@ -126,10 +126,16 @@ static void set_oom_score_adj(pid_t pid,
>     int value)
>      >
>      > void tst_enable_oom_protection(pid_t pid)
>      > {
>      > + if (geteuid() != 0)
>
>
> This is not working as expected in Github CI. I'm still looking at the
> problem.
> https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true
I tested local but it works well. I guess ci fails because of linux user 
namespace. Maybe we should require CAP_SYS_RESOURCE cap instead of using 
geteuid.

Best Regards
Yang Xu
>
> And the worth mentioning, maybe better to do this check
> in set_oom_score_adj() if we do not decide to expose
> that function to user.
>
> --
> Regards,
> Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  3:32   ` Li Wang
  2021-12-22  6:05     ` xuyang2018.jy
@ 2021-12-22  8:06     ` Petr Vorel
  1 sibling, 0 replies; 16+ messages in thread
From: Petr Vorel @ 2021-12-22  8:06 UTC (permalink / raw)
  To: xuyang2018.jy, Li Wang; +Cc: Richard Palethorpe, ltp

Hi Xu, Li, all,

> Hi Xu, Petr,

> On Wed, Dec 22, 2021 at 10:26 AM xuyang2018.jy@fujitsu.com <
> xuyang2018.jy@fujitsu.com> wrote:

> > Hi Petr
> > > If needed to set value also for non-root, use set_oom_score_adj().
> > If so, why not rename set_oom_score_adj to tst_set_oom_score_adj and add
> > declartion to tst_memutils.h?


> Yes, it makes sense to expose this function to users to cover
> more oom test scenarios. For instance, set a high (>0) or low (<0)
> score in child_alloc() to verify if OOM-Killer still works well.
> But so far, we don't have such tests.

Sure, I we can do it. But first I'd prefer to fix CI (beware we have last
working day before xmas vacation).

> > ps: also have a word typo in set_oom_score_adj, adjustement => adjustment.
+1, thanks!

Kind regards,
Petr

> > Best Regards
> > Yang Xu

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  6:05     ` xuyang2018.jy
@ 2021-12-22  8:14       ` Petr Vorel
  2021-12-22  8:37         ` xuyang2018.jy
  0 siblings, 1 reply; 16+ messages in thread
From: Petr Vorel @ 2021-12-22  8:14 UTC (permalink / raw)
  To: xuyang2018.jy; +Cc: Richard Palethorpe, ltp

Hi Xu,

> > This is not working as expected in Github CI. I'm still looking at the
> > problem.
> > https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true
> I tested local but it works well. I guess ci fails because of linux user 
> namespace. Maybe we should require CAP_SYS_RESOURCE cap instead of using 
> geteuid.
Good catch, verifying.

Kind regards,
Petr

> Best Regards
> Yang Xu

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  8:14       ` Petr Vorel
@ 2021-12-22  8:37         ` xuyang2018.jy
  2021-12-22  9:48           ` Li Wang
  2021-12-22 10:38           ` Petr Vorel
  0 siblings, 2 replies; 16+ messages in thread
From: xuyang2018.jy @ 2021-12-22  8:37 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Richard Palethorpe, ltp

Hi Petr
> Hi Xu,
>
>>> This is not working as expected in Github CI. I'm still looking at the
>>> problem.
>>> https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true
>> I tested local but it works well. I guess ci fails because of linux user
>> namespace. Maybe we should require CAP_SYS_RESOURCE cap instead of using
>> geteuid.
> Good catch, verifying.
You can refer to my ltp fork
https://github.com/xuyang0410/ltp/commits/oom_kill_ci_fixes

Best Regards
Yang Xu
>
> Kind regards,
> Petr
>
>> Best Regards
>> Yang Xu

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  8:37         ` xuyang2018.jy
@ 2021-12-22  9:48           ` Li Wang
  2021-12-22 10:09             ` Cyril Hrubis
  2021-12-22 10:38           ` Petr Vorel
  1 sibling, 1 reply; 16+ messages in thread
From: Li Wang @ 2021-12-22  9:48 UTC (permalink / raw)
  To: xuyang2018.jy; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 780 bytes --]

Hi Xu, Petr,

On Wed, Dec 22, 2021 at 4:37 PM xuyang2018.jy@fujitsu.com <
xuyang2018.jy@fujitsu.com> wrote:

> Hi Petr
> > Hi Xu,
> >
> >>> This is not working as expected in Github CI. I'm still looking at the
> >>> problem.
> >>>
> https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true
> >> I tested local but it works well. I guess ci fails because of linux user
> >> namespace. Maybe we should require CAP_SYS_RESOURCE cap instead of using
> >> geteuid.
> > Good catch, verifying.
> You can refer to my ltp fork
> https://github.com/xuyang0410/ltp/commits/oom_kill_ci_fixes


+1

I was thinking of adding the permission but that's
might not be a good way for the namespace. Like your
method which only does check and return is better.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 1917 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  9:48           ` Li Wang
@ 2021-12-22 10:09             ` Cyril Hrubis
  2021-12-22 10:24               ` Petr Vorel
  0 siblings, 1 reply; 16+ messages in thread
From: Cyril Hrubis @ 2021-12-22 10:09 UTC (permalink / raw)
  To: Li Wang; +Cc: Richard Palethorpe, ltp

Hi!
> > >>> This is not working as expected in Github CI. I'm still looking at the
> > >>> problem.
> > >>>
> > https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true
> > >> I tested local but it works well. I guess ci fails because of linux user
> > >> namespace. Maybe we should require CAP_SYS_RESOURCE cap instead of using
> > >> geteuid.
> > > Good catch, verifying.
> > You can refer to my ltp fork
> > https://github.com/xuyang0410/ltp/commits/oom_kill_ci_fixes
> 
> 
> +1
> 
> I was thinking of adding the permission but that's
> might not be a good way for the namespace. Like your
> method which only does check and return is better.

Wouldn't it be actually easier just to catch the error from the actual
write? The whole problem here is that we use SAFE_PRINTF() on something
that may actualy fail and the failure shouldn't be fatal at all.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22 10:09             ` Cyril Hrubis
@ 2021-12-22 10:24               ` Petr Vorel
  2021-12-22 10:40                 ` Cyril Hrubis
  0 siblings, 1 reply; 16+ messages in thread
From: Petr Vorel @ 2021-12-22 10:24 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: Richard Palethorpe, ltp

Hi Cyril, all,

> Hi!
> > > >>> This is not working as expected in Github CI. I'm still looking at the
> > > >>> problem.

> > > https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true
> > > >> I tested local but it works well. I guess ci fails because of linux user
> > > >> namespace. Maybe we should require CAP_SYS_RESOURCE cap instead of using
> > > >> geteuid.
> > > > Good catch, verifying.
> > > You can refer to my ltp fork
> > > https://github.com/xuyang0410/ltp/commits/oom_kill_ci_fixes


> > +1

> > I was thinking of adding the permission but that's
> > might not be a good way for the namespace. Like your
> > method which only does check and return is better.

> Wouldn't it be actually easier just to catch the error from the actual
> write? The whole problem here is that we use SAFE_PRINTF() on something
> that may actualy fail and the failure shouldn't be fatal at all.

We use just FILE_PRINTF(), but we check the result and TWARN, which causes the
failure in CI.

I've sent v2, which checks CAP_SYS_ADMIN and CAP_SYS_RESOURCE,
but feel free just to bring simpler solution.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22  8:37         ` xuyang2018.jy
  2021-12-22  9:48           ` Li Wang
@ 2021-12-22 10:38           ` Petr Vorel
  1 sibling, 0 replies; 16+ messages in thread
From: Petr Vorel @ 2021-12-22 10:38 UTC (permalink / raw)
  To: xuyang2018.jy; +Cc: Richard Palethorpe, ltp

Hi Xu,

> Hi Petr
> > Hi Xu,

> >>> This is not working as expected in Github CI. I'm still looking at the
> >>> problem.
> >>> https://github.com/wangli5665/ltp/runs/4602025797?check_suite_focus=true
> >> I tested local but it works well. I guess ci fails because of linux user
> >> namespace. Maybe we should require CAP_SYS_RESOURCE cap instead of using
> >> geteuid.
> > Good catch, verifying.
> You can refer to my ltp fork
> https://github.com/xuyang0410/ltp/commits/oom_kill_ci_fixes
Ah, I didn't noticed your work, we both worked on the same thing.
I added check on both places (for the cleanup as well) and check also
CAP_SYS_ADMIN (obviously CAP_SYS_RESOURCE would be enough).

Kind regards,
Petr

> Best Regards
> Yang Xu

> > Kind regards,
> > Petr

> >> Best Regards
> >> Yang Xu

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22 10:24               ` Petr Vorel
@ 2021-12-22 10:40                 ` Cyril Hrubis
  2021-12-22 10:48                   ` Petr Vorel
  2021-12-22 11:25                   ` Li Wang
  0 siblings, 2 replies; 16+ messages in thread
From: Cyril Hrubis @ 2021-12-22 10:40 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Richard Palethorpe, ltp

Hi!
> We use just FILE_PRINTF(), but we check the result and TWARN, which causes the
> failure in CI.

Which is why I argued that we should add a version that does not print
anything and just returns if the operation was successful or not. I
guess it could be called TRY_FILE_PRINTF() or something.

> I've sent v2, which checks CAP_SYS_ADMIN and CAP_SYS_RESOURCE,
> but feel free just to bring simpler solution.

I still think that the most acurate test would be just writing to the
file and checking the result.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22 10:40                 ` Cyril Hrubis
@ 2021-12-22 10:48                   ` Petr Vorel
  2021-12-22 11:29                     ` Li Wang
  2021-12-22 11:25                   ` Li Wang
  1 sibling, 1 reply; 16+ messages in thread
From: Petr Vorel @ 2021-12-22 10:48 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: Richard Palethorpe, ltp

> Hi!
> > We use just FILE_PRINTF(), but we check the result and TWARN, which causes the
> > failure in CI.

> Which is why I argued that we should add a version that does not print
> anything and just returns if the operation was successful or not. I
> guess it could be called TRY_FILE_PRINTF() or something.
OK, got that.

> > I've sent v2, which checks CAP_SYS_ADMIN and CAP_SYS_RESOURCE,
> > but feel free just to bring simpler solution.

> I still think that the most acurate test would be just writing to the
> file and checking the result.
OK. Anybody taking this (so that not more people working on it)?

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22 10:40                 ` Cyril Hrubis
  2021-12-22 10:48                   ` Petr Vorel
@ 2021-12-22 11:25                   ` Li Wang
  1 sibling, 0 replies; 16+ messages in thread
From: Li Wang @ 2021-12-22 11:25 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 1057 bytes --]

Hi Cyril, Petr, Xu,

On Wed, Dec 22, 2021 at 6:39 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > We use just FILE_PRINTF(), but we check the result and TWARN, which
> causes the
> > failure in CI.
>
> Which is why I argued that we should add a version that does not print
> anything and just returns if the operation was successful or not. I
> guess it could be called TRY_FILE_PRINTF() or something.
>

or SILENT_FILE_PRINTF()?



>
> > I've sent v2, which checks CAP_SYS_ADMIN and CAP_SYS_RESOURCE,
> > but feel free just to bring simpler solution.
>
> I still think that the most acurate test would be just writing to the
> file and checking the result.
>

Yes, this method is simpler.

But checking the CAP_SYS_RESOURCE cap can show the
original problem more clearly, people read code easily to know
why here we do that cap check.
(e.g. like me cost much time debugging but not know the root cause)

So I think both of your solutions are great. Can we add the
silent file_printf into lib? I think it is useful to LTP as well.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2724 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22 10:48                   ` Petr Vorel
@ 2021-12-22 11:29                     ` Li Wang
  2021-12-22 11:32                       ` Li Wang
  0 siblings, 1 reply; 16+ messages in thread
From: Li Wang @ 2021-12-22 11:29 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp, Richard Palethorpe


[-- Attachment #1.1: Type: text/plain, Size: 435 bytes --]

On Wed, Dec 22, 2021 at 6:48 PM Petr Vorel <pvorel@suse.cz> wrote:

> > I've sent v2, which checks CAP_SYS_ADMIN and CAP_SYS_RESOURCE,
> > > but feel free just to bring simpler solution.
>
> > I still think that the most acurate test would be just writing to the
> > file and checking the result.
> OK. Anybody taking this (so that not more people working on it)?


I'm considering using both of the two methods.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 1067 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root
  2021-12-22 11:29                     ` Li Wang
@ 2021-12-22 11:32                       ` Li Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Li Wang @ 2021-12-22 11:32 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp, Richard Palethorpe


[-- Attachment #1.1: Type: text/plain, Size: 610 bytes --]

On Wed, Dec 22, 2021 at 7:29 PM Li Wang <liwang@redhat.com> wrote:

>
>
> On Wed, Dec 22, 2021 at 6:48 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > I've sent v2, which checks CAP_SYS_ADMIN and CAP_SYS_RESOURCE,
>> > > but feel free just to bring simpler solution.
>>
>> > I still think that the most acurate test would be just writing to the
>> > file and checking the result.
>> OK. Anybody taking this (so that not more people working on it)?
>
>
> I'm considering using both of the two methods.
>

Or now just go with Petr's patch to get rid of the fails before
Christmas vacation:).

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 1628 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-12-22 11:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-21 19:35 [LTP] [PATCH 1/1] lib: Skip tst_{disable, enable}_oom_protection() for non-root Petr Vorel
2021-12-22  2:25 ` xuyang2018.jy
2021-12-22  3:32   ` Li Wang
2021-12-22  6:05     ` xuyang2018.jy
2021-12-22  8:14       ` Petr Vorel
2021-12-22  8:37         ` xuyang2018.jy
2021-12-22  9:48           ` Li Wang
2021-12-22 10:09             ` Cyril Hrubis
2021-12-22 10:24               ` Petr Vorel
2021-12-22 10:40                 ` Cyril Hrubis
2021-12-22 10:48                   ` Petr Vorel
2021-12-22 11:29                     ` Li Wang
2021-12-22 11:32                       ` Li Wang
2021-12-22 11:25                   ` Li Wang
2021-12-22 10:38           ` Petr Vorel
2021-12-22  8:06     ` Petr Vorel

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.