select_bad_process: cleanup 'releasing' check

PATCH In review Oleg Nesterov 2 participants 3 messages Aug 27, 2006 – Aug 28, 2006

Subsystems (2)

Memory management Memory management - OOM killer

lore

Share & tools

lore

Oleg Nesterov

author

2006-08-27 14:01 UTC #0 lore raw

PATCH 1/1 select_bad_process: cleanup 'releasing' check

On top of "select_bad_process: kill a bogus PF_DEAD/TASK_DEAD check"

No logic changes, but imho easier to read.

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

mm/oom_kill.c renamed from mm/oom_kill.c~ +10 ~~-11~~

@@ -205,7 +205,6 @@ static struct task_struct *select_bad_pr
 	do_posix_clock_monotonic_gettime(&uptime);
 	do_each_thread(g, p) {
 		unsigned long points;
-		int releasing;
 
 		/*
 		 * skip kernel threads and tasks which have already released

@@ -227,16 +226,15 @@ static struct task_struct *select_bad_pr
 		 * the process of exiting and releasing its resources.
 		 * Otherwise we could get an OOM deadlock.
 		 */
-		releasing = test_tsk_thread_flag(p, TIF_MEMDIE) ||
-						p->flags & PF_EXITING;
-		if (releasing) {
-			if (p->flags & PF_EXITING && p == current) {
-				chosen = p;
-				*ppoints = ULONG_MAX;
-				break;
-			}
-			return ERR_PTR(-1UL);
-		}
+		if ((p->flags & PF_EXITING) && p == current) {
+			chosen = p;
+			*ppoints = ULONG_MAX;
+			break;
+		}
+		if ((p->flags & PF_EXITING) ||
+				test_tsk_thread_flag(p, TIF_MEMDIE))
+			return ERR_PTR(-1UL);
+
 		if (p->oomkilladj == OOM_DISABLE)
 			continue;

@@ -246,6 +244,7 @@ static struct task_struct *select_bad_pr
 			*ppoints = points;
 		}
 	} while_each_thread(g, p);
+
 	return chosen;
 }

Nick Piggin

Acked-by on v1 1/1

2006-08-28 10:45 UTC #1 lore raw

Quoted 29 lines · Oleg Nesterov

On top of "select_bad_process: kill a bogus PF_DEAD/TASK_DEAD check"

No logic changes, but imho easier to read.

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

mm/oom_kill.c renamed from mm/oom_kill.c~ +0 ~~-10~~

@@ -205,7 +205,6 @@ static struct task_struct *select_bad_pr
 	do_posix_clock_monotonic_gettime(&uptime);
 	do_each_thread(g, p) {
 		unsigned long points;
-		int releasing;
 
 		/*
 		 * skip kernel threads and tasks which have already released

@@ -227,16 +226,15 @@ static struct task_struct *select_bad_pr
 		 * the process of exiting and releasing its resources.
 		 * Otherwise we could get an OOM deadlock.
 		 */
-		releasing = test_tsk_thread_flag(p, TIF_MEMDIE) ||
-						p->flags & PF_EXITING;
-		if (releasing) {
-			if (p->flags & PF_EXITING && p == current) {
-				chosen = p;
-				*ppoints = ULONG_MAX;
-				break;
-			}
-			return ERR_PTR(-1UL);

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

Quoted 10 lines

mm/oom_kill.c renamed from mm/oom_kill.c~ +9 -1

-		}
+		if ((p->flags & PF_EXITING) && p == current) {
+			chosen = p;
+			*ppoints = ULONG_MAX;
+			break;
+		}
+		if ((p->flags & PF_EXITING) ||
+				test_tsk_thread_flag(p, TIF_MEMDIE))
+			return ERR_PTR(-1UL);
+

Hmm, actually I think I spot a bug in the original logic: we don't want to have more than 1 task with TIF_MEMDIE at once, becaues that gives it access to memory reserves (but I saw it first in the new formulation, so maybe that does suggest it is more readable ;)

What I think should be done is the check for TIF_MEMDIE (and return -1) first, and then the PF_EXITING test. At which point, if current is found to be exiting, it should be chosen but not break... that way a subsequent MEMDIE or EXITING task has the chance to trigger the -1 return.

Anyway, if you don't want to do all that, I will when my hand gets better. Otherwise the 3 patches you sent look good, they could all have an

Acked-by Nick Piggin <[email protected]>

Thanks,
Nick

Oleg Nesterov

author

2006-08-28 11:00 UTC #2 lore raw

Quoted 28 lines · Nick Piggin

Quoted 10 lines · Oleg Nesterov
mm/oom_kill.c renamed from mm/oom_kill.c~ +0 -9
-		releasing = test_tsk_thread_flag(p, TIF_MEMDIE) ||
-						p->flags & PF_EXITING;
-		if (releasing) {
-			if (p->flags & PF_EXITING && p == current) {
-				chosen = p;
-				*ppoints = ULONG_MAX;
-				break;
-			}
-			return ERR_PTR(-1UL);
mm/oom_kill.c static struct task_struct *select_bad_pr · line 237
Quoted 10 lines
mm/oom_kill.c renamed from mm/oom_kill.c~ +9 -1
-		}
+		if ((p->flags & PF_EXITING) && p == current) {
+			chosen = p;
+			*ppoints = ULONG_MAX;
+			break;
+		}
+		if ((p->flags & PF_EXITING) ||
+				test_tsk_thread_flag(p, TIF_MEMDIE))
+			return ERR_PTR(-1UL);
+
Hmm, actually I think I spot a bug in the original logic: we don't want to have more than 1 task with TIF_MEMDIE at once, becaues that gives it access to memory reserves (but I saw it first in the new formulation, so maybe that does suggest it is more readable ;)

Quoted 4 lines

What I think should be done is the check for TIF_MEMDIE (and return -1) first, and then the PF_EXITING test. At which point, if current is found to be exiting, it should be chosen but not break... that way a subsequent MEMDIE or EXITING task has the chance to trigger the -1 return.

Aha! The logic looked somewhat strange to me, but ...

Quoted 1 line

Anyway, if you don't want to do all that, I will when my hand gets better.

I have little understanding of this magic, i'd better not to try to fix it.

Quoted 3 lines

Otherwise the 3 patches you sent look good, they could all have an
Acked-by Nick Piggin <[email protected]>

Thanks!

Oleg.