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

Groups > linux.kernel > #1407548 > unrolled thread

[PATCH 0/5] Handle oom bypass more gracefully

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] Handle oom bypass more gracefully Michal Hocko <mhocko@kernel.org> - 2016-05-26 14:50 +0200
    [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj Michal Hocko <mhocko@kernel.org> - 2016-05-26 14:50 +0200
      Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Michal Hocko <mhocko@kernel.org> - 2016-05-27 13:20 +0200
        Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-05-27 18:20 +0200
          Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Michal Hocko <mhocko@kernel.org> - 2016-05-30 09:10 +0200
            Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-05-30 11:30 +0200
              Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Michal Hocko <mhocko@kernel.org> - 2016-05-30 11:50 +0200
                Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-05-30 12:50 +0200
                  Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Michal Hocko <mhocko@kernel.org> - 2016-05-30 13:20 +0200
                    Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-05-30 14:30 +0200
                      Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have  same view of oom_score_adj Michal Hocko <mhocko@kernel.org> - 2016-05-30 14:30 +0200
    [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm Michal Hocko <mhocko@kernel.org> - 2016-05-26 14:50 +0200
      Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-05-26 16:40 +0200
        Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no  external tasks sharing mm Michal Hocko <mhocko@kernel.org> - 2016-05-26 17:00 +0200
          Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are noexternal tasks sharing mm Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-05-26 17:30 +0200
            Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are  noexternal tasks sharing mm Michal Hocko <mhocko@kernel.org> - 2016-05-26 17:40 +0200
              Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-05-26 18:20 +0200
                Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no  external tasks sharing mm Michal Hocko <mhocko@kernel.org> - 2016-05-27 08:50 +0200
                  Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no  external tasks sharing mm Michal Hocko <mhocko@kernel.org> - 2016-05-27 09:20 +0200
                    Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no  external tasks sharing mm Michal Hocko <mhocko@kernel.org> - 2016-05-27 10:10 +0200
    [PATCH 2/6] proc, oom_adj: extract oom_score_adj setting into a helper Michal Hocko <mhocko@kernel.org> - 2016-05-26 14:50 +0200
    Re: [PATCH 0/5] Handle oom bypass more gracefully Michal Hocko <mhocko@kernel.org> - 2016-05-27 18:10 +0200

Page 1 of 2  [1] 2  Next page →

#1407548 — [PATCH 0/5] Handle oom bypass more gracefully

Hi,
the following 6 patches should put some order to very rare cases of
mm shared between processes and make the paths which bypass the oom
killer oom reapable and so much more reliable finally.  Even though mm
shared outside of threadgroup is rare (either use_mm by kernel threads
or exotic clone(CLONE_VM) without CLONE_THREAD resp. CLONE_SIGHAND) it
makes the current oom killer logic quite hard to follow and evaluate. It
is possible to select an oom victim which shares the mm with unkillable
process or bypass the oom killer even when other processes sharing the
mm are still alive and other weird cases.

Patch 1 optimizes oom_kill_task to skip the costly process
iteration when the current oom victim is not sharing mm with other
processes. Patch 2 is a clean up of oom_score_adj handling and a
preparatory work. Patch 3 enforces oom_adj_score to be consistent
between processes sharing the mm to behave consistently with the regular
thread groups. Patch 4 tries to handle vforked tasks better in the oom
path, patch 5 ensures that all tasks sharing the mm are killed and
finally patch 6 should guarantee that task_will_free_mem will always
imply reapable bypass of the oom killer.

The patchset is based on the current mmotm tree (mmotm-2016-05-23-16-51).
I would really appreciate a deep review as this area is full of land
mines but I hope I've made the code much cleaner with less kludges.

I am CCing Oleg (sorry I know you hate this code) but I would feel much
better if you double checked my assumptions about locking and vfork
behavior.

Michal Hocko (6):
      mm, oom: do not loop over all tasks if there are no external tasks sharing mm
      proc, oom_adj: extract oom_score_adj setting into a helper
      mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj
      mm, oom: skip over vforked tasks
      mm, oom: kill all tasks sharing the mm
      mm, oom: fortify task_will_free_mem

 fs/proc/base.c      | 168 +++++++++++++++++++++++++++++-----------------------
 include/linux/mm.h  |   2 +
 include/linux/oom.h |  72 ++++++++++++++++++++--
 mm/memcontrol.c     |   4 +-
 mm/oom_kill.c       |  96 ++++++++++--------------------
 5 files changed, 196 insertions(+), 146 deletions(-)

[toc] | [next] | [standalone]

#1407549 — [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

From: Michal Hocko <mhocko@suse.com>

oom_score_adj is shared for the thread groups (via struct signal) but
this is not sufficient to cover processes sharing mm (CLONE_VM without
CLONE_THREAD resp. CLONE_SIGHAND) and so we can easily end up in a
situation when some processes update their oom_score_adj and confuse
the oom killer. In the worst case some of those processes might hide
from oom killer altogether via OOM_SCORE_ADJ_MIN while others are
eligible. OOM killer would then pick up those eligible but won't be
allowed to kill others sharing the same mm so the mm wouldn't release
the mm and so the memory.

It would be ideal to have the oom_score_adj per mm_struct becuase that
is the natural entity OOM killer considers. But this will not work
because some programs are doing
	vfork()
	set_oom_adj()
	exec()

We can achieve the same though. oom_score_adj write handler can set the
oom_score_adj for all processes sharing the same mm if the task is not
in the middle of vfork. As a result all the processes will share the
same oom_score_adj.

Note that we have to serialize all the oom_score_adj writers now to
guarantee they do not interleave and generate inconsistent results.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/proc/base.c     | 35 +++++++++++++++++++++++++++++++++++
 include/linux/mm.h |  2 ++
 mm/oom_kill.c      |  2 +-
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 23679673bf5a..e3ee4fb1930c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1043,10 +1043,13 @@ static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
 
 static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 {
+	static DEFINE_MUTEX(oom_adj_mutex);
+	struct mm_struct *mm = NULL;
 	struct task_struct *task;
 	unsigned long flags;
 	int err = 0;
 
+	mutex_lock(&oom_adj_mutex);
 	task = get_proc_task(file_inode(file));
 	if (!task) {
 		err = -ESRCH;
@@ -1085,6 +1088,20 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 		}
 	}
 
+	/*
+	 * If we are not in the vfork and share mm with other processes we
+	 * have to propagate the score otherwise we would have a schizophrenic
+	 * requirements for the same mm. We can use racy check because we
+	 * only risk the slow path.
+	 */
+	if (!task->vfork_done &&
+			atomic_read(&task->mm->mm_users) > get_nr_threads(task)) {
+		mm = task->mm;
+
+		/* pin the mm so it doesn't go away and get reused */
+		atomic_inc(&mm->mm_count);
+	}
+
 	task->signal->oom_score_adj = oom_adj;
 	if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
 		task->signal->oom_score_adj_min = (short)oom_adj;
@@ -1094,7 +1111,25 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 err_task_lock:
 	task_unlock(task);
 	put_task_struct(task);
+
+	if (mm) {
+		struct task_struct *p;
+
+		rcu_read_lock();
+		for_each_process(p) {
+			task_lock(p);
+			if (!p->vfork_done && process_shares_mm(p, mm)) {
+				p->signal->oom_score_adj = oom_adj;
+				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
+					p->signal->oom_score_adj_min = (short)oom_adj;
+			}
+			task_unlock(p);
+		}
+		rcu_read_unlock();
+		mmdrop(mm);
+	}
 out:
+	mutex_unlock(&oom_adj_mutex);
 	return err;
 }
 
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 05102822912c..b44d3d792a00 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2248,6 +2248,8 @@ static inline int in_gate_area(struct mm_struct *mm, unsigned long addr)
 }
 #endif	/* __HAVE_ARCH_GATE_AREA */
 
+extern bool process_shares_mm(struct task_struct *p, struct mm_struct *mm);
+
 #ifdef CONFIG_SYSCTL
 extern int sysctl_drop_caches;
 int drop_caches_sysctl_handler(struct ctl_table *, int,
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0e33e912f7e4..eeccb4d7e7f5 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -416,7 +416,7 @@ bool oom_killer_disabled __read_mostly;
  * task's threads: if one of those is using this mm then this task was also
  * using it.
  */
-static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
+bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
 {
 	struct task_struct *t;
 
-- 
2.8.1

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

#1408012 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

And here again. Get rid of the mm_users check because it is not
reliable.
---
From 7681e91cba6bcd45f9ebc5d2dcee3df06c687296 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 25 May 2016 19:50:34 +0200
Subject: [PATCH] mm, oom_adj: make sure processes sharing mm have same view of
 oom_score_adj

oom_score_adj is shared for the thread groups (via struct signal) but
this is not sufficient to cover processes sharing mm (CLONE_VM without
CLONE_THREAD resp. CLONE_SIGHAND) and so we can easily end up in a
situation when some processes update their oom_score_adj and confuse
the oom killer. In the worst case some of those processes might hide
from oom killer altogether via OOM_SCORE_ADJ_MIN while others are
eligible. OOM killer would then pick up those eligible but won't be
allowed to kill others sharing the same mm so the mm wouldn't release
the mm and so the memory.

It would be ideal to have the oom_score_adj per mm_struct becuase that
is the natural entity OOM killer considers. But this will not work
because some programs are doing
	vfork()
	set_oom_adj()
	exec()

We can achieve the same though. oom_score_adj write handler can set the
oom_score_adj for all processes sharing the same mm if the task is not
in the middle of vfork. As a result all the processes will share the
same oom_score_adj. The current implementation is rather pessimistic
and checks all the existing processes by default if there are more than
1 holder of the mm but we do not have any reliable way to check for
external users yet.

Note that we have to serialize all the oom_score_adj writers now to
guarantee they do not interleave and generate inconsistent results.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/proc/base.c     | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/mm.h |  2 ++
 mm/oom_kill.c      |  2 +-
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 0afc77d4d84a..fa0b3ca94dfb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1043,10 +1043,13 @@ static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
 
 static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 {
+	static DEFINE_MUTEX(oom_adj_mutex);
+	struct mm_struct *mm = NULL;
 	struct task_struct *task;
 	unsigned long flags;
 	int err = 0;
 
+	mutex_lock(&oom_adj_mutex);
 	task = get_proc_task(file_inode(file));
 	if (!task) {
 		err = -ESRCH;
@@ -1079,6 +1082,21 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 		}
 	}
 
+	/*
+	 * Make sure we will check other processes sharing the mm if this is
+	 * not vfrok which wants its own oom_score_adj.
+	 * pin the mm so it doesn't go away and get reused.
+	 */
+	if (!task->vfork_done) {
+		struct task_struct *p = find_lock_task_mm(task);
+
+		if (p && atomic_read(&p->mm->mm_users) > 1) {
+			mm = p->mm;
+			atomic_inc(&mm->mm_count);
+			task_unlock(p);
+		}
+	}
+
 	task->signal->oom_score_adj = oom_adj;
 	if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
 		task->signal->oom_score_adj_min = (short)oom_adj;
@@ -1087,7 +1105,25 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 	unlock_task_sighand(task, &flags);
 err_put_task:
 	put_task_struct(task);
+
+	if (mm) {
+		struct task_struct *p;
+
+		rcu_read_lock();
+		for_each_process(p) {
+			task_lock(p);
+			if (!p->vfork_done && process_shares_mm(p, mm)) {
+				p->signal->oom_score_adj = oom_adj;
+				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
+					p->signal->oom_score_adj_min = (short)oom_adj;
+			}
+			task_unlock(p);
+		}
+		rcu_read_unlock();
+		mmdrop(mm);
+	}
 out:
+	mutex_unlock(&oom_adj_mutex);
 	return err;
 }
 
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 05102822912c..b44d3d792a00 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2248,6 +2248,8 @@ static inline int in_gate_area(struct mm_struct *mm, unsigned long addr)
 }
 #endif	/* __HAVE_ARCH_GATE_AREA */
 
+extern bool process_shares_mm(struct task_struct *p, struct mm_struct *mm);
+
 #ifdef CONFIG_SYSCTL
 extern int sysctl_drop_caches;
 int drop_caches_sysctl_handler(struct ctl_table *, int,
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 1685890d424e..268b76b88220 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -416,7 +416,7 @@ bool oom_killer_disabled __read_mostly;
  * task's threads: if one of those is using this mm then this task was also
  * using it.
  */
-static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
+bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
 {
 	struct task_struct *t;
 
-- 
2.8.1

-- 
Michal Hocko
SUSE Labs

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

#1408168 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Fri, May 27, 2016 at 01:18:03PM +0200, Michal Hocko wrote:
...
> @@ -1087,7 +1105,25 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
>  	unlock_task_sighand(task, &flags);
>  err_put_task:
>  	put_task_struct(task);
> +
> +	if (mm) {
> +		struct task_struct *p;
> +
> +		rcu_read_lock();
> +		for_each_process(p) {
> +			task_lock(p);
> +			if (!p->vfork_done && process_shares_mm(p, mm)) {
> +				p->signal->oom_score_adj = oom_adj;
> +				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
> +					p->signal->oom_score_adj_min = (short)oom_adj;
> +			}
> +			task_unlock(p);

I.e. you write to /proc/pid1/oom_score_adj and get
/proc/pid2/oom_score_adj updated if pid1 and pid2 share mm?
IMO that looks unexpected from userspace pov.

May be, we'd better add mm->oom_score_adj and set it to the min
signal->oom_score_adj over all processes sharing it? This would
require iterating over all processes every time oom_score_adj gets
updated, but that's a slow path.

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

#1408809 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Fri 27-05-16 19:18:21, Vladimir Davydov wrote:
> On Fri, May 27, 2016 at 01:18:03PM +0200, Michal Hocko wrote:
> ...
> > @@ -1087,7 +1105,25 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> >  	unlock_task_sighand(task, &flags);
> >  err_put_task:
> >  	put_task_struct(task);
> > +
> > +	if (mm) {
> > +		struct task_struct *p;
> > +
> > +		rcu_read_lock();
> > +		for_each_process(p) {
> > +			task_lock(p);
> > +			if (!p->vfork_done && process_shares_mm(p, mm)) {
> > +				p->signal->oom_score_adj = oom_adj;
> > +				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
> > +					p->signal->oom_score_adj_min = (short)oom_adj;
> > +			}
> > +			task_unlock(p);
> 
> I.e. you write to /proc/pid1/oom_score_adj and get
> /proc/pid2/oom_score_adj updated if pid1 and pid2 share mm?
> IMO that looks unexpected from userspace pov.

How much different it is from threads in the same thread group?
Processes sharing the mm without signals is a rather weird threading
model isn't it? Currently we just lie to users about their oom_score_adj
in this weird corner case. The only exception was OOM_SCORE_ADJ_MIN
where we really didn't kill the task but all other values are simply
ignored in practice.

> May be, we'd better add mm->oom_score_adj and set it to the min
> signal->oom_score_adj over all processes sharing it? This would
> require iterating over all processes every time oom_score_adj gets
> updated, but that's a slow path.

Not sure I understand. So you would prefer that mm->oom_score_adj might
disagree with p->signal->oom_score_adj?

-- 
Michal Hocko
SUSE Labs

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

#1408935 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Mon, May 30, 2016 at 09:07:05AM +0200, Michal Hocko wrote:
> On Fri 27-05-16 19:18:21, Vladimir Davydov wrote:
> > On Fri, May 27, 2016 at 01:18:03PM +0200, Michal Hocko wrote:
> > ...
> > > @@ -1087,7 +1105,25 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > >  	unlock_task_sighand(task, &flags);
> > >  err_put_task:
> > >  	put_task_struct(task);
> > > +
> > > +	if (mm) {
> > > +		struct task_struct *p;
> > > +
> > > +		rcu_read_lock();
> > > +		for_each_process(p) {
> > > +			task_lock(p);
> > > +			if (!p->vfork_done && process_shares_mm(p, mm)) {
> > > +				p->signal->oom_score_adj = oom_adj;
> > > +				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
> > > +					p->signal->oom_score_adj_min = (short)oom_adj;
> > > +			}
> > > +			task_unlock(p);
> > 
> > I.e. you write to /proc/pid1/oom_score_adj and get
> > /proc/pid2/oom_score_adj updated if pid1 and pid2 share mm?
> > IMO that looks unexpected from userspace pov.
> 
> How much different it is from threads in the same thread group?
> Processes sharing the mm without signals is a rather weird threading
> model isn't it?

I think so too. I wouldn't be surprised if it turned out that nobody had
ever used it. But may be there's someone out there who does.

> Currently we just lie to users about their oom_score_adj
> in this weird corner case.

Hmm, looks like a bug, but nobody has ever complained about it.

> The only exception was OOM_SCORE_ADJ_MIN
> where we really didn't kill the task but all other values are simply
> ignored in practice.
> 
> > May be, we'd better add mm->oom_score_adj and set it to the min
> > signal->oom_score_adj over all processes sharing it? This would
> > require iterating over all processes every time oom_score_adj gets
> > updated, but that's a slow path.
> 
> Not sure I understand. So you would prefer that mm->oom_score_adj might
> disagree with p->signal->oom_score_adj?

No, I wouldn't. I'd rather agree that oom_score_adj should be per mm,
because we choose the victim basing solely on mm stats.

What I mean is we don't touch p->signal->oom_score_adj of other tasks
sharing mm, but instead store minimal oom_score_adj over all tasks
sharing mm in the mm_struct whenever a task's oom_score_adj is modified.
And use mm->oom_score_adj instead of signal->oom_score_adj in oom killer
code. This would save us from any accusations of user API modifications
and it would also make the oom code a bit easier to follow IMHO.

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

#1408948 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Mon 30-05-16 11:47:53, Vladimir Davydov wrote:
> On Mon, May 30, 2016 at 09:07:05AM +0200, Michal Hocko wrote:
> > On Fri 27-05-16 19:18:21, Vladimir Davydov wrote:
> > > On Fri, May 27, 2016 at 01:18:03PM +0200, Michal Hocko wrote:
> > > ...
> > > > @@ -1087,7 +1105,25 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > >  	unlock_task_sighand(task, &flags);
> > > >  err_put_task:
> > > >  	put_task_struct(task);
> > > > +
> > > > +	if (mm) {
> > > > +		struct task_struct *p;
> > > > +
> > > > +		rcu_read_lock();
> > > > +		for_each_process(p) {
> > > > +			task_lock(p);
> > > > +			if (!p->vfork_done && process_shares_mm(p, mm)) {
> > > > +				p->signal->oom_score_adj = oom_adj;
> > > > +				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
> > > > +					p->signal->oom_score_adj_min = (short)oom_adj;
> > > > +			}
> > > > +			task_unlock(p);
> > > 
> > > I.e. you write to /proc/pid1/oom_score_adj and get
> > > /proc/pid2/oom_score_adj updated if pid1 and pid2 share mm?
> > > IMO that looks unexpected from userspace pov.
> > 
> > How much different it is from threads in the same thread group?
> > Processes sharing the mm without signals is a rather weird threading
> > model isn't it?
> 
> I think so too. I wouldn't be surprised if it turned out that nobody had
> ever used it. But may be there's someone out there who does.

I have heard some rumors about users. But I haven't heard anything about
their oom_score_adj usage patterns.

> > Currently we just lie to users about their oom_score_adj
> > in this weird corner case.
> 
> Hmm, looks like a bug, but nobody has ever complained about it.

Yes and that leads me to a suspicion that we can do that. Maybe I should
just add a note into the log that we are doing that so that people can
complain? Something like the following
diff --git a/fs/proc/base.c b/fs/proc/base.c
index fa0b3ca94dfb..7f3495415719 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1104,7 +1104,6 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 err_sighand:
 	unlock_task_sighand(task, &flags);
 err_put_task:
-	put_task_struct(task);
 
 	if (mm) {
 		struct task_struct *p;
@@ -1113,6 +1112,10 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 		for_each_process(p) {
 			task_lock(p);
 			if (!p->vfork_done && process_shares_mm(p, mm)) {
+				pr_info("updating oom_score_adj for %d (%s) from %d to %d because it shares mm with %d (%s). Report if this is unexpected.\n",
+						task_pid_nr(p), p->comm,
+						p->signal->oom_score_adj, oom_adj,
+						task_pid_nr(task), task->comm);
 				p->signal->oom_score_adj = oom_adj;
 				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
 					p->signal->oom_score_adj_min = (short)oom_adj;
@@ -1122,6 +1125,7 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 		rcu_read_unlock();
 		mmdrop(mm);
 	}
+	put_task_struct(task);
 out:
 	mutex_unlock(&oom_adj_mutex);
 	return err;

> > The only exception was OOM_SCORE_ADJ_MIN
> > where we really didn't kill the task but all other values are simply
> > ignored in practice.
> > 
> > > May be, we'd better add mm->oom_score_adj and set it to the min
> > > signal->oom_score_adj over all processes sharing it? This would
> > > require iterating over all processes every time oom_score_adj gets
> > > updated, but that's a slow path.
> > 
> > Not sure I understand. So you would prefer that mm->oom_score_adj might
> > disagree with p->signal->oom_score_adj?
> 
> No, I wouldn't. I'd rather agree that oom_score_adj should be per mm,
> because we choose the victim basing solely on mm stats.
> 
> What I mean is we don't touch p->signal->oom_score_adj of other tasks
> sharing mm, but instead store minimal oom_score_adj over all tasks
> sharing mm in the mm_struct whenever a task's oom_score_adj is modified.
> And use mm->oom_score_adj instead of signal->oom_score_adj in oom killer
> code. This would save us from any accusations of user API modifications
> and it would also make the oom code a bit easier to follow IMHO.

I understand your point but this is essentially lying because we
consider a different value than the user can observe in userspace.
Consider somebody doing insanity like

current->oom_score_adj = OOM_SCORE_ADJ_MIN
p = clone(CLONE_VM)
p->oom_score_adj = OOM_SCORE_ADJ_MAX

so one process would want to be always selected while the other one
doesn't want to get killed. All they can see is that everything is
put in place until the oom killer comes over and ignores that.

I think we should just be explicit. Maybe we want to treat
OOM_SCORE_ADJ_MIN special - e.g. do not even try to set oom_score_adj if
one of the sharing tasks is oom disabled. But I would rather wait for
somebody to complain and explain why the usecase really makes sense than
be all silent with implicit behavior.

Btw. we have already had per mm oom_core_adj but we had to revert it due
to vfork behavior. See 0753ba01e126 ("mm: revert "oom: move oom_adj
value""). This patch gets us back except it handles the vfork issue.
-- 
Michal Hocko
SUSE Labs

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

#1408978 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Mon, May 30, 2016 at 11:39:50AM +0200, Michal Hocko wrote:
> On Mon 30-05-16 11:47:53, Vladimir Davydov wrote:
> > On Mon, May 30, 2016 at 09:07:05AM +0200, Michal Hocko wrote:
> > > On Fri 27-05-16 19:18:21, Vladimir Davydov wrote:
> > > > On Fri, May 27, 2016 at 01:18:03PM +0200, Michal Hocko wrote:
> > > > ...
> > > > > @@ -1087,7 +1105,25 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > > >  	unlock_task_sighand(task, &flags);
> > > > >  err_put_task:
> > > > >  	put_task_struct(task);
> > > > > +
> > > > > +	if (mm) {
> > > > > +		struct task_struct *p;
> > > > > +
> > > > > +		rcu_read_lock();
> > > > > +		for_each_process(p) {
> > > > > +			task_lock(p);
> > > > > +			if (!p->vfork_done && process_shares_mm(p, mm)) {
> > > > > +				p->signal->oom_score_adj = oom_adj;
> > > > > +				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
> > > > > +					p->signal->oom_score_adj_min = (short)oom_adj;
> > > > > +			}
> > > > > +			task_unlock(p);
> > > > 
> > > > I.e. you write to /proc/pid1/oom_score_adj and get
> > > > /proc/pid2/oom_score_adj updated if pid1 and pid2 share mm?
> > > > IMO that looks unexpected from userspace pov.
> > > 
> > > How much different it is from threads in the same thread group?
> > > Processes sharing the mm without signals is a rather weird threading
> > > model isn't it?
> > 
> > I think so too. I wouldn't be surprised if it turned out that nobody had
> > ever used it. But may be there's someone out there who does.
> 
> I have heard some rumors about users. But I haven't heard anything about
> their oom_score_adj usage patterns.
> 
> > > Currently we just lie to users about their oom_score_adj
> > > in this weird corner case.
> > 
> > Hmm, looks like a bug, but nobody has ever complained about it.
> 
> Yes and that leads me to a suspicion that we can do that. Maybe I should
> just add a note into the log that we are doing that so that people can
> complain? Something like the following
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index fa0b3ca94dfb..7f3495415719 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1104,7 +1104,6 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
>  err_sighand:
>  	unlock_task_sighand(task, &flags);
>  err_put_task:
> -	put_task_struct(task);
>  
>  	if (mm) {
>  		struct task_struct *p;
> @@ -1113,6 +1112,10 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
>  		for_each_process(p) {
>  			task_lock(p);
>  			if (!p->vfork_done && process_shares_mm(p, mm)) {
> +				pr_info("updating oom_score_adj for %d (%s) from %d to %d because it shares mm with %d (%s). Report if this is unexpected.\n",
> +						task_pid_nr(p), p->comm,
> +						p->signal->oom_score_adj, oom_adj,
> +						task_pid_nr(task), task->comm);

IMO this could be acceptable from userspace pov, but I don't very much
like how vfork is special-cased here and in oom killer code.

>  				p->signal->oom_score_adj = oom_adj;
>  				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
>  					p->signal->oom_score_adj_min = (short)oom_adj;
> @@ -1122,6 +1125,7 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
>  		rcu_read_unlock();
>  		mmdrop(mm);
>  	}
> +	put_task_struct(task);
>  out:
>  	mutex_unlock(&oom_adj_mutex);
>  	return err;
> 
> > > The only exception was OOM_SCORE_ADJ_MIN
> > > where we really didn't kill the task but all other values are simply
> > > ignored in practice.
> > > 
> > > > May be, we'd better add mm->oom_score_adj and set it to the min
> > > > signal->oom_score_adj over all processes sharing it? This would
> > > > require iterating over all processes every time oom_score_adj gets
> > > > updated, but that's a slow path.
> > > 
> > > Not sure I understand. So you would prefer that mm->oom_score_adj might
> > > disagree with p->signal->oom_score_adj?
> > 
> > No, I wouldn't. I'd rather agree that oom_score_adj should be per mm,
> > because we choose the victim basing solely on mm stats.
> > 
> > What I mean is we don't touch p->signal->oom_score_adj of other tasks
> > sharing mm, but instead store minimal oom_score_adj over all tasks
> > sharing mm in the mm_struct whenever a task's oom_score_adj is modified.
> > And use mm->oom_score_adj instead of signal->oom_score_adj in oom killer
> > code. This would save us from any accusations of user API modifications
> > and it would also make the oom code a bit easier to follow IMHO.
> 
> I understand your point but this is essentially lying because we
> consider a different value than the user can observe in userspace.
> Consider somebody doing insanity like
> 
> current->oom_score_adj = OOM_SCORE_ADJ_MIN
> p = clone(CLONE_VM)
> p->oom_score_adj = OOM_SCORE_ADJ_MAX
> 
> so one process would want to be always selected while the other one
> doesn't want to get killed. All they can see is that everything is
> put in place until the oom killer comes over and ignores that.

If we stored minimal oom_score_adj in mm struct, oom killer wouldn't
kill any of these processes, and it looks fine to me as long as we want
oom killer to be mm based, not task or signal_struct based.

Come to think of it, it'd be difficult to keep mm->oom_score_adj in sync
with p->signal->oom_score_adj, because we would need to update
mm->oom_score_adj not only on /proc write, but also on fork. May be, we
could keep all signal_structs sharing mm linked in per mm list so that
we could quickly update mm->oom_score_adj on fork? That way we wouldn't
need to special case vfork.

> 
> I think we should just be explicit. Maybe we want to treat
> OOM_SCORE_ADJ_MIN special - e.g. do not even try to set oom_score_adj if
> one of the sharing tasks is oom disabled. But I would rather wait for
> somebody to complain and explain why the usecase really makes sense than
> be all silent with implicit behavior.
> 
> Btw. we have already had per mm oom_core_adj but we had to revert it due
> to vfork behavior. See 0753ba01e126 ("mm: revert "oom: move oom_adj
> value""). This patch gets us back except it handles the vfork issue.

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

#1408990 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Mon 30-05-16 13:26:44, Vladimir Davydov wrote:
> On Mon, May 30, 2016 at 11:39:50AM +0200, Michal Hocko wrote:
[...]
> > Yes and that leads me to a suspicion that we can do that. Maybe I should
> > just add a note into the log that we are doing that so that people can
> > complain? Something like the following
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index fa0b3ca94dfb..7f3495415719 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1104,7 +1104,6 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> >  err_sighand:
> >  	unlock_task_sighand(task, &flags);
> >  err_put_task:
> > -	put_task_struct(task);
> >  
> >  	if (mm) {
> >  		struct task_struct *p;
> > @@ -1113,6 +1112,10 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> >  		for_each_process(p) {
> >  			task_lock(p);
> >  			if (!p->vfork_done && process_shares_mm(p, mm)) {
> > +				pr_info("updating oom_score_adj for %d (%s) from %d to %d because it shares mm with %d (%s). Report if this is unexpected.\n",
> > +						task_pid_nr(p), p->comm,
> > +						p->signal->oom_score_adj, oom_adj,
> > +						task_pid_nr(task), task->comm);
> 
> IMO this could be acceptable from userspace pov, but I don't very much
> like how vfork is special-cased here and in oom killer code.

Well, the vfork has to be special cased here. We definitely have to
support
	vfork()
	set_oom_score_adj()
	exec()

use case. And I do not see other way without adding something to the
clone hot paths which sounds like not justifiable considering we are
talking about a really rare usecase that basically nobody cares about.
 
[...]
> > so one process would want to be always selected while the other one
> > doesn't want to get killed. All they can see is that everything is
> > put in place until the oom killer comes over and ignores that.
> 
> If we stored minimal oom_score_adj in mm struct, oom killer wouldn't
> kill any of these processes, and it looks fine to me as long as we want
> oom killer to be mm based, not task or signal_struct based.
> 
> Come to think of it, it'd be difficult to keep mm->oom_score_adj in sync
> with p->signal->oom_score_adj, because we would need to update
> mm->oom_score_adj not only on /proc write, but also on fork. May be, we
> could keep all signal_structs sharing mm linked in per mm list so that
> we could quickly update mm->oom_score_adj on fork? That way we wouldn't
> need to special case vfork.

Yes the current approach is slightly racy but I do not see that would
matter all that much. What you are suggesting might work but I am not
really sure we want to complicate the whole thing now. Sure if we see
that those races are real we can try to find a better solution, but I
would like to start as easy as possible and placing all the logic into
the oom_score_adj proc handler sounds like a good spot to me.
-- 
Michal Hocko
SUSE Labs

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

#1409053 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Mon, May 30, 2016 at 01:11:48PM +0200, Michal Hocko wrote:
> On Mon 30-05-16 13:26:44, Vladimir Davydov wrote:
> > On Mon, May 30, 2016 at 11:39:50AM +0200, Michal Hocko wrote:
> [...]
> > > Yes and that leads me to a suspicion that we can do that. Maybe I should
> > > just add a note into the log that we are doing that so that people can
> > > complain? Something like the following
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index fa0b3ca94dfb..7f3495415719 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -1104,7 +1104,6 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > >  err_sighand:
> > >  	unlock_task_sighand(task, &flags);
> > >  err_put_task:
> > > -	put_task_struct(task);
> > >  
> > >  	if (mm) {
> > >  		struct task_struct *p;
> > > @@ -1113,6 +1112,10 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > >  		for_each_process(p) {
> > >  			task_lock(p);
> > >  			if (!p->vfork_done && process_shares_mm(p, mm)) {
> > > +				pr_info("updating oom_score_adj for %d (%s) from %d to %d because it shares mm with %d (%s). Report if this is unexpected.\n",
> > > +						task_pid_nr(p), p->comm,
> > > +						p->signal->oom_score_adj, oom_adj,
> > > +						task_pid_nr(task), task->comm);
> > 
> > IMO this could be acceptable from userspace pov, but I don't very much
> > like how vfork is special-cased here and in oom killer code.
> 
> Well, the vfork has to be special cased here. We definitely have to
> support
> 	vfork()
> 	set_oom_score_adj()
> 	exec()
> 
> use case. And I do not see other way without adding something to the
> clone hot paths which sounds like not justifiable considering we are
> talking about a really rare usecase that basically nobody cares about.

I don't think that vfork->exec use case is rare. Quite the contrary, I'm
pretty sure it's used often, because in contrast to fork->exec it avoids
copying page tables, which can be very expensive for fat processes.

Frankly, I don't understand why you are so determined not to add
anything to the fork path. Of course, if the overhead were that
dramatic, we would have to forget the idea, but if it were say <= 0.1 %
for a contrived test that calls fork in a loop, IMHO the modification
would be justified.

>  
> [...]
> > > so one process would want to be always selected while the other one
> > > doesn't want to get killed. All they can see is that everything is
> > > put in place until the oom killer comes over and ignores that.
> > 
> > If we stored minimal oom_score_adj in mm struct, oom killer wouldn't
> > kill any of these processes, and it looks fine to me as long as we want
> > oom killer to be mm based, not task or signal_struct based.
> > 
> > Come to think of it, it'd be difficult to keep mm->oom_score_adj in sync
> > with p->signal->oom_score_adj, because we would need to update
> > mm->oom_score_adj not only on /proc write, but also on fork. May be, we
> > could keep all signal_structs sharing mm linked in per mm list so that
> > we could quickly update mm->oom_score_adj on fork? That way we wouldn't
> > need to special case vfork.
> 
> Yes the current approach is slightly racy but I do not see that would
> matter all that much. What you are suggesting might work but I am not
> really sure we want to complicate the whole thing now. Sure if we see
> that those races are real we can try to find a better solution, but I
> would like to start as easy as possible and placing all the logic into
> the oom_score_adj proc handler sounds like a good spot to me.

IMHO with all those p->vfork_done and p->signal_struct->oom_score_adj
checks the oom code is becoming more difficult to understand. Linking
signal_struct in a per mm list would probably require more code, but
IMHO it would be easier to follow.

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

#1409055 — Re: [PATCH 3/6] mm, oom_adj: make sure processes sharing mm have same view of oom_score_adj

On Mon 30-05-16 15:19:32, Vladimir Davydov wrote:
> On Mon, May 30, 2016 at 01:11:48PM +0200, Michal Hocko wrote:
> > On Mon 30-05-16 13:26:44, Vladimir Davydov wrote:
> > > On Mon, May 30, 2016 at 11:39:50AM +0200, Michal Hocko wrote:
> > [...]
> > > > Yes and that leads me to a suspicion that we can do that. Maybe I should
> > > > just add a note into the log that we are doing that so that people can
> > > > complain? Something like the following
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index fa0b3ca94dfb..7f3495415719 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -1104,7 +1104,6 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > >  err_sighand:
> > > >  	unlock_task_sighand(task, &flags);
> > > >  err_put_task:
> > > > -	put_task_struct(task);
> > > >  
> > > >  	if (mm) {
> > > >  		struct task_struct *p;
> > > > @@ -1113,6 +1112,10 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > >  		for_each_process(p) {
> > > >  			task_lock(p);
> > > >  			if (!p->vfork_done && process_shares_mm(p, mm)) {
> > > > +				pr_info("updating oom_score_adj for %d (%s) from %d to %d because it shares mm with %d (%s). Report if this is unexpected.\n",
> > > > +						task_pid_nr(p), p->comm,
> > > > +						p->signal->oom_score_adj, oom_adj,
> > > > +						task_pid_nr(task), task->comm);
> > > 
> > > IMO this could be acceptable from userspace pov, but I don't very much
> > > like how vfork is special-cased here and in oom killer code.
> > 
> > Well, the vfork has to be special cased here. We definitely have to
> > support
> > 	vfork()
> > 	set_oom_score_adj()
> > 	exec()
> > 
> > use case. And I do not see other way without adding something to the
> > clone hot paths which sounds like not justifiable considering we are
> > talking about a really rare usecase that basically nobody cares about.
> 
> I don't think that vfork->exec use case is rare. Quite the contrary, I'm
> pretty sure it's used often, because in contrast to fork->exec it avoids
> copying page tables, which can be very expensive for fat processes.

Ohh, yes, the way I put it is ambiguous. What I wanted to say is that
the oom is really unlikely so it doesn't justify hot path changes.

> Frankly, I don't understand why you are so determined not to add
> anything to the fork path.

It is not just the fork path. It would require touching exit path as
well and all that code is quite complex already. I would prefer if the
oom related complexity stay in the oom proper.

> Of course, if the overhead were that
> dramatic, we would have to forget the idea, but if it were say <= 0.1 %
> for a contrived test that calls fork in a loop, IMHO the modification
> would be justified.

But why if the proc handler resp. oom_kill_process paths can handle most
cases and the occasional races should be tolerate able AFAICS.
-- 
Michal Hocko
SUSE Labs

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

#1407551 — [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm

From: Michal Hocko <mhocko@suse.com>

oom_kill_process makes sure to kill all processes outside of the thread
group which are sharing the mm. This requires to iterate over all tasks.
This is however not a common case so we can optimize it a bit and only
do that path only if we know that there are external users of the mm
struct outside of the thread group.

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

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 5bb2f7698ad7..0e33e912f7e4 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -820,6 +820,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	task_unlock(victim);
 
 	/*
+	 * skip expensive iterations over all tasks if we know that there
+	 * are no users outside of threads in the same thread group
+	 */
+	if (atomic_read(&mm->mm_users) <= get_nr_threads(victim))
+		goto oom_reap;
+
+	/*
 	 * Kill all user processes sharing victim->mm in other thread groups, if
 	 * any.  They don't get access to memory reserves, though, to avoid
 	 * depletion of all memory.  This prevents mm->mmap_sem livelock when an
@@ -848,6 +855,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	}
 	rcu_read_unlock();
 
+oom_reap:
 	if (can_oom_reap)
 		wake_oom_reaper(victim);
 
-- 
2.8.1

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

#1407583 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm

Michal Hocko wrote:
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 5bb2f7698ad7..0e33e912f7e4 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -820,6 +820,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	task_unlock(victim);
>  
>  	/*
> +	 * skip expensive iterations over all tasks if we know that there
> +	 * are no users outside of threads in the same thread group
> +	 */
> +	if (atomic_read(&mm->mm_users) <= get_nr_threads(victim))
> +		goto oom_reap;

Is this really safe? Isn't it possible that victim thread's thread group has
more than atomic_read(&mm->mm_users) threads which are past exit_mm() and blocked
at exit_task_work() which are before __exit_signal() from release_task() from
exit_notify()?

> +
> +	/*
>  	 * Kill all user processes sharing victim->mm in other thread groups, if
>  	 * any.  They don't get access to memory reserves, though, to avoid
>  	 * depletion of all memory.  This prevents mm->mmap_sem livelock when an

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

#1407590 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm

On Thu 26-05-16 23:30:06, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 5bb2f7698ad7..0e33e912f7e4 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -820,6 +820,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> >  	task_unlock(victim);
> >  
> >  	/*
> > +	 * skip expensive iterations over all tasks if we know that there
> > +	 * are no users outside of threads in the same thread group
> > +	 */
> > +	if (atomic_read(&mm->mm_users) <= get_nr_threads(victim))
> > +		goto oom_reap;
> 
> Is this really safe? Isn't it possible that victim thread's thread group has
> more than atomic_read(&mm->mm_users) threads which are past exit_mm() and blocked
> at exit_task_work() which are before __exit_signal() from release_task() from
> exit_notify()?

You are right. The race window between exit_mm and __exit_signal is
really large. I thought about == check instead but that wouldn't work
for the same reason, dang, it looked so promissing.

Scratch this patch then.

-- 
Michal Hocko
SUSE Labs

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

#1407597 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are noexternal tasks sharing mm

Michal Hocko wrote:
> On Thu 26-05-16 23:30:06, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index 5bb2f7698ad7..0e33e912f7e4 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -820,6 +820,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> > >  	task_unlock(victim);
> > >  
> > >  	/*
> > > +	 * skip expensive iterations over all tasks if we know that there
> > > +	 * are no users outside of threads in the same thread group
> > > +	 */
> > > +	if (atomic_read(&mm->mm_users) <= get_nr_threads(victim))
> > > +		goto oom_reap;
> > 
> > Is this really safe? Isn't it possible that victim thread's thread group has
> > more than atomic_read(&mm->mm_users) threads which are past exit_mm() and blocked
> > at exit_task_work() which are before __exit_signal() from release_task() from
> > exit_notify()?
> 
> You are right. The race window between exit_mm and __exit_signal is
> really large. I thought about == check instead but that wouldn't work
> for the same reason, dang, it looked so promissing.
> 
> Scratch this patch then.
> 

I think that remembering whether this mm might be shared between
multiple thread groups at clone() time (i.e. whether
clone(CLONE_VM without CLONE_SIGHAND) was ever requested on this mm)
is safe (given that that thread already got SIGKILL or is exiting).

By the way, in oom_kill_process(), how (p->flags & PF_KTHREAD) case can
become true when process_shares_mm() is true? Even if it can become true,
why can't we reap that mm? Is (p->flags & PF_KTHREAD) case only for
not to send SIGKILL rather than not to reap that mm?

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

#1407602 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are noexternal tasks sharing mm

On Fri 27-05-16 00:25:23, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Thu 26-05-16 23:30:06, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > index 5bb2f7698ad7..0e33e912f7e4 100644
> > > > --- a/mm/oom_kill.c
> > > > +++ b/mm/oom_kill.c
> > > > @@ -820,6 +820,13 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> > > >  	task_unlock(victim);
> > > >  
> > > >  	/*
> > > > +	 * skip expensive iterations over all tasks if we know that there
> > > > +	 * are no users outside of threads in the same thread group
> > > > +	 */
> > > > +	if (atomic_read(&mm->mm_users) <= get_nr_threads(victim))
> > > > +		goto oom_reap;
> > > 
> > > Is this really safe? Isn't it possible that victim thread's thread group has
> > > more than atomic_read(&mm->mm_users) threads which are past exit_mm() and blocked
> > > at exit_task_work() which are before __exit_signal() from release_task() from
> > > exit_notify()?
> > 
> > You are right. The race window between exit_mm and __exit_signal is
> > really large. I thought about == check instead but that wouldn't work
> > for the same reason, dang, it looked so promissing.
> > 
> > Scratch this patch then.
> > 
> 
> I think that remembering whether this mm might be shared between
> multiple thread groups at clone() time (i.e. whether
> clone(CLONE_VM without CLONE_SIGHAND) was ever requested on this mm)
> is safe (given that that thread already got SIGKILL or is exiting).

I was already playing with that idea but I didn't want to add anything
to the fork path which is really hot. This patch is not really needed
for the rest. It just felt like a nice optimization. I do not think it
is worth deeper changes in the fast paths.

> By the way, in oom_kill_process(), how (p->flags & PF_KTHREAD) case can
> become true when process_shares_mm() is true?

not sure I understand. But the PF_KTHREAD check is there to catch
use_mm() usage by kernel threads.

> Even if it can become true,
> why can't we reap that mm? Is (p->flags & PF_KTHREAD) case only for
> not to send SIGKILL rather than not to reap that mm?

If we reaped the mm then the kernel thread could blow up when accessing
a memory.

-- 
Michal Hocko
SUSE Labs

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

#1407619 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm

Michal Hocko wrote:
> On Fri 27-05-16 00:25:23, Tetsuo Handa wrote:
> > I think that remembering whether this mm might be shared between
> > multiple thread groups at clone() time (i.e. whether
> > clone(CLONE_VM without CLONE_SIGHAND) was ever requested on this mm)
> > is safe (given that that thread already got SIGKILL or is exiting).
> 
> I was already playing with that idea but I didn't want to add anything
> to the fork path which is really hot. This patch is not really needed
> for the rest. It just felt like a nice optimization. I do not think it
> is worth deeper changes in the fast paths.

"[PATCH 6/6] mm, oom: fortify task_will_free_mem" depends on [PATCH 1/6].
You will need to update [PATCH 6/6].

It seems to me that [PATCH 6/6] resembles
http://lkml.kernel.org/r/201605250005.GHH26082.JOtQOSLMFFOFVH@I-love.SAKURA.ne.jp .
I think we will be happy if we can speed up mm_is_reapable() test using
"whether this mm might be shared between multiple thread groups" flag.
I don't think updating such flag at clone() is too heavy operation to add.

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

#1407849 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm

On Fri 27-05-16 01:14:35, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Fri 27-05-16 00:25:23, Tetsuo Handa wrote:
> > > I think that remembering whether this mm might be shared between
> > > multiple thread groups at clone() time (i.e. whether
> > > clone(CLONE_VM without CLONE_SIGHAND) was ever requested on this mm)
> > > is safe (given that that thread already got SIGKILL or is exiting).
> > 
> > I was already playing with that idea but I didn't want to add anything
> > to the fork path which is really hot. This patch is not really needed
> > for the rest. It just felt like a nice optimization. I do not think it
> > is worth deeper changes in the fast paths.
> 
> "[PATCH 6/6] mm, oom: fortify task_will_free_mem" depends on [PATCH 1/6].
> You will need to update [PATCH 6/6].
> 
> It seems to me that [PATCH 6/6] resembles
> http://lkml.kernel.org/r/201605250005.GHH26082.JOtQOSLMFFOFVH@I-love.SAKURA.ne.jp .
> I think we will be happy if we can speed up mm_is_reapable() test using
> "whether this mm might be shared between multiple thread groups" flag.
> I don't think updating such flag at clone() is too heavy operation to add.

It is still an operation which is not needed for 99% of situations. So
if we do not need it for correctness then I do not think this is worth
bothering.

-- 
Michal Hocko
SUSE Labs

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

#1407860 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm

On Fri 27-05-16 08:45:10, Michal Hocko wrote:
[...]
> It is still an operation which is not needed for 99% of situations. So
> if we do not need it for correctness then I do not think this is worth
> bothering.

Since you have pointed out exit_mm vs. __exit_signal race yesterday I
was thinking how to make the check reliable. Even
atomic_read(mm->mm_users) > get_nr_threads() is not reliable and we can
miss other tasks just because the current thread group is mostly past
exit_mm. So far I couldn't find a way to tweak this around though.
I will think about it more but I am afraid that a flag would be really
needed afterall.
-- 
Michal Hocko
SUSE Labs

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

#1407895 — Re: [PATCH 1/6] mm, oom: do not loop over all tasks if there are no external tasks sharing mm

On Fri 27-05-16 09:15:07, Michal Hocko wrote:
> On Fri 27-05-16 08:45:10, Michal Hocko wrote:
> [...]
> > It is still an operation which is not needed for 99% of situations. So
> > if we do not need it for correctness then I do not think this is worth
> > bothering.
> 
> Since you have pointed out exit_mm vs. __exit_signal race yesterday I
> was thinking how to make the check reliable. Even
> atomic_read(mm->mm_users) > get_nr_threads() is not reliable and we can
> miss other tasks just because the current thread group is mostly past
> exit_mm. So far I couldn't find a way to tweak this around though.

Just for the record I was playing with the following yesterday but I
couldn't convince myself that this is safe and reasonable in the first
place (I do not like it to be honest).
---
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 1685890d424e..db027eca8be5 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -123,6 +123,35 @@ struct task_struct *find_lock_task_mm(struct task_struct *p)
 	return t;
 }
 
+bool task_has_external_users(struct task_struct *p)
+{
+	struct mm_struct *mm = NULL;
+	struct task_struct *t;
+	int active_threads = 0;
+	bool ret = true;	/* be pessimistic */
+
+	rcu_read_lock();
+	for_each_thread(p, t) {
+		task_lock(t);
+		if (likely(t->mm)) {
+			active_threads++;
+			if (!mm) {
+				mm = t->mm;
+				atomic_inc(&mm->mm_count);
+			}
+		}
+		task_unlock(t);
+	}
+	rcu_read_unlock();
+
+	if (mm) {
+		if (atomic_read(&mm->mm_users) <= active_threads)
+			ret = false;
+		mmdrop(mm);
+	}
+	return ret;
+}
+
 /*
  * order == -1 means the oom kill is required by sysrq, otherwise only
  * for display purposes.
-- 
Michal Hocko
SUSE Labs

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web