oom killer: break from infinite loop

In multi-threading environment, if the current task(A) have got the mm->mmap_sem semaphore, and the thread(B) in the same process is selected to be oom killed, because they shares the same semaphore, thread B can not really be killed. So __alloc_pages_slowpath turns to be a infinite loop. Here set all the threads in the group to TIF_MEMDIE, it gets a chance to break and exit.

Signed-off-by Anfei Zhou <[email protected]>

---
 mm/oom_kill.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

mm/oom_kill.c +4 -0

index 9b223af..aab9892 100644
@@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
  */
 static void __oom_kill_task(struct task_struct *p, int verbose)
 {
+	struct task_struct *t;
+
 	if (is_global_init(p)) {
 		WARN_ON(1);
 		printk(KERN_WARNING "tried to kill init!\n");

@@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose)
 	 */
 	p->rt.time_slice = HZ;
 	set_tsk_thread_flag(p, TIF_MEMDIE);
+	for (t = next_thread(p); t != p; t = next_thread(t))
+		set_tsk_thread_flag(t, TIF_MEMDIE);
 
 	force_sig(SIGKILL, p);
 }

Signature · 7 lines

1.6.4.rc1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

In multi-threading environment, if the current task(A) have got the mm->mmap_sem semaphore, and the thread(B) in the same process is selected to be oom killed, because they shares the same semaphore, thread B can not really be killed. So __alloc_pages_slowpath turns to be a infinite loop. Here set all the threads in the group to TIF_MEMDIE, it gets a chance to break and exit.

Signed-off-by Anfei Zhou <[email protected]>

---
 mm/oom_kill.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

mm/oom_kill.c +2 -0

index 9b223af..aab9892 100644
@@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
  */
 static void __oom_kill_task(struct task_struct *p, int verbose)
 {
+	struct task_struct *t;
+
 	if (is_global_init(p)) {
 		WARN_ON(1);
 		printk(KERN_WARNING "tried to kill init!\n");

Quoted 18 lines · On Thu, 25 Mar 2010 00:25:05 +0800 Anfei Zhou <[email protected]> wrote:

mm/oom_kill.c +4 -0

@@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
  */
 static void __oom_kill_task(struct task_struct *p, int verbose)
 {
+	struct task_struct *t;
+
 	if (is_global_init(p)) {
 		WARN_ON(1);
 		printk(KERN_WARNING "tried to kill init!\n");

@@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose)
 	 */
 	p->rt.time_slice = HZ;
 	set_tsk_thread_flag(p, TIF_MEMDIE);
+	for (t = next_thread(p); t != p; t = next_thread(t))
+		set_tsk_thread_flag(t, TIF_MEMDIE);
 

mm/oom_kill.c static void __oom_kill_task(struct task_struct *p, int verbose) · line 420

Quoted 1 line

mm/oom_kill.c +0 -0

 	force_sig(SIGKILL, p);

Don't we need some sort of locking while walking that ring?

Quoted 22 lines · Andrew Morton

Quoted 18 lines · On Thu, 25 Mar 2010 00:25:05 +0800 Anfei Zhou <[email protected]> wrote:

mm/oom_kill.c +4 -0

@@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
  */
 static void __oom_kill_task(struct task_struct *p, int verbose)
 {
+	struct task_struct *t;
+
 	if (is_global_init(p)) {
 		WARN_ON(1);
 		printk(KERN_WARNING "tried to kill init!\n");

@@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose)
 	 */
 	p->rt.time_slice = HZ;
 	set_tsk_thread_flag(p, TIF_MEMDIE);
+	for (t = next_thread(p); t != p; t = next_thread(t))
+		set_tsk_thread_flag(t, TIF_MEMDIE);
 

Quoted 3 lines

mm/oom_kill.c static void __oom_kill_task(struct task_struct *p, int verbose) · line 420

Quoted 1 line

mm/oom_kill.c +0 -0

 	force_sig(SIGKILL, p);

Don't we need some sort of locking while walking that ring?

This should be always called under tasklist_lock, I think. At least this seems to be true in Linus's tree.

I'd suggest to do

- set_tsk_thread_flag(p, TIF_MEMDIE);
+ t = p;
+ do {
+ set_tsk_thread_flag(t, TIF_MEMDIE);
+ } while_each_thread(p, t);

but this is matter of taste.

Off-topic, but we shouldn't use force_sig(), SIGKILL doesn't need "force" semantics.

I'd wish I could understand the changelog ;)

Quoted 3 lines · Oleg Nesterov

Off-topic, but we shouldn't use force_sig(), SIGKILL doesn't need "force" semantics.

This may need a dedicated patch, there are some other places to force_sig(SIGKILL, ...) too.

Quoted 2 lines

I'd wish I could understand the changelog ;)

Assume thread A and B are in the same group. If A runs into the oom, and selects B as the victim, B won't exit because at least in exit_mm(), it can not get the mm->mmap_sem semaphore which A has already got.

I see. But still I can't understand. To me, the problem is not that B can't exit, the problem is that A doesn't know it should exit. All threads should exit and free ->mm. Even if B could exit, this is not enough. And, to some extent, it doesn't matter if it holds mmap_sem or not.

Don't get me wrong. Even if I don't understand oom_kill.c the patch looks obviously good to me, even from "common sense" pov. I am just curious.

So, my understanding is: we are going to kill the whole thread group but TIF_MEMDIE is per-thread. Mark the whole thread group as TIF_MEMDIE so that any thread can notice this flag and (say, __alloc_pages_slowpath) fail asap.

Is my understanding correct?

The problem with this approach is that we could easily deplete all memory reserves if the oom killed task has an extremely large number of threads, there has always been only a single thread with TIF_MEMDIE set per cpuset or memcg; for systems that don't run with cpusets or memory controller, this has been limited to one thread with TIF_MEMDIE for the entire system.

There's risk involved with suddenly allowing 1000 threads to have TIF_MEMDIE set and the chances of fully depleting all allowed zones is much higher if they allocate memory prior to exit, for example.

An alternative is to fail allocations if they are failable and the allocating task has a pending SIGKILL. It's better to preempt the oom killer since current is going to be exiting anyway and this avoids a needless kill.

That's possible if it's guaranteed that __GFP_NOFAIL allocations with a pending SIGKILL are granted ALLOC_NO_WATERMARKS to prevent them from endlessly looping while making no progress.

Comments?

---

mm/page_alloc.c +17 -11

@@ -1610,13 +1610,21 @@ try_next_zone:
 }
 
 static inline int
-should_alloc_retry(gfp_t gfp_mask, unsigned int order,
+should_alloc_retry(struct task_struct *p, gfp_t gfp_mask, unsigned int order,
 				unsigned long pages_reclaimed)
 {
 	/* Do not loop if specifically requested */
 	if (gfp_mask & __GFP_NORETRY)
 		return 0;
 
+	/* Loop if specifically requested */
+	if (gfp_mask & __GFP_NOFAIL)
+		return 1;
+
+	/* Task is killed, fail the allocation if possible */
+	if (fatal_signal_pending(p))
+		return 0;
+
 	/*
 	 * In this implementation, order <= PAGE_ALLOC_COSTLY_ORDER
 	 * means __GFP_NOFAIL, but that may not be true in other

@@ -1635,13 +1643,6 @@ should_alloc_retry(gfp_t gfp_mask, unsigned int order,
 	if (gfp_mask & __GFP_REPEAT && pages_reclaimed < (1 << order))
 		return 1;
 
-	/*
-	 * Don't let big-order allocations loop unless the caller
-	 * explicitly requests that.
-	 */
-	if (gfp_mask & __GFP_NOFAIL)
-		return 1;
-
 	return 0;
 }
 

@@ -1798,6 +1799,7 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
 	if (likely(!(gfp_mask & __GFP_NOMEMALLOC))) {
 		if (!in_interrupt() &&
 		    ((p->flags & PF_MEMALLOC) ||
+		     (fatal_signal_pending(p) && (gfp_mask & __GFP_NOFAIL)) ||
 		     unlikely(test_thread_flag(TIF_MEMDIE))))
 			alloc_flags |= ALLOC_NO_WATERMARKS;
 	}

@@ -1812,6 +1814,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	int migratetype)
 {
 	const gfp_t wait = gfp_mask & __GFP_WAIT;
+	const gfp_t nofail = gfp_mask & __GFP_NOFAIL;
 	struct page *page = NULL;
 	int alloc_flags;
 	unsigned long pages_reclaimed = 0;

@@ -1876,7 +1879,7 @@ rebalance:
 		goto nopage;
 
 	/* Avoid allocations with no watermarks from looping endlessly */
-	if (test_thread_flag(TIF_MEMDIE) && !(gfp_mask & __GFP_NOFAIL))
+	if (test_thread_flag(TIF_MEMDIE) && !nofail)
 		goto nopage;
 
 	/* Try direct reclaim and then allocating */

@@ -1888,6 +1891,10 @@ rebalance:
 	if (page)
 		goto got_pg;
 
+	/* Task is killed, fail the allocation if possible */
+	if (fatal_signal_pending(p) && !nofail)
+		goto nopage;
+
 	/*
 	 * If we failed to make any progress reclaiming, then we are
 	 * running out of options and have to consider going OOM

@@ -1909,8 +1916,7 @@ rebalance:
 			 * made, there are no other options and retrying is
 			 * unlikely to help.
 			 */
-			if (order > PAGE_ALLOC_COSTLY_ORDER &&
-						!(gfp_mask & __GFP_NOFAIL))
+			if (order > PAGE_ALLOC_COSTLY_ORDER && !nofail)
 				goto nopage;
 
 			goto restart;

Can't comment, I do not understand these subtleties.

But I'd like to note that fatal_signal_pending() can be true when the process wasn't killed, but another thread does exit_group/exec.

mm/oom_kill.c void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, · line 689

Quoted 12 lines

mm/oom_kill.c +6 -0

@@ -681,6 +681,16 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
 	}
 
 	/*
+	 * If current has a pending SIGKILL, then automatically select it.  The
+	 * goal is to allow it to allocate so that it may quickly exit and free
+	 * its memory.
+	 */
+	if (fatal_signal_pending(current)) {
+		__oom_kill_task(current);

I am worried...

Note that __oom_kill_task() does force_sig(SIGKILL) which assumes that ->sighand != NULL. This is not true if out_of_memory() is called after current has already passed exit_notify().

Hmm. looking at oom_kill.c... Afaics there are more problems with mt apllications. select_bad_process() does for_each_process() which can only see the group leaders. This is fine, but what if ->group_leader has already exited? In this case its ->mm == NULL, and we ignore the whole thread group.

IOW, unless I missed something, it is very easy to hide the process from oom-kill:

	int main()
	{
		pthread_create(memory_hog_func);
		syscall(__NR_exit);
	}

Quoted 3 lines · Oleg Nesterov

Note that __oom_kill_task() does force_sig(SIGKILL) which assumes that ->sighand != NULL. This is not true if out_of_memory() is called after current has already passed exit_notify().

We have an even bigger problem if current is in the oom killer at exit_notify() since it has already detached its ->mm in exit_mm() :)

Quoted 9 lines

IOW, unless I missed something, it is very easy to hide the process from oom-kill:

	int main()
	{
		pthread_create(memory_hog_func);
		syscall(__NR_exit);
	}

The check for !p->mm was moved in the -mm tree (and the oom killer was entirely rewritten in that tree, so I encourage you to work off of it instead

with oom-avoid-race-for-oom-killed-tasks-detaching-mm-prior-to-exit.patch to even after the check for PF_EXITING. This is set in the exit path before the ->mm is detached

so if the oom killer finds an already exiting task,
it will become a no-op since it should eventually free memory and avoids a needless oom kill.

OK, but I guess this !p->mm check is still wrong for the same reason. In fact I do not understand why it is needed in select_bad_process() right before oom_badness() which checks ->mm too (and this check is equally wrong).

Probably something like the patch below makes sense. Note that "skip kernel threads" logic is wrong too, we should check PF_KTHREAD. Probably it is better to check it in select_bad_process() instead, near is_global_init().

The new helper, find_lock_task_mm(), should be used by oom_forkbomb_penalty() too.

dump_tasks() doesn't need it, it does do_each_thread(). Cough, __out_of_memory() and out_of_memory() call it without tasklist. We are going to panic() anyway, but still.

Oleg.

mm/oom_kill.c +15 -6

@@ -129,6 +129,19 @@ static unsigned long oom_forkbomb_penalt
 				(child_rss / sysctl_oom_forkbomb_thres) : 0;
 }
 
+static find_lock_task_mm(struct task_struct *p)
+{
+	struct task_struct *t = p;
+	do {
+		task_lock(t);
+		if (likely(t->mm && !(t->flags & PF_KTHREAD)))
+			return t;
+		task_unlock(t);
+	} while_each_thred(p, t);
+
+	return NULL;
+}
+
 /**
  * oom_badness - heuristic function to determine which candidate task to kill
  * @p: task struct of which task we should calculate

@@ -159,13 +172,9 @@ unsigned int oom_badness(struct task_str
 	if (p->flags & PF_OOM_ORIGIN)
 		return 1000;
 
-	task_lock(p);
-	mm = p->mm;
-	if (!mm) {
-		task_unlock(p);
+	p = find_lock_task_mm(p);
+	if (!p)
 		return 0;
-	}
-
 	/*
 	 * The baseline for the badness score is the proportion of RAM that each
 	 * task's rss and swap space use.

@@ -330,12 +339,6 @@ static struct task_struct *select_bad_pr
 			*ppoints = 1000;

dump_header() always requires tasklist_lock to be held because it calls dump_tasks() which iterates through the tasklist. There are a few places where this isn't maintained, so make sure tasklist_lock is always held whenever calling dump_header().

Quoted 4 lines

dump_header() always requires tasklist_lock to be held because it calls dump_tasks() which iterates through the tasklist. There are a few places where this isn't maintained, so make sure tasklist_lock is always held whenever calling dump_header().

Looks correct, but I'd suggest you to update the changelog.

Not only dump_tasks() needs tasklist, oom_kill_process() needs it too for list_for_each_entry(children).

You fixed this:

mm/oom_kill.c void pagefault_out_of_memory(void) · line 725

Quoted 8 lines

mm/oom_kill.c +2 -0

@@ -724,8 +719,10 @@ void pagefault_out_of_memory(void)
 
 	if (try_set_system_oom()) {
 		constrained_alloc(NULL, 0, NULL, &totalpages);
+		read_lock(&tasklist_lock);
 		err = oom_kill_process(current, 0, 0, 0, totalpages, NULL,
 					"Out of memory (pagefault)");
+		read_unlock(&tasklist_lock);

Quoted 4 lines · Oleg Nesterov

Probably something like the patch below makes sense. Note that "skip kernel threads" logic is wrong too, we should check PF_KTHREAD. Probably it is better to check it in select_bad_process() instead, near is_global_init().

is_global_init() will be true for p->flags & PF_KTHREAD.

Quoted 20 lines

mm/oom_kill.c +2 -6

@@ -159,13 +172,9 @@ unsigned int oom_badness(struct task_str
 	if (p->flags & PF_OOM_ORIGIN)
 		return 1000;
 
-	task_lock(p);
-	mm = p->mm;
-	if (!mm) {
-		task_unlock(p);
+	p = find_lock_task_mm(p);
+	if (!p)
 		return 0;
-	}
-
 	/*
 	 * The baseline for the badness score is the proportion of RAM that each
 	 * task's rss and swap space use.

@@ -330,12 +339,6 @@ static struct task_struct *select_bad_pr
 			*ppoints = 1000;
 		}
 

mm/oom_kill.c static struct task_struct *select_bad_pr · line 343

Quoted 8 lines

mm/oom_kill.c +0 -6

-		/*
-		 * skip kernel threads and tasks which have already released
-		 * their mm.
-		 */
-		if (!p->mm)
-			continue;
 		if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 			continue;

You can't do this for the reason I cited in another email, oom_badness() returning 0 does not exclude a task from being chosen by selcet_bad_process(), it will use that task if nothing else has been found yet. We must explicitly filter it from consideration by checking for !p->mm.

Quoted 20 lines

Quoted 20 lines

mm/oom_kill.c +2 -6

@@ -159,13 +172,9 @@ unsigned int oom_badness(struct task_str
 	if (p->flags & PF_OOM_ORIGIN)
 		return 1000;
 
-	task_lock(p);
-	mm = p->mm;
-	if (!mm) {
-		task_unlock(p);
+	p = find_lock_task_mm(p);
+	if (!p)
 		return 0;
-	}
-
 	/*
 	 * The baseline for the badness score is the proportion of RAM that each
 	 * task's rss and swap space use.

@@ -330,12 +339,6 @@ static struct task_struct *select_bad_pr
 			*ppoints = 1000;
 		}
 

Quoted 14 lines

mm/oom_kill.c static struct task_struct *select_bad_pr · line 343

Quoted 8 lines

mm/oom_kill.c +0 -6

-		/*
-		 * skip kernel threads and tasks which have already released
-		 * their mm.
-		 */
-		if (!p->mm)
-			continue;
 		if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 			continue;

You can't do this for the reason I cited in another email, oom_badness() returning 0 does not exclude a task from being chosen by selcet_bad_process(), it will use that task if nothing else has been found yet. We must explicitly filter it from consideration by checking for !p->mm.

Yes, you are right. OK, oom_badness() can never return points < 0, we can make it int and oom_badness() can return -1 if !mm. IOW,

	- unsigned int points;
	+ int points;
	...

	points = oom_badness(...);
	if (points >= 0 && (points > *ppoints || !chosen))
		chosen = p;

Quoted 13 lines · Oleg Nesterov

Quoted 5 lines

You can't do this for the reason I cited in another email, oom_badness() returning 0 does not exclude a task from being chosen by selcet_bad_process(), it will use that task if nothing else has been found yet. We must explicitly filter it from consideration by checking for !p->mm.

Yes, you are right. OK, oom_badness() can never return points < 0, we can make it int and oom_badness() can return -1 if !mm. IOW,

- unsigned int points;
+ int points;
...

Quoted 4 lines

	points = oom_badness(...);
	if (points >= 0 && (points > *ppoints || !chosen))
		chosen = p;

oom_badness() and its predecessor badness() in mainline never return negative scores, so I don't see the value in doing this; just filter the task in select_bad_process() with !p->mm as it has always been done.

Once again:

	void *memory_hog_thread(void *arg)
	{
		for (;;)
			malloc(A_LOT);
	}

	int main(void)
	{
		pthread_create(memory_hog_thread, ...);
		syscall(__NR_exit, 0);
	}

Now, even if we fix PF_EXITING check, select_bad_process() will always ignore this process. The group leader has ->mm == NULL.

select_bad_process() thinks a kernel thread can't have ->mm != NULL, this is not true due to use_mm().

Change the code to check PF_KTHREAD.

Signed-off-by Oleg Nesterov <[email protected]>

select_bad_process() checks PF_EXITING to detect the task which is going to release its memory, but the logic is very wrong.

- a single process P with the dead group leader disables select_bad_process() completely, it will always return ERR_PTR() while P can live forever

- if the PF_EXITING task has already released its ->mm it doesn't make sense to expect it is goiing to free more memory (except task_struct/etc)

Change the code to ignore the PF_EXITING tasks without ->mm.

Signed-off-by Oleg Nesterov <[email protected]>

---

 mm/oom_kill.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 19 lines · Oleg Nesterov

select_bad_process() checks PF_EXITING to detect the task which is going to release its memory, but the logic is very wrong.

- a single process P with the dead group leader disables select_bad_process() completely, it will always return ERR_PTR() while P can live forever

- if the PF_EXITING task has already released its ->mm it doesn't make sense to expect it is goiing to free more memory (except task_struct/etc)

Change the code to ignore the PF_EXITING tasks without ->mm.

Signed-off-by Oleg Nesterov <[email protected]>

---

 mm/oom_kill.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/oom_kill.c static struct task_struct *select_bad_pr · line 325

Quoted 8 lines

mm/oom_kill.c renamed from mm/oom_kill.c~2_FIX_PF_EXITING +1 -1

@@ -322,7 +322,7 @@ static struct task_struct *select_bad_pr
 		 * the process of exiting and releasing its resources.
 		 * Otherwise we could get an easy OOM deadlock.
 		 */
-		if (p->flags & PF_EXITING) {
+		if ((p->flags & PF_EXITING) && p->mm) {

Even this check is satisfied, it still can't say p is a good victim or it will release memory automatically if multi threaded, as the exiting of p doesn't mean the other threads are going to exit, so the ->mm won't be released.

Quoted 21 lines · anfei

Quoted 19 lines · Oleg Nesterov

select_bad_process() checks PF_EXITING to detect the task which is going to release its memory, but the logic is very wrong.

- a single process P with the dead group leader disables select_bad_process() completely, it will always return ERR_PTR() while P can live forever

- if the PF_EXITING task has already released its ->mm it doesn't make sense to expect it is goiing to free more memory (except task_struct/etc)

Change the code to ignore the PF_EXITING tasks without ->mm.

Signed-off-by Oleg Nesterov <[email protected]>

---

 mm/oom_kill.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 13 lines

mm/oom_kill.c static struct task_struct *select_bad_pr · line 325

Quoted 8 lines

mm/oom_kill.c renamed from mm/oom_kill.c~2_FIX_PF_EXITING +1 -1

@@ -322,7 +322,7 @@ static struct task_struct *select_bad_pr
 		 * the process of exiting and releasing its resources.
 		 * Otherwise we could get an easy OOM deadlock.
 		 */
-		if (p->flags & PF_EXITING) {
+		if ((p->flags & PF_EXITING) && p->mm) {

Even this check is satisfied, it still can't say p is a good victim or it will release memory automatically if multi threaded, as the exiting of p doesn't mean the other threads are going to exit, so the ->mm won't be released.

Yes, completely agreed.

Unfortunately I forgot to copy this into the changelog, but when I discussed this change I mentioned "still not perfect, but much better".

I do not really know what is the "right" solution. Even if we fix this check for mt case, we also have CLONE_VM tasks.

So, this patch just tries to improve things, to avoid the easy-to-trigger false positives.

Oleg.

Quoted 4 lines · Oleg Nesterov

I do not really know what is the "right" solution. Even if we fix this check for mt case, we also have CLONE_VM tasks.

What about checking mm->mm_users too? If there are any other users, just let badness judge. CLONE_VM tasks but not mt seem rare, and badness doesn't consider it too.

It doesn't make sense to call thread_group_cputime() under task_lock(), we can drop this lock right after we read get_mm_rss() and save the value in the local variable.

Note: probably it makes more sense to use sum_exec_runtime instead of utime + stime, it is much more precise. A task can eat a lot of CPU time, but its Xtime can be zero.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 7 lines

Quoted 7 lines

Yes, you are right. OK, oom_badness() can never return points < 0, we can make it int and oom_badness() can return -1 if !mm. IOW,

- unsigned int points;
+ int points;
...

Quoted 8 lines

Quoted 4 lines

	points = oom_badness(...);
	if (points >= 0 && (points > *ppoints || !chosen))
		chosen = p;

oom_badness() and its predecessor badness() in mainline never return negative scores, so I don't see the value in doing this; just filter the task in select_bad_process() with !p->mm as it has always been done.

David, you continue to ignore my arguments ;) select_bad_process() must not filter out the tasks with ->mm == NULL.

Once again:

	void *memory_hog_thread(void *arg)
	{
		for (;;)
			malloc(A_LOT);
	}

	int main(void)
	{
		pthread_create(memory_hog_thread, ...);
		syscall(__NR_exit, 0);
	}

Now, even if we fix PF_EXITING check, select_bad_process() will always ignore this process. The group leader has ->mm == NULL.

See?

That is why I think we need something like find_lock_task_mm() in the pseudo-patch I sent.

Quoted 3 lines · Oleg Nesterov

David, you continue to ignore my arguments ;) select_bad_process() must not filter out the tasks with ->mm == NULL.

I'm not ignoring your arguments, I think you're ignoring what I'm responding to.

I prefer to keep oom_badness() to be a positive range as
it always has been (and /proc/pid/oom_score has always used an unsigned qualifier),

so I disagree that we need to change oom_badness() to return anything other than 0 for such tasks. We need to filter them explicitly in select_bad_process() instead, so please do this there.

Quoted 5 lines

Quoted 3 lines

David, you continue to ignore my arguments ;) select_bad_process() must not filter out the tasks with ->mm == NULL.

I'm not ignoring your arguments, I think you're ignoring what I'm responding to.

Ah, sorry, I misunderstood your replies.

Quoted 3 lines

I prefer to keep oom_badness() to be a positive range as
it always has been (and /proc/pid/oom_score has always used an unsigned qualifier),

Yes, I thought about /proc/pid/oom_score, but imho this is minor issue. We can s/%lu/%ld/ though, or just report 0 if oom_badness() returns -1. Or something.

An oom_badness() score of 0 means "never kill" according to Documentation/filesystems/proc.txt, so explicitly exclude it from being selected for kill. These tasks have either detached their p->mm or are set to OOM_DISABLE.

Quoted 5 lines · Oleg Nesterov

Quoted 4 lines

I prefer to keep oom_badness() to be a positive range as
it always has been (and /proc/pid/oom_score has always used an unsigned qualifier),

Quoted 3 lines

Yes, I thought about /proc/pid/oom_score, but imho this is minor issue. We can s/%lu/%ld/ though, or just report 0 if oom_badness() returns -1. Or something.

Just have it return 0, meaning never kill, and then ensure "chosen" is never set for an oom_badness() of 0, even if we don't have another task to kill. That's how Documentation/filesystems/proc.txt describes it anyway.

Quoted 7 lines · David Rientjes

Quoted 3 lines · Oleg Nesterov

Note that __oom_kill_task() does force_sig(SIGKILL) which assumes that ->sighand != NULL. This is not true if out_of_memory() is called after current has already passed exit_notify().

Quoted 2 lines

We have an even bigger problem if current is in the oom killer at exit_notify() since it has already detached its ->mm in exit_mm() :)

Can't understand... I thought that in theory even kmalloc(1) can trigger oom.

Quoted 10 lines

Quoted 9 lines

IOW, unless I missed something, it is very easy to hide the process from oom-kill:

	int main()
	{
		pthread_create(memory_hog_func);
		syscall(__NR_exit);
	}

Quoted 3 lines

The check for !p->mm was moved in the -mm tree (and the oom killer was entirely rewritten in that tree, so I encourage you to work off of it instead

OK, but I guess this !p->mm check is still wrong for the same reason. In fact I do not understand why it is needed in select_bad_process() right before oom_badness() which checks ->mm too (and this check is equally wrong).

Quoted 3 lines

so if the oom killer finds an already exiting task,
it will become a no-op since it should eventually free memory and avoids a needless oom kill.

No, afaics, And this reminds that I already complained about this PF_EXITING check.

Once again, p is the group leader. It can be dead (no ->mm, PF_EXITING is set) but it can have sub-threads. This means, unless I missed something, any user can trivially disable select_bad_process() forever.

Well. Looks like, -mm has a lot of changes in oom_kill.c. Perhaps it would be better to fix these mt bugs first...

Say, oom_forkbomb_penalty() does list_for_each_entry(tsk->children). Again, this is not right even if we forget about !child->mm check. This list_for_each_entry() can only see the processes forked by the main thread.

Likewise, oom_kill_process()->list_for_each_entry() is not right too.

Hmm. Why oom_forkbomb_penalty() does thread_group_cputime() under task_lock() ? It seems, ->alloc_lock() is only needed for get_mm_rss().

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · David Rientjes

Quoted 3 lines · Oleg Nesterov

Note that __oom_kill_task() does force_sig(SIGKILL) which assumes that ->sighand != NULL. This is not true if out_of_memory() is called after current has already passed exit_notify().

Quoted 5 lines

Quoted 2 lines

We have an even bigger problem if current is in the oom killer at exit_notify() since it has already detached its ->mm in exit_mm() :)

Can't understand... I thought that in theory even kmalloc(1) can trigger oom.

__oom_kill_task() cannot be called on a task without an ->mm.

Quoted 8 lines

Quoted 3 lines

The check for !p->mm was moved in the -mm tree (and the oom killer was entirely rewritten in that tree, so I encourage you to work off of it instead

OK, but I guess this !p->mm check is still wrong for the same reason. In fact I do not understand why it is needed in select_bad_process() right before oom_badness() which checks ->mm too (and this check is equally wrong).

It prevents kthreads from being killed.

Quoted 11 lines

Quoted 3 lines

so if the oom killer finds an already exiting task,
it will become a no-op since it should eventually free memory and avoids a needless oom kill.

No, afaics, And this reminds that I already complained about this PF_EXITING check.

Once again, p is the group leader. It can be dead (no ->mm, PF_EXITING is set) but it can have sub-threads. This means, unless I missed something, any user can trivially disable select_bad_process() forever.

The task is in the process of exiting and will do so if its not current, otherwise it will get access to memory reserves since we're obviously oom in the exit path. Thus, we'll be freeing that memory soon or recalling the oom killer to kill additional tasks once those children have been reparented (or one of its children was sacrificed).

Quoted 5 lines

Say, oom_forkbomb_penalty() does list_for_each_entry(tsk->children). Again, this is not right even if we forget about !child->mm check. This list_for_each_entry() can only see the processes forked by the main thread.

That's the intention.

Quoted 2 lines

Likewise, oom_kill_process()->list_for_each_entry() is not right too.

Why?

Quoted 3 lines

Hmm. Why oom_forkbomb_penalty() does thread_group_cputime() under task_lock() ? It seems, ->alloc_lock() is only needed for get_mm_rss().

Right, but we need to ensure that the check for !child->mm || child->mm == tsk->mm fails before adding in get_mm_rss(child->mm). It can race and detach its mm prior to the dereference.

It would be possible to move the thread_group_cputime() out of this critical section,

but I felt it was
better to do filter all tasks with child->mm == tsk->mm first before unnecessarily finding the cputime for them.

Quoted 11 lines · David Rientjes

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · David Rientjes

Quoted 3 lines · Oleg Nesterov

Note that __oom_kill_task() does force_sig(SIGKILL) which assumes that ->sighand != NULL. This is not true if out_of_memory() is called after current has already passed exit_notify().

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

We have an even bigger problem if current is in the oom killer at exit_notify() since it has already detached its ->mm in exit_mm() :)

Can't understand... I thought that in theory even kmalloc(1) can trigger oom.

__oom_kill_task() cannot be called on a task without an ->mm.

Why? You ignored this part:

Say, right after exit_mm() we are doing acct_process(), and f_op->write() needs a page. So, you are saying that in this case __page_cache_alloc() can never trigger out_of_memory() ?

why this is not possible?

David, I am not arguing, I am asking.

Quoted 9 lines

Why? You ignored this part:

Say, right after exit_mm() we are doing acct_process(), and f_op->write() needs a page. So, you are saying that in this case __page_cache_alloc() can never trigger out_of_memory() ?

why this is not possible?

David, I am not arguing, I am asking.

In case I wasn't clear...

Yes, currently __oom_kill_task(p) is not possible if p->mm == NULL.

But your patch adds

	if (fatal_signal_pending(current))
		__oom_kill_task(current);

into out_of_memory().

Quoted 16 lines · Oleg Nesterov

Quoted 9 lines

Why? You ignored this part:

Say, right after exit_mm() we are doing acct_process(), and f_op->write() needs a page. So, you are saying that in this case __page_cache_alloc() can never trigger out_of_memory() ?

why this is not possible?

David, I am not arguing, I am asking.

In case I wasn't clear...

Yes, currently __oom_kill_task(p) is not possible if p->mm == NULL.

But your patch adds

Quoted 5 lines

	if (fatal_signal_pending(current))
		__oom_kill_task(current);

into out_of_memory().

Ok, and it's possible during the tasklist scan if current is PF_EXITING and that gets passed to oom_kill_process(),

mm/oom_kill.c static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order, · line 462

Quoted 8 lines

mm/oom_kill.c +1 -1

@@ -459,7 +459,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 	 * its children or threads, just set TIF_MEMDIE so it can die quickly
 	 */
 	if (p->flags & PF_EXITING) {
-		__oom_kill_task(p);
+		set_tsk_thread_flag(p, TIF_MEMDIE);

So, probably this makes sense anyway but not strictly necessary, up to you.

mm/oom_kill.c void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, · line 689

Quoted 3 lines

mm/oom_kill.c +1 -1

 	if (fatal_signal_pending(current)) {
-		__oom_kill_task(current);
+		set_tsk_thread_flag(current, TIF_MEMDIE);

Yes, I think this fix is needed.

Why? You ignored this part:

Say, right after exit_mm() we are doing acct_process(), and f_op->write() needs a page. So, you are saying that in this case __page_cache_alloc() can never trigger out_of_memory() ?

why this is not possible?

Quoted 6 lines

Quoted 4 lines

OK, but I guess this !p->mm check is still wrong for the same reason. In fact I do not understand why it is needed in select_bad_process() right before oom_badness() which checks ->mm too (and this check is equally wrong).

It prevents kthreads from being killed.

No it doesn't, see use_mm(). See also another email I sent.

Quoted 5 lines

The task is in the process of exiting and will do so if its not current, otherwise it will get access to memory reserves since we're obviously oom in the exit path. Thus, we'll be freeing that memory soon or recalling the oom killer to kill additional tasks once those children have been reparented (or one of its children was sacrificed).

Just can't understand.

OK, a bad user does

	int sleep_forever(void *)
	{
		pause();
	}

	int main(void)
	{
		pthread_create(sleep_forever);
		syscall(__NR_exit);
	}

Now, every time select_bad_process() is called it will find this process and PF_EXITING is true, so it just returns ERR_PTR(-1UL). And note that this process is not going to exit.

Quoted 7 lines

Quoted 5 lines

Say, oom_forkbomb_penalty() does list_for_each_entry(tsk->children). Again, this is not right even if we forget about !child->mm check. This list_for_each_entry() can only see the processes forked by the main thread.

That's the intention.

Why? shouldn't oom_badness() return the same result for any thread in thread group? We should take all childs into account.

Quoted 7 lines

Quoted 3 lines

Hmm. Why oom_forkbomb_penalty() does thread_group_cputime() under task_lock() ? It seems, ->alloc_lock() is only needed for get_mm_rss().

Right, but we need to ensure that the check for !child->mm || child->mm == tsk->mm fails before adding in get_mm_rss(child->mm). It can race and detach its mm prior to the dereference.

Oh, yes sure, I mentioned get_mm_rss() above.

Quoted 2 lines

It would be possible to move the thread_group_cputime() out of this critical section,

Yes, this is what I meant.

Quoted 3 lines

but I felt it was
better to do filter all tasks with child->mm == tsk->mm first before unnecessarily finding the cputime for them.

Yes, but we can check child->mm == tsk->mm, call get_mm_counter() and drop task_lock().

Quoted 2 lines · Oleg Nesterov

Why? You ignored this part:

Quoted 6 lines

Say, right after exit_mm() we are doing acct_process(), and f_op->write() needs a page. So, you are saying that in this case __page_cache_alloc() can never trigger out_of_memory() ?

why this is not possible?

It can, but the check for p->mm is sufficient since exit_notify()

We cannot rely on oom_badness() to filter this task because we still select it as our chosen task even with a badness score of 0 if !chosen

Your point about p->mm being non-NULL for kthreads using use_mm() is taken, we should probably just change the is_global_init() check in select_bad_process() to p->flags & PF_KTHREAD and ensure we reject oom_kill_process() for them.

Quoted 13 lines

OK, a bad user does

	int sleep_forever(void *)
	{
		pause();
	}

	int main(void)
	{
		pthread_create(sleep_forever);
		syscall(__NR_exit);
	}

Quoted 4 lines

Now, every time select_bad_process() is called it will find this process and PF_EXITING is true, so it just returns ERR_PTR(-1UL). And note that this process is not going to exit.

Hmm, so it looks like we need to filter on !p->mm before checking for PF_EXITING so that tasks that are EXIT_ZOMBIE won't make the oom killer into a no-op.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Say, oom_forkbomb_penalty() does list_for_each_entry(tsk->children). Again, this is not right even if we forget about !child->mm check. This list_for_each_entry() can only see the processes forked by the main thread.

Quoted 5 lines

Quoted 1 line

That's the intention.

Why? shouldn't oom_badness() return the same result for any thread in thread group? We should take all childs into account.

oom_forkbomb_penalty() only cares about first-descendant children that do not share the same memory,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Hmm. Why oom_forkbomb_penalty() does thread_group_cputime() under task_lock() ? It seems, ->alloc_lock() is only needed for get_mm_rss().

[...snip...]
We need task_lock() to ensure child->mm hasn't detached between the check for child->mm == tsk->mm and get_mm_rss(child->mm). So I'm not sure what you're trying to improve with this variation, it's a tradeoff between calling thread_group_cputime() under task_lock() for a subset of a task's threads when we already need to hold task_lock() anyway vs. calling it for all threads unconditionally.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Say, oom_forkbomb_penalty() does list_for_each_entry(tsk->children). Again, this is not right even if we forget about !child->mm check. This list_for_each_entry() can only see the processes forked by the main thread.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

That's the intention.

Why? shouldn't oom_badness() return the same result for any thread in thread group? We should take all childs into account.

oom_forkbomb_penalty() only cares about first-descendant children that do not share the same memory,

I see, but the code doesn't really do this. I mean, it doesn't really see the first-descendant children, only those which were forked by the main thread.

Look. We have a main thread M and the sub-thread T. T forks a lot of processes which use a lot of memory. These processes _are_ the first descendant children of the M+T thread group, they should be accounted. But M->children list is empty.

oom_forkbomb_penalty() and oom_kill_process() should do

	t = tsk;
	do {
		list_for_each_entry(child, &t->children, sibling) {
			... take child into account ...
		}
	} while_each_thread(tsk, t);

See the patch below. Yes, this is minor, but it is always good to avoid the unnecessary locks, and thread_group_cputime() is O(N).

Not only for performance reasons. This allows to change the locking in thread_group_cputime() if needed without fear to deadlock with task_lock().

Oleg.

mm/oom_kill.c +4 -1

@@ -97,13 +97,16 @@ static unsigned long oom_forkbomb_penalt
 		return 0;
 	list_for_each_entry(child, &tsk->children, sibling) {
 		struct task_cputime task_time;
-		unsigned long runtime;
+		unsigned long runtime, this_rss;
 
 		task_lock(child);
 		if (!child->mm || child->mm == tsk->mm) {
 			task_unlock(child);
 			continue;
 		}
+		this_rss = get_mm_rss(child->mm);
+		task_unlock(child);
+
 		thread_group_cputime(child, &task_time);
 		runtime = cputime_to_jiffies(task_time.utime) +
 			  cputime_to_jiffies(task_time.stime);

@@ -113,10 +116,9 @@ static unsigned long oom_forkbomb_penalt
 		 * get to execute at all in such cases anyway.

Quoted 5 lines · Oleg Nesterov

Look. We have a main thread M and the sub-thread T. T forks a lot of processes which use a lot of memory. These processes _are_ the first descendant children of the M+T thread group, they should be accounted. But M->children list is empty.

Quoted 9 lines

oom_forkbomb_penalty() and oom_kill_process() should do

	t = tsk;
	do {
		list_for_each_entry(child, &t->children, sibling) {
			... take child into account ...
		}
	} while_each_thread(tsk, t);

In this case, it seems more appropriate that we would penalize T and not M

since it's not necessarily responsible for the behavior of the children it forks. T is the buggy/malicious program, not M.

Quoted 8 lines

mm/oom_kill.c +1 -1

@@ -97,13 +97,16 @@ static unsigned long oom_forkbomb_penalt
 		return 0;
 	list_for_each_entry(child, &tsk->children, sibling) {
 		struct task_cputime task_time;
-		unsigned long runtime;
+		unsigned long runtime, this_rss;

mm/oom_kill.c static unsigned long oom_forkbomb_penalt · line 110

Quoted 10 lines

mm/oom_kill.c +3 -0

 
 		task_lock(child);
 		if (!child->mm || child->mm == tsk->mm) {
 			task_unlock(child);
 			continue;
 		}
+		this_rss = get_mm_rss(child->mm);
+		task_unlock(child);
+
 	/*

This patch looks good, will you send it to Andrew with a changelog and sign-off line? Also feel free to add:

Acked-by David Rientjes <[email protected]>

->siglock is no longer needed to access task->signal, change oom_adjust_read() and oom_adjust_write() to read/write oom_adj lockless.

Yes, this means that "echo 2 >oom_adj" and "echo 1 >oom_adj" can race and the second write can win, but I hope this is OK.

Quoted 4 lines · Oleg Nesterov

->siglock is no longer needed to access task->signal, change oom_adjust_read() and oom_adjust_write() to read/write oom_adj lockless.

Quoted 3 lines

Yes, this means that "echo 2 >oom_adj" and "echo 1 >oom_adj" can race and the second write can win, but I hope this is OK.

Ok, but could you base this on -mm at
http://userweb.kernel.org/~akpm/mmotm/ since an additional tunable has been added (oom_score_adj), which does the same thing?

Quoted 4 lines · Oleg Nesterov

->siglock is no longer needed to access task->signal, change oom_adjust_read() and oom_adjust_write() to read/write oom_adj lockless.

Quoted 2 lines

Yes, this means that "echo 2 >oom_adj" and "echo 1 >oom_adj" can race and the second write can win, but I hope this is OK.

Ok, but could you base this on -mm at
http://userweb.kernel.org/~akpm/mmotm/ since an additional tunable has been added (oom_score_adj), which does the same thing?

David, I just can't understand why
oom-badness-heuristic-rewrite.patch
duplicates the related code in fs/proc/base.c and why it preserves the deprecated signal->oom_adj.

OK. Please forget about lock_task_sighand/signal issues. Can't we kill signal->oom_adj and create a single helper for both /proc/pid/{oom_adj,oom_score_adj} ?

	static ssize_t oom_any_adj_write(struct file *file, const char __user *buf,
						size_t count, bool deprecated_mode)
	{
		struct task_struct *task;
		char buffer[PROC_NUMBUF];
		unsigned long flags;
		long oom_score_adj;
		int err;

		memset(buffer, 0, sizeof(buffer));
		if (count > sizeof(buffer) - 1)
			count = sizeof(buffer) - 1;
		if (copy_from_user(buffer, buf, count))
			return -EFAULT;

		err = strict_strtol(strstrip(buffer), 0, &oom_score_adj);
		if (err)
			return -EINVAL;

		if (depraceted_mode) {
			 if (oom_score_adj == OOM_ADJUST_MAX)
				oom_score_adj = OOM_SCORE_ADJ_MAX;

			 else
				oom_score_adj = (oom_score_adj * OOM_SCORE_ADJ_MAX) /
						-OOM_DISABLE;
		}

		if (oom_score_adj < OOM_SCORE_ADJ_MIN ||
				oom_score_adj > OOM_SCORE_ADJ_MAX)

			return -EINVAL;

		task = get_proc_task(file->f_path.dentry->d_inode);
		if (!task)
			return -ESRCH;
		if (!lock_task_sighand(task, &flags)) {
			put_task_struct(task);
			return -ESRCH;
		}
		if (oom_score_adj < task->signal->oom_score_adj &&
				!capable(CAP_SYS_RESOURCE)) {
			unlock_task_sighand(task, &flags);
			put_task_struct(task);
			return -EACCES;

		}

		task->signal->oom_score_adj = oom_score_adj;

		unlock_task_sighand(task, &flags);
		put_task_struct(task);
		return count;
	}

Quoted 4 lines · Oleg Nesterov

David, I just can't understand why
oom-badness-heuristic-rewrite.patch
duplicates the related code in fs/proc/base.c and why it preserves the deprecated signal->oom_adj.

You could combine the two write functions together and then two read functions together if you'd like.

Quoted 7 lines

	static ssize_t oom_any_adj_write(struct file *file, const char __user *buf,
						size_t count, bool deprecated_mode)
	{

		if (depraceted_mode) {
			 if (oom_score_adj == OOM_ADJUST_MAX)
				oom_score_adj = OOM_SCORE_ADJ_MAX;

???

Quoted 7 lines

			 else
				oom_score_adj = (oom_score_adj * OOM_SCORE_ADJ_MAX) /
						-OOM_DISABLE;
		}

		if (oom_score_adj < OOM_SCORE_ADJ_MIN ||
				oom_score_adj > OOM_SCORE_ADJ_MAX)

That doesn't work for depraceted_mode (sic), you'd need to test for OOM_ADJUST_MIN and OOM_ADJUST_MAX in that case.

There have been efforts to reuse as much of this code as possible for other sysctl handlers as well, you might be better off looking for

Quoted 2 lines

That doesn't work for depraceted_mode (sic), you'd need to test for OOM_ADJUST_MIN and OOM_ADJUST_MAX in that case.

Yes, probably "if (depraceted_mode)" should do more checks, I didn't try to verify that MIN/MAX are correctly converted. I showed this code to explain what I mean.

Quoted 2 lines

There have been efforts to reuse as much of this code as possible for other sysctl handlers as well, you might be better off looking for

David, sorry ;) Right now I'd better try to stop the overloading of ->siglock. And, I'd like to shrink struct_signal if possible, but this is minor.

Quoted 3 lines · Oleg Nesterov

Quoted 2 lines

That doesn't work for depraceted_mode (sic), you'd need to test for OOM_ADJUST_MIN and OOM_ADJUST_MAX in that case.

Quoted 4 lines

Yes, probably "if (depraceted_mode)" should do more checks, I didn't try to verify that MIN/MAX are correctly converted. I showed this code to explain what I mean.

Ok, please cc me on the patch, it will be good to get rid of the duplicate code and remove oom_adj from struct signal_struct.

Do we need ->siglock? Why can't we just do

	struct sighand_struct *sighand;
	struct signal_struct *sig;

	rcu_read_lock();
	sighand = rcu_dereference(task->sighand);
	if (!sighand) {
		rcu_read_unlock();
		return;
	}
	sig = task->signal;

	... load/store to sig ...

	rcu_read_unlock();

But. Unless we kill signal->oom_adj, we have another reason for ->siglock, we can't update both oom_adj and oom_score_adj atomically, and if we race with another thread they can be inconsistent wrt each other. Yes, oom_adj is not actually used, except we report it back to user-space, but still.

So, I am going to send 2 patches. The first one factors out the code in base.c and kills signal->oom_adj, the next one removes ->siglock.

proc_oom_score(task) have a reference to task_struct, but that is all. If this task was already released before we take tasklist_lock

	- we can't use task->group_leader, it points to nowhere

	- it is not safe to call badness() even if this task is
	  ->group_leader, has_intersects_mems_allowed() assumes
	  it is safe to iterate over ->thread_group list.

Add the pid_alive() check to ensure __unhash_process() was not called.

Note: I think we shouldn't use ->group_leader, badness() should return the same result for any sub-thread. However this is not true currently, and I think that ->mm check and list_for_each_entry(p->children) in badness are not right.

Quoted 11 lines · Oleg Nesterov

proc_oom_score(task) have a reference to task_struct, but that is all. If this task was already released before we take tasklist_lock

	- we can't use task->group_leader, it points to nowhere

	- it is not safe to call badness() even if this task is
	  ->group_leader, has_intersects_mems_allowed() assumes
	  it is safe to iterate over ->thread_group list.

Add the pid_alive() check to ensure __unhash_process() was not called.

Quoted 5 lines

Note: I think we shouldn't use ->group_leader, badness() should return the same result for any sub-thread. However this is not true currently, and I think that ->mm check and list_for_each_entry(p->children) in badness are not right.

I think it would be better to just use task and not task->group_leader.

Quoted 20 lines · David Rientjes

Quoted 16 lines · Oleg Nesterov

proc_oom_score(task) have a reference to task_struct, but that is all. If this task was already released before we take tasklist_lock

	- we can't use task->group_leader, it points to nowhere

	- it is not safe to call badness() even if this task is
	  ->group_leader, has_intersects_mems_allowed() assumes
	  it is safe to iterate over ->thread_group list.

Add the pid_alive() check to ensure __unhash_process() was not called.

Note: I think we shouldn't use ->group_leader, badness() should return the same result for any sub-thread. However this is not true currently, and I think that ->mm check and list_for_each_entry(p->children) in badness are not right.

Quoted 1 line

I think it would be better to just use task and not task->group_leader.

Sure, agreed. I preserved ->group_leader just because I didn't understand why the current code doesn't use task. But note that pid_alive() is still needed.

But. Oh well. David, oom-badness-heuristic-rewrite.patch changed badness() to consult p->signal->oom_score_adj. Until recently this was wrong when it is called from proc_oom_score().

This means oom-badness-heuristic-rewrite.patch depends on signals-make-task_struct-signal-immutable-refcountable.patch, or we need the pid_alive() check again.

oom_badness() gets the new argument, long totalpages, and the callers were updated. However, long uptime is not used any longer, probably it make sense to kill this arg and simplify the callers? Unless you are going to take run-time into account later.

So, I think -mm needs the patch below, but I have no idea how to write the changelog ;)

Oleg.

fs/proc/base.c +1 -1

@@ -430,12 +430,13 @@ static const struct file_operations proc
 /* The badness from the OOM killer */
 static int proc_oom_score(struct task_struct *task, char *buffer)
 {
-	unsigned long points;
+	unsigned long points = 0;

Quoted 4 lines · Oleg Nesterov

But. Oh well. David, oom-badness-heuristic-rewrite.patch changed badness() to consult p->signal->oom_score_adj. Until recently this was wrong when it is called from proc_oom_score().

Quoted 4 lines

This means oom-badness-heuristic-rewrite.patch depends on signals-make-task_struct-signal-immutable-refcountable.patch, or we need the pid_alive() check again.

oom-badness-heuristic-rewrite.patch didn't change anything, Linus' tree currently dereferences p->signal->oom_adj

fs/proc/base.c static const struct file_operations proc · line 442

Quoted 8 lines

fs/proc/base.c +2 -1

 	do_posix_clock_monotonic_gettime(&uptime);
 	read_lock(&tasklist_lock);
-	points = oom_badness(task->group_leader,
+	if (pid_alive(task))
+		points = oom_badness(task,
 				global_page_state(NR_INACTIVE_ANON) +
 				global_page_state(NR_ACTIVE_ANON) +
 				global_page_state(NR_INACTIVE_FILE) +

This should be protected by the get_proc_task() on the inode before this function is called from proc_info_read().

proc_oom_score(task) have a reference to task_struct, but that is all. If this task was already released before we take tasklist_lock

- we can't use task->group_leader, it points to nowhere

- it is not safe to call badness() even if this task is ->group_leader, has_intersects_mems_allowed() assumes it is safe to iterate over ->thread_group list.

- even worse, badness() can hit ->signal == NULL

Add the pid_alive() check to ensure __unhash_process() was not called.

Also, use "task" instead of task->group_leader. badness() should return the same result for any sub-thread. Currently this is not true, but this should be changed anyway.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 10 lines · Oleg Nesterov

I see. But still I can't understand. To me, the problem is not that B can't exit, the problem is that A doesn't know it should exit. All threads should exit and free ->mm. Even if B could exit, this is not enough. And, to some extent, it doesn't matter if it holds mmap_sem or not.

Don't get me wrong. Even if I don't understand oom_kill.c the patch looks obviously good to me, even from "common sense" pov. I am just curious.

Quoted 7 lines

So, my understanding is: we are going to kill the whole thread group but TIF_MEMDIE is per-thread. Mark the whole thread group as TIF_MEMDIE so that any thread can notice this flag and (say, __alloc_pages_slowpath) fail asap.

Is my understanding correct?

[Adding Mel Gorman <[email protected]> to the cc]

The problem with this approach is that we could easily deplete all memory reserves if the oom killed task has an extremely large number of threads, there has always been only a single thread with TIF_MEMDIE set per cpuset or memcg; for systems that don't run with cpusets or memory controller, this has been limited to one thread with TIF_MEMDIE for the entire system.

There's risk involved with suddenly allowing 1000 threads to have TIF_MEMDIE set and the chances of fully depleting all allowed zones is much higher if they allocate memory prior to exit, for example.

An alternative is to fail allocations if they are failable and the allocating task has a pending SIGKILL. It's better to preempt the oom killer since current is going to be exiting anyway and this avoids a needless kill.

That's possible if it's guaranteed that __GFP_NOFAIL allocations with a pending SIGKILL are granted ALLOC_NO_WATERMARKS to prevent them from endlessly looping while making no progress.

Comments?
---

mm/page_alloc.c +18 -12

@@ -1610,13 +1610,21 @@ try_next_zone:
 }
 
 static inline int
-should_alloc_retry(gfp_t gfp_mask, unsigned int order,
+should_alloc_retry(struct task_struct *p, gfp_t gfp_mask, unsigned int order,
 				unsigned long pages_reclaimed)
 {
 	/* Do not loop if specifically requested */
 	if (gfp_mask & __GFP_NORETRY)
 		return 0;
 
+	/* Loop if specifically requested */
+	if (gfp_mask & __GFP_NOFAIL)
+		return 1;
+
+	/* Task is killed, fail the allocation if possible */
+	if (fatal_signal_pending(p))
+		return 0;
+
 	/*
 	 * In this implementation, order <= PAGE_ALLOC_COSTLY_ORDER
 	 * means __GFP_NOFAIL, but that may not be true in other

@@ -1635,13 +1643,6 @@ should_alloc_retry(gfp_t gfp_mask, unsigned int order,
 	if (gfp_mask & __GFP_REPEAT && pages_reclaimed < (1 << order))
 		return 1;
 
-	/*
-	 * Don't let big-order allocations loop unless the caller
-	 * explicitly requests that.
-	 */
-	if (gfp_mask & __GFP_NOFAIL)
-		return 1;
-
 	return 0;
 }
 

@@ -1798,6 +1799,7 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
 	if (likely(!(gfp_mask & __GFP_NOMEMALLOC))) {
 		if (!in_interrupt() &&
 		    ((p->flags & PF_MEMALLOC) ||
+		     (fatal_signal_pending(p) && (gfp_mask & __GFP_NOFAIL)) ||
 		     unlikely(test_thread_flag(TIF_MEMDIE))))
 			alloc_flags |= ALLOC_NO_WATERMARKS;
 	}

@@ -1812,6 +1814,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	int migratetype)
 {
 	const gfp_t wait = gfp_mask & __GFP_WAIT;
+	const gfp_t nofail = gfp_mask & __GFP_NOFAIL;
 	struct page *page = NULL;
 	int alloc_flags;
 	unsigned long pages_reclaimed = 0;

@@ -1876,7 +1879,7 @@ rebalance:
 		goto nopage;
 
 	/* Avoid allocations with no watermarks from looping endlessly */
-	if (test_thread_flag(TIF_MEMDIE) && !(gfp_mask & __GFP_NOFAIL))
+	if (test_thread_flag(TIF_MEMDIE) && !nofail)
 		goto nopage;
 
 	/* Try direct reclaim and then allocating */

@@ -1888,6 +1891,10 @@ rebalance:
 	if (page)
 		goto got_pg;
 
+	/* Task is killed, fail the allocation if possible */
+	if (fatal_signal_pending(p) && !nofail)
+		goto nopage;
+
 	/*
 	 * If we failed to make any progress reclaiming, then we are
 	 * running out of options and have to consider going OOM

@@ -1909,8 +1916,7 @@ rebalance:
 			 * made, there are no other options and retrying is
 			 * unlikely to help.
 			 */
-			if (order > PAGE_ALLOC_COSTLY_ORDER &&
-						!(gfp_mask & __GFP_NOFAIL))
+			if (order > PAGE_ALLOC_COSTLY_ORDER && !nofail)
 				goto nopage;
 
 			goto restart;

@@ -1919,7 +1925,7 @@ rebalance:
 
 	/* Check if we should retry the allocation */
 	pages_reclaimed += did_some_progress;
-	if (should_alloc_retry(gfp_mask, order, pages_reclaimed)) {
+	if (should_alloc_retry(p, gfp_mask, order, pages_reclaimed)) {
 		/* Wait for some write requests to complete then retry */
 		congestion_wait(BLK_RW_ASYNC, HZ/50);
 		goto rebalance;

I think this method is okay, but it's easy to trigger another bug of oom. See select_bad_process():

	if (!p->mm)
		continue;

!p->mm is not always an unaccepted condition. e.g. "p" is killed and doing exit, setting tsk->mm to NULL is before releasing the memory. And in multi threading environment, this happens much more. In __out_of_memory(), it panics if select_bad_process returns NULL. The simple way to fix it is as mem_cgroup_out_of_memory() does.

mm/oom_kill.c +0 -1

index afeab2a..9aae208 100644
@@ -588,12 +588,8 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		return;
 
-	/* Found nothing?!?! Either we hang forever, or we panic. */

Quoted 10 lines · anfei

I think this method is okay, but it's easy to trigger another bug of oom. See select_bad_process():

	if (!p->mm)
		continue;

!p->mm is not always an unaccepted condition. e.g. "p" is killed and doing exit, setting tsk->mm to NULL is before releasing the memory. And in multi threading environment, this happens much more. In __out_of_memory(), it panics if select_bad_process returns NULL. The simple way to fix it is as mem_cgroup_out_of_memory() does.

This is fixed by oom-avoid-race-for-oom-killed-tasks-detaching-mm-prior-to-exit.patch in the -mm tree.

See
http://userweb.kernel.org/~akpm/mmotm/broken-out/oom-avoid-race-for-oom-killed-tasks-detaching-mm-prior-to-exit.patch

Quoted 17 lines

mm/oom_kill.c +2 -6

index afeab2a..9aae208 100644
@@ -588,12 +588,8 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		return;
 
-	/* Found nothing?!?! Either we hang forever, or we panic. */
-	if (!p) {
-		read_unlock(&tasklist_lock);
-		dump_header(NULL, gfp_mask, order, NULL);
-		panic("Out of memory and no killable processes...\n");
-	}
+	if (!p)
+		p = current;
 

mm/oom_kill.c retry: · line 595

Quoted 2 lines

mm/oom_kill.c +0 -0

 	if (oom_kill_process(p, gfp_mask, order, points, NULL,
 			     "Out of memory"))

The reason p wasn't selected is because it fails to meet the criteria for candidacy in select_bad_process(), not necessarily because of a race with the !p->mm check that the -mm patch cited above fixes. It's quite possible that current has an oom_adj value of OOM_DISABLE, for example, where this would be wrong.

Quoted 17 lines

Quoted 17 lines

mm/oom_kill.c +2 -6

index afeab2a..9aae208 100644
@@ -588,12 +588,8 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		return;
 
-	/* Found nothing?!?! Either we hang forever, or we panic. */
-	if (!p) {
-		read_unlock(&tasklist_lock);
-		dump_header(NULL, gfp_mask, order, NULL);
-		panic("Out of memory and no killable processes...\n");
-	}
+	if (!p)
+		p = current;
 

Quoted 8 lines

mm/oom_kill.c retry: · line 595

Quoted 2 lines

mm/oom_kill.c +0 -0

 	if (oom_kill_process(p, gfp_mask, order, points, NULL,
 			     "Out of memory"))

The reason p wasn't selected is because it fails to meet the criteria for candidacy in select_bad_process(), not necessarily because of a race with the !p->mm check that the -mm patch cited above fixes. It's quite possible that current has an oom_adj value of OOM_DISABLE, for example, where this would be wrong.

I see. And what about changing mem_cgroup_out_of_memory() too?

mm/oom_kill.c +0 -0

index 0cb1ca4..9e89a29 100644
@@ -510,8 +510,10 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		goto out;

Quoted 8 lines

mm/oom_kill.c +0 -0

index 0cb1ca4..9e89a29 100644
@@ -510,8 +510,10 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		goto out;

mm/oom_kill.c retry: · line 519

Quoted 10 lines

mm/oom_kill.c +4 -2

 
-	if (!p)
-		p = current;
+	if (!p) {
+		read_unlock(&tasklist_lock);
+		panic("Out of memory and no killable processes...\n");
+	}
 
 	if (oom_kill_process(p, gfp_mask, 0, points, limit, mem,
 				"Memory cgroup out of memory"))

This actually does appear to be necessary but for a different reason: if current is unkillable because it has OOM_DISABLE, for example, then oom_kill_process() will repeatedly fail and mem_cgroup_out_of_memory() will infinitely loop.

Kame-san?

Quoted 24 lines

Quoted 8 lines

mm/oom_kill.c +0 -0

index 0cb1ca4..9e89a29 100644
@@ -510,8 +510,10 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		goto out;

mm/oom_kill.c retry: · line 519

Quoted 10 lines

mm/oom_kill.c +4 -2

 
-	if (!p)
-		p = current;
+	if (!p) {
+		read_unlock(&tasklist_lock);
+		panic("Out of memory and no killable processes...\n");
+	}
 
 	if (oom_kill_process(p, gfp_mask, 0, points, limit, mem,
 				"Memory cgroup out of memory"))

This actually does appear to be necessary but for a different reason: if current is unkillable because it has OOM_DISABLE, for example, then oom_kill_process() will repeatedly fail and mem_cgroup_out_of_memory() will infinitely loop.

Quoted 2 lines

Kame-san?

When a memcg goes into OOM and it only has unkillable processes (OOM_DISABLE), we can do nothing. (we can't panic because container's death != system death.)

Because memcg itself has mutex+waitqueue for mutual execusion of OOM killer, I think infinite-loop will not be critical probelm for the whole system.

And, now, memcg has oom-kill-disable + oom-kill-notifier features. So, If a memcg goes into OOM and there is no killable process, but oom-kill is not disabled by memcg.....it means system admin's mis-configuraton.

He can stop inifite loop by hand, anyway.

# echo 1 > ..../group_A/memory.oom_control

Quoted 37 lines · KAMEZAWA Hiroyuki

Quoted 24 lines

Quoted 8 lines

mm/oom_kill.c +0 -0

index 0cb1ca4..9e89a29 100644
@@ -510,8 +510,10 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		goto out;

mm/oom_kill.c retry: · line 519

Quoted 10 lines

mm/oom_kill.c +4 -2

 
-	if (!p)
-		p = current;
+	if (!p) {
+		read_unlock(&tasklist_lock);
+		panic("Out of memory and no killable processes...\n");
+	}
 
 	if (oom_kill_process(p, gfp_mask, 0, points, limit, mem,
 				"Memory cgroup out of memory"))

This actually does appear to be necessary but for a different reason: if current is unkillable because it has OOM_DISABLE, for example, then oom_kill_process() will repeatedly fail and mem_cgroup_out_of_memory() will infinitely loop.

Quoted 2 lines

Kame-san?

When a memcg goes into OOM and it only has unkillable processes (OOM_DISABLE), we can do nothing. (we can't panic because container's death != system death.)

Because memcg itself has mutex+waitqueue for mutual execusion of OOM killer, I think infinite-loop will not be critical probelm for the whole system.

And, now, memcg has oom-kill-disable + oom-kill-notifier features. So, If a memcg goes into OOM and there is no killable process, but oom-kill is not disabled by memcg.....it means system admin's mis-configuraton.

Quoted 3 lines

He can stop inifite loop by hand, anyway.

# echo 1 > ..../group_A/memory.oom_control

Then we should be able to do this since current is by definition unkillable since it was not found in select_bad_process(), right?

---

mm/oom_kill.c +0 -0

@@ -500,12 +500,9 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, limit, mem, CONSTRAINT_NONE, NULL);

Quoted 43 lines · On Tue, 30 Mar 2010 23:07:08 -0700 (PDT) David Rientjes <[email protected]>…

Quoted 40 lines · KAMEZAWA Hiroyuki

Quoted 24 lines

Quoted 8 lines

mm/oom_kill.c +0 -0

index 0cb1ca4..9e89a29 100644
@@ -510,8 +510,10 @@ retry:
 	if (PTR_ERR(p) == -1UL)
 		goto out;

mm/oom_kill.c retry: · line 519

Quoted 10 lines

mm/oom_kill.c +4 -2

 
-	if (!p)
-		p = current;
+	if (!p) {
+		read_unlock(&tasklist_lock);
+		panic("Out of memory and no killable processes...\n");
+	}
 
 	if (oom_kill_process(p, gfp_mask, 0, points, limit, mem,
 				"Memory cgroup out of memory"))

This actually does appear to be necessary but for a different reason: if current is unkillable because it has OOM_DISABLE, for example, then oom_kill_process() will repeatedly fail and mem_cgroup_out_of_memory() will infinitely loop.

Quoted 2 lines

Kame-san?

When a memcg goes into OOM and it only has unkillable processes (OOM_DISABLE), we can do nothing. (we can't panic because container's death != system death.)

Because memcg itself has mutex+waitqueue for mutual execusion of OOM killer, I think infinite-loop will not be critical probelm for the whole system.

And, now, memcg has oom-kill-disable + oom-kill-notifier features. So, If a memcg goes into OOM and there is no killable process, but oom-kill is not disabled by memcg.....it means system admin's mis-configuraton.

He can stop inifite loop by hand, anyway.

# echo 1 > ..../group_A/memory.oom_control

Quoted 2 lines

Then we should be able to do this since current is by definition unkillable since it was not found in select_bad_process(), right?

To me, this patch is acceptable and seems reasnoable.

But I didn't joined to memcg development when this check was added and don't know why kill current..