oom: break after selecting process to kill

PATCH In review Niv Yehezkel 4 participants 10 messages Sep 11, 2014 – Sep 23, 2014

Subsystems (2)

Memory management Memory management - OOM killer

lore

Share & tools

lore

Niv Yehezkel

author

2014-09-11 21:33 UTC #0 lore raw

PATCH 1/1 oom: break after selecting process to kill

There is no need to fallback and continue computing badness for each running process after we have found a process currently performing the swapoff syscall. We ought to immediately select this process for killing.

Signed-off-by Niv Yehezkel <[email protected]>

---
 mm/oom_kill.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/oom_kill.c +5 -1

index 1e11df8..68ac30e 100644
@@ -305,6 +305,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 	struct task_struct *g, *p;
 	struct task_struct *chosen = NULL;
 	unsigned long chosen_points = 0;
+	bool process_selected = false;
 
 	rcu_read_lock();
 	for_each_process_thread(g, p) {

@@ -315,7 +316,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			process_selected = true;
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:

@@ -324,6 +326,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};
+		if (process_selected)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Signature · 7 lines

1.7.10.4

--
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>

Zhang Zhen

2014-09-12 01:24 UTC #1 lore raw

Quoted 33 lines · Niv Yehezkel

Signed-off-by Niv Yehezkel <[email protected]>

---
 mm/oom_kill.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/oom_kill.c +3 -1

index 1e11df8..68ac30e 100644
@@ -305,6 +305,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 	struct task_struct *g, *p;
 	struct task_struct *chosen = NULL;
 	unsigned long chosen_points = 0;
+	bool process_selected = false;
 
 	rcu_read_lock();
 	for_each_process_thread(g, p) {

@@ -315,7 +316,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			process_selected = true;
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:

mm/oom_kill.c static struct task_struct *select_bad_process(unsigned int *ppoints, · line 330

Quoted 5 lines

mm/oom_kill.c +2 -0

@@ -324,6 +326,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};
+		if (process_selected)
+			break;

Hi,
The following comment shows that we prefer thread group leaders for display purposes.
If we break here and two threads in a thread group are performing the swapoff syscall, maybe we can not get thread group leaders.

Thanks!

mm/oom_kill.c static struct task_struct *select_bad_process(unsigned int *ppoints, · line 333

Quoted 4 lines

mm/oom_kill.c +0 -0

 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

--
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>

Niv Yehezkel

author

2014-09-12 07:39 UTC #2 lore raw

Quoted 45 lines · Zhang Zhen

Quoted 33 lines · Niv Yehezkel
There is no need to fallback and continue computing badness for each running process after we have found a process currently performing the swapoff syscall. We ought to immediately select this process for killing.
Signed-off-by Niv Yehezkel <[email protected]>
---
 mm/oom_kill.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
mm/oom_kill.c +3 -1
index 1e11df8..68ac30e 100644
@@ -305,6 +305,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 	struct task_struct *g, *p;
 	struct task_struct *chosen = NULL;
 	unsigned long chosen_points = 0;
+	bool process_selected = false;
 
 	rcu_read_lock();
 	for_each_process_thread(g, p) {
@@ -315,7 +316,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			process_selected = true;
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:
mm/oom_kill.c static struct task_struct *select_bad_process(unsigned int *ppoints, · line 330
Quoted 5 lines
mm/oom_kill.c +2 -0
@@ -324,6 +326,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};
+		if (process_selected)
+			break;
Hi,
The following comment shows that we prefer thread group leaders for display purposes.
If we break here and two threads in a thread group are performing the swapoff syscall, maybe we can not get thread group leaders.

Quoted 8 lines

Thanks!

Quoted 4 lines

mm/oom_kill.c +0 -0

 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Well, this is not the logic implemented in the loop.
Once a process is selected, it fallbacks and continues the loop.
If two threads are performing the swapoff, the latter will be chosen whatsoever.

Michal Hocko

2014-09-12 08:09 UTC #3 lore raw

In reply to Niv Yehezkel

Quoted 4 lines · Niv Yehezkel

There is no need to fallback and continue computing badness for each running process after we have found a process currently performing the swapoff syscall. We ought to immediately select this process for killing.

a) this is not only about swapoff. KSM (run_store) is currently considered oom origin as well.
b) you forgot to tell us what led you to this change. It sounds like a minor optimization to me. We can potentially skip scanning through many tasks but this is not guaranteed at all because our task might be at the very end of the tasks list as well.
c) finally this might select thread != thread_group_leader which is a minor issue affecting oom report

I am not saying the change is wrong but please make sure you first describe your motivation. Does it fix any issue you are seeing? Is this just something that struck you while reading the code? Maybe it was /* always select this thread first */ comment for OOM_SCAN_SELECT. Besides that your process_selected is not really needed. You could test for chosen_points == ULONG_MAX as well. This would be even more straightforward because any score like that is ultimate candidate.

Quoted 39 lines

Signed-off-by Niv Yehezkel <[email protected]>

---
 mm/oom_kill.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/oom_kill.c +5 -1

index 1e11df8..68ac30e 100644
@@ -305,6 +305,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 	struct task_struct *g, *p;
 	struct task_struct *chosen = NULL;
 	unsigned long chosen_points = 0;
+	bool process_selected = false;
 
 	rcu_read_lock();
 	for_each_process_thread(g, p) {

@@ -315,7 +316,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			process_selected = true;
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:

@@ -324,6 +326,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};
+		if (process_selected)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Signature · 1 line

1.7.10.4

Signature · 8 lines

Michal Hocko
SUSE Labs

--
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>

Niv Yehezkel

author

2014-09-12 08:23 UTC #4 lore raw

Quoted 54 lines · Michal Hocko

Quoted 4 lines · Niv Yehezkel
There is no need to fallback and continue computing badness for each running process after we have found a process currently performing the swapoff syscall. We ought to immediately select this process for killing.
a) this is not only about swapoff. KSM (run_store) is currently considered oom origin as well.
b) you forgot to tell us what led you to this change. It sounds like a minor optimization to me. We can potentially skip scanning through many tasks but this is not guaranteed at all because our task might be at the very end of the tasks list as well.
c) finally this might select thread != thread_group_leader which is a minor issue affecting oom report
I am not saying the change is wrong but please make sure you first describe your motivation. Does it fix any issue you are seeing? Is this just something that struck you while reading the code? Maybe it was /* always select this thread first */ comment for OOM_SCAN_SELECT. Besides that your process_selected is not really needed. You could test for chosen_points == ULONG_MAX as well. This would be even more straightforward because any score like that is ultimate candidate.
Quoted 31 lines
Signed-off-by Niv Yehezkel <[email protected]>
---
 mm/oom_kill.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
mm/oom_kill.c +3 -1
index 1e11df8..68ac30e 100644
@@ -305,6 +305,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 	struct task_struct *g, *p;
 	struct task_struct *chosen = NULL;
 	unsigned long chosen_points = 0;
+	bool process_selected = false;
 
 	rcu_read_lock();
 	for_each_process_thread(g, p) {
@@ -315,7 +316,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			process_selected = true;
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:
@@ -324,6 +326,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};

Quoted 12 lines

Quoted 8 lines

mm/oom_kill.c +2 -0

+		if (process_selected)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Signature · 1 line

1.7.10.4

Signature · 2 lines

Michal Hocko
SUSE Labs

Been reviewing kernel code lately and looking for implementations not fulfilling their actual intention. That's about most of the patches I tend to send. Motivation is pretty much derived from the Eudyptula challenge so there is not concrete reason for this patch.

To the point: I have not witnessed any major affects to performance due to this. I fixed the patch and attached it to this mail.

Michal Hocko

2014-09-12 12:18 UTC #5 lore raw

On Fri 12-09-14 04:23:29, Niv Yehezkel wrote:
[...]

Quoted 6 lines

From 1e92f232e9367565d93629b54117b27b9bbfebda Mon Sep 17 00:00:00 2001
From: Niv Yehezkel <[email protected]>
Date: Fri, 12 Sep 2014 04:21:48 -0400
Subject: [PATCH] break after selecting process to kill

Now the justification please ;)

Quoted 30 lines

Signed-off-by Niv Yehezkel <[email protected]>

---
 mm/oom_kill.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/oom_kill.c +3 -1

index 1e11df8..3203578 100644
@@ -315,7 +315,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:

@@ -324,6 +324,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};
+		if (chosen_points == ULONG_MAX)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Signature · 1 line

1.7.10.4

Signature · 8 lines

Michal Hocko
SUSE Labs

--
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>

Niv Yehezkel

author

2014-09-12 12:22 UTC #6 lore raw

Quoted 33 lines · Michal Hocko

On Fri 12-09-14 04:23:29, Niv Yehezkel wrote:
[...]
Quoted 6 lines
From 1e92f232e9367565d93629b54117b27b9bbfebda Mon Sep 17 00:00:00 2001
From: Niv Yehezkel <[email protected]>
Date: Fri, 12 Sep 2014 04:21:48 -0400
Subject: [PATCH] break after selecting process to kill
Now the justification please ;)
Quoted 22 lines
Signed-off-by Niv Yehezkel <[email protected]>
---
 mm/oom_kill.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
mm/oom_kill.c +1 -1
index 1e11df8..3203578 100644
@@ -315,7 +315,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:
@@ -324,6 +324,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};

Quoted 13 lines

Quoted 8 lines

mm/oom_kill.c +2 -0

+		if (chosen_points == ULONG_MAX)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Signature · 1 line

1.7.10.4

Signature · 2 lines

Michal Hocko
SUSE Labs

As mentioned earlier, there's no need to keep iterating over all running processes once the process with the highest score has been found.

Michal Hocko

2014-09-12 12:31 UTC #7 lore raw

Quoted 48 lines · Niv Yehezkel

Quoted 33 lines · Michal Hocko
On Fri 12-09-14 04:23:29, Niv Yehezkel wrote:
[...]
Quoted 6 lines
From 1e92f232e9367565d93629b54117b27b9bbfebda Mon Sep 17 00:00:00 2001
From: Niv Yehezkel <[email protected]>
Date: Fri, 12 Sep 2014 04:21:48 -0400
Subject: [PATCH] break after selecting process to kill
Now the justification please ;)
Quoted 22 lines
Signed-off-by Niv Yehezkel <[email protected]>
---
 mm/oom_kill.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
mm/oom_kill.c +1 -1
index 1e11df8..3203578 100644
@@ -315,7 +315,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:
@@ -324,6 +324,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};
Quoted 13 lines
Quoted 8 lines
mm/oom_kill.c +2 -0
+		if (chosen_points == ULONG_MAX)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;
Signature · 1 line
1.7.10.4
Signature · 2 lines
Michal Hocko
SUSE Labs

Quoted 2 lines

As mentioned earlier, there's no need to keep iterating over all running processes once the process with the highest score has been found.

Please refer to Documentation/SubmittingPatches, especially "2) Describe your changes." section for more information about the preferred workflow. I really do not want to be nit picking on you but this is not the right way to send your changes.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
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>

David Rientjes

reviewer

2014-09-14 20:46 UTC #8 lore raw

In reply to Niv Yehezkel

Quoted 31 lines · Niv Yehezkel

Quoted 17 lines

mm/oom_kill.c +1 -1

index 1e11df8..3203578 100644
@@ -315,7 +315,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:

@@ -324,6 +324,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};

Quoted 13 lines

Quoted 8 lines

mm/oom_kill.c +2 -0

+		if (chosen_points == ULONG_MAX)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Signature · 1 line

1.7.10.4

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 3 lines

As mentioned earlier, there's no need to keep iterating over all running processes once the process with the highest score has been found.

This would lead to unnecessary oom killing since we may miss a process that returns OOM_SCAN_ABORT simply because it is later in the tasklist (we want to defer oom killing if there is an exiting process or an oom kill victim hasn't exited yet). NACK.

Michal Hocko

2014-09-23 18:30 UTC #9 lore raw

Quoted 33 lines · David Rientjes

Quoted 31 lines · Niv Yehezkel

Quoted 30 lines

Quoted 25 lines

mm/oom_kill.c +3 -1

index 1e11df8..3203578 100644
@@ -315,7 +315,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_SELECT:
 			chosen = p;
 			chosen_points = ULONG_MAX;
-			/* fall through */
+			break;
 		case OOM_SCAN_CONTINUE:
 			continue;
 		case OOM_SCAN_ABORT:

@@ -324,6 +324,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		case OOM_SCAN_OK:
 			break;
 		};
+		if (chosen_points == ULONG_MAX)
+			break;
 		points = oom_badness(p, NULL, nodemask, totalpages);
 		if (!points || points < chosen_points)
 			continue;

Signature · 1 line

1.7.10.4

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 8 lines

Quoted 3 lines
As mentioned earlier, there's no need to keep iterating over all running processes once the process with the highest score has been found.
This would lead to unnecessary oom killing since we may miss a process that returns OOM_SCAN_ABORT simply because it is later in the tasklist (we want to defer oom killing if there is an exiting process or an oom kill victim hasn't exited yet). NACK.

Good point David. I have completely missed this part!

Signature · 8 lines

Michal Hocko
SUSE Labs

--
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>