Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1407545 > unrolled thread

[PATCH 4/6] mm, oom: skip over vforked tasks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/6] mm, oom: skip over vforked tasks Michal Hocko <mhocko@kernel.org> - 2016-05-26 14:50 +0200
    Re: [PATCH 4/6] mm, oom: skip over vforked tasks Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-05-27 18:50 +0200
      Re: [PATCH 4/6] mm, oom: skip over vforked tasks Michal Hocko <mhocko@kernel.org> - 2016-05-30 09:20 +0200
        Re: [PATCH 4/6] mm, oom: skip over vforked tasks Michal Hocko <mhocko@kernel.org> - 2016-05-30 12:00 +0200
          Re: [PATCH 4/6] mm, oom: skip over vforked tasks Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-05-30 12:50 +0200
            Re: [PATCH 4/6] mm, oom: skip over vforked tasks Michal Hocko <mhocko@kernel.org> - 2016-05-30 13:00 +0200
    Re: [PATCH 4/6] mm, oom: skip over vforked tasks Michal Hocko <mhocko@kernel.org> - 2016-05-30 14:10 +0200

#1407545 — [PATCH 4/6] mm, oom: skip over vforked tasks

From: Michal Hocko <mhocko@suse.com>

vforked tasks are not really sitting on memory so it doesn't matter much
to kill them. Parents are waiting for vforked task killable so it is
better to chose parent which is the real mm owner. Teach oom_badness
to ignore all tasks which haven't passed mm_release. oom_kill_process
should ignore them as well because they will drop the mm soon and they
will not block oom_reaper because they cannot touch any memory.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index eeccb4d7e7f5..d1cbaaa1a666 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -176,11 +176,13 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
 
 	/*
 	 * Do not even consider tasks which are explicitly marked oom
-	 * unkillable or have been already oom reaped.
+	 * unkillable or have been already oom reaped or they are in
+	 * the middle of vfork
 	 */
 	adj = (long)p->signal->oom_score_adj;
 	if (adj == OOM_SCORE_ADJ_MIN ||
-			test_bit(MMF_OOM_REAPED, &p->mm->flags)) {
+			test_bit(MMF_OOM_REAPED, &p->mm->flags) ||
+			p->vfork_done) {
 		task_unlock(p);
 		return 0;
 	}
@@ -839,6 +841,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	for_each_process(p) {
 		if (!process_shares_mm(p, mm))
 			continue;
+		/*
+		 * vforked tasks are ignored because they will drop the mm soon
+		 * hopefully and even if not they will not mind being oom
+		 * reaped because they cannot touch any memory.
+		 */
+		if (p->vfork_done)
+			continue;
 		if (same_thread_group(p, victim))
 			continue;
 		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p) ||
-- 
2.8.1

[toc] | [next] | [standalone]

#1408181

On Thu, May 26, 2016 at 02:40:13PM +0200, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> vforked tasks are not really sitting on memory so it doesn't matter much
> to kill them. Parents are waiting for vforked task killable so it is
> better to chose parent which is the real mm owner. Teach oom_badness
> to ignore all tasks which haven't passed mm_release. oom_kill_process
> should ignore them as well because they will drop the mm soon and they
> will not block oom_reaper because they cannot touch any memory.

That is, if a process calls vfork->exec to spawn a child, and a newly
spawned child happens to invoke oom somewhere in exec, instead of
killing the child, which hasn't done anything yet, we'll kill the main
process while the child continues to run. Not sure if it's really bad
though.

...
> @@ -839,6 +841,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	for_each_process(p) {
>  		if (!process_shares_mm(p, mm))
>  			continue;
> +		/*
> +		 * vforked tasks are ignored because they will drop the mm soon
> +		 * hopefully and even if not they will not mind being oom
> +		 * reaped because they cannot touch any memory.

They shouldn't modify memory, but they still can touch it AFAIK.

> +		 */
> +		if (p->vfork_done)
> +			continue;
>  		if (same_thread_group(p, victim))
>  			continue;
>  		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p) ||

[toc] | [prev] | [next] | [standalone]

#1408817

On Fri 27-05-16 19:48:30, Vladimir Davydov wrote:
> On Thu, May 26, 2016 at 02:40:13PM +0200, Michal Hocko wrote:
[...]
> > @@ -839,6 +841,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> >  	for_each_process(p) {
> >  		if (!process_shares_mm(p, mm))
> >  			continue;
> > +		/*
> > +		 * vforked tasks are ignored because they will drop the mm soon
> > +		 * hopefully and even if not they will not mind being oom
> > +		 * reaped because they cannot touch any memory.
> 
> They shouldn't modify memory, but they still can touch it AFAIK.

You are right. This means that the vforked child might see zero pages.
Let me think whether this is acceptable or not.

Thanks!

> 
> > +		 */
> > +		if (p->vfork_done)
> > +			continue;
> >  		if (same_thread_group(p, victim))
> >  			continue;
> >  		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p) ||

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1408963

On Mon 30-05-16 09:13:57, Michal Hocko wrote:
> On Fri 27-05-16 19:48:30, Vladimir Davydov wrote:
> > On Thu, May 26, 2016 at 02:40:13PM +0200, Michal Hocko wrote:
> [...]
> > > @@ -839,6 +841,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> > >  	for_each_process(p) {
> > >  		if (!process_shares_mm(p, mm))
> > >  			continue;
> > > +		/*
> > > +		 * vforked tasks are ignored because they will drop the mm soon
> > > +		 * hopefully and even if not they will not mind being oom
> > > +		 * reaped because they cannot touch any memory.
> > 
> > They shouldn't modify memory, but they still can touch it AFAIK.
> 
> You are right. This means that the vforked child might see zero pages.
> Let me think whether this is acceptable or not.

OK, I was thinking about it some more and I think you have a good point
here. I can see two options here:
- keep vforked task alive and skip the oom reaper. If the victim exits
  normally and the oom wouldn't get resolved the vforked task will be
  selected in the next round because the victim would clean up
  vfork_done state in  wait_for_vfork_done. We are still risking that
  the victim gets stuck though
- kill vforked task and so it would be reapable.

The later sounds more robust to me because we invoke the oom_reaper and
the side effect shouldn't be really a problem because the vforked task
couldn't have done a lot of useful work anyway. So I will drop this
patch and update "mm, oom: fortify task_will_free_mem" to skip the
the vfork check as well.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1408979

On Mon, May 30, 2016 at 11:52:12AM +0200, Michal Hocko wrote:
> On Mon 30-05-16 09:13:57, Michal Hocko wrote:
> > On Fri 27-05-16 19:48:30, Vladimir Davydov wrote:
> > > On Thu, May 26, 2016 at 02:40:13PM +0200, Michal Hocko wrote:
> > [...]
> > > > @@ -839,6 +841,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> > > >  	for_each_process(p) {
> > > >  		if (!process_shares_mm(p, mm))
> > > >  			continue;
> > > > +		/*
> > > > +		 * vforked tasks are ignored because they will drop the mm soon
> > > > +		 * hopefully and even if not they will not mind being oom
> > > > +		 * reaped because they cannot touch any memory.
> > > 
> > > They shouldn't modify memory, but they still can touch it AFAIK.
> > 
> > You are right. This means that the vforked child might see zero pages.
> > Let me think whether this is acceptable or not.
> 
> OK, I was thinking about it some more and I think you have a good point
> here. I can see two options here:
> - keep vforked task alive and skip the oom reaper. If the victim exits
>   normally and the oom wouldn't get resolved the vforked task will be
>   selected in the next round because the victim would clean up
>   vfork_done state in  wait_for_vfork_done. We are still risking that
>   the victim gets stuck though
> - kill vforked task and so it would be reapable.

IMHO it all depends on what we're trying to achieve. If we want per task
oom, which could make some sense since a task can consume a lot of mem
via e.g. pipe buffers, we would go with option #1. However, it's rather
difficult to find out how much of kmem a task consumes w/o using kmemcg,
so IMHO per-mm approach makes more sense in general. In this case I
think we should kill both vforked task and its parent if their mm was
selected provided their oom_score_adj allows that.

> 
> The later sounds more robust to me because we invoke the oom_reaper and
> the side effect shouldn't be really a problem because the vforked task
> couldn't have done a lot of useful work anyway. So I will drop this
> patch and update "mm, oom: fortify task_will_free_mem" to skip the
> the vfork check as well.

[toc] | [prev] | [next] | [standalone]

#1408983

On Mon 30-05-16 13:40:17, Vladimir Davydov wrote:
> On Mon, May 30, 2016 at 11:52:12AM +0200, Michal Hocko wrote:
> > On Mon 30-05-16 09:13:57, Michal Hocko wrote:
> > > On Fri 27-05-16 19:48:30, Vladimir Davydov wrote:
> > > > On Thu, May 26, 2016 at 02:40:13PM +0200, Michal Hocko wrote:
> > > [...]
> > > > > @@ -839,6 +841,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> > > > >  	for_each_process(p) {
> > > > >  		if (!process_shares_mm(p, mm))
> > > > >  			continue;
> > > > > +		/*
> > > > > +		 * vforked tasks are ignored because they will drop the mm soon
> > > > > +		 * hopefully and even if not they will not mind being oom
> > > > > +		 * reaped because they cannot touch any memory.
> > > > 
> > > > They shouldn't modify memory, but they still can touch it AFAIK.
> > > 
> > > You are right. This means that the vforked child might see zero pages.
> > > Let me think whether this is acceptable or not.
> > 
> > OK, I was thinking about it some more and I think you have a good point
> > here. I can see two options here:
> > - keep vforked task alive and skip the oom reaper. If the victim exits
> >   normally and the oom wouldn't get resolved the vforked task will be
> >   selected in the next round because the victim would clean up
> >   vfork_done state in  wait_for_vfork_done. We are still risking that
> >   the victim gets stuck though
> > - kill vforked task and so it would be reapable.
> 
> IMHO it all depends on what we're trying to achieve. If we want per task
> oom, which could make some sense since a task can consume a lot of mem
> via e.g. pipe buffers, we would go with option #1. However, it's rather
> difficult to find out how much of kmem a task consumes w/o using kmemcg,
> so IMHO per-mm approach makes more sense in general. In this case I
> think we should kill both vforked task and its parent if their mm was
> selected provided their oom_score_adj allows that.

Yes agreed. Going with per-mm is a safier behavior because the vast
majority of the consumed memory should be per mm not per task_struct.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1409035

So I've ended up with a replacement for this patch which does the
following:
---
From c40900923c78b51215794cc445d3f5a589b8f785 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 30 May 2016 13:53:28 +0200
Subject: [PATCH] mm, oom: skip vforked tasks from being selected

vforked tasks are not really sitting on any memory. They are sharing
the mm with parent until they exec into a new code. Until then it is
just pinning the address space. OOM killer will kill the vforked task
along with its parent but we still can end up selecting vforked task
when the parent wouldn't be selected. E.g. init doing vfork to launch
a task or vforked being a child of oom unkillable task with an updated
oom_score_adj to be killable.

Make sure to not select vforked task as an oom victim by checking
vfork_done in oom_badness.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7ba6bdf9ae94..36c821403d0f 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -176,11 +176,13 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
 
 	/*
 	 * Do not even consider tasks which are explicitly marked oom
-	 * unkillable or have been already oom reaped.
+	 * unkillable or have been already oom reaped or the are in
+	 * the middle of vfork
 	 */
 	adj = (long)p->signal->oom_score_adj;
 	if (adj == OOM_SCORE_ADJ_MIN ||
-			test_bit(MMF_OOM_REAPED, &p->mm->flags)) {
+			test_bit(MMF_OOM_REAPED, &p->mm->flags) ||
+			p->vfork_done) {
 		task_unlock(p);
 		return 0;
 	}
-- 
2.8.1

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web