* [patch -mm] mm, oom: prefer thread group leaders for display purposes
@ 2014-01-16 2:40 David Rientjes
2014-01-16 7:05 ` Johannes Weiner
0 siblings, 1 reply; 7+ messages in thread
From: David Rientjes @ 2014-01-16 2:40 UTC (permalink / raw)
To: Andrew Morton
Cc: Johannes Weiner, Michal Hocko, KAMEZAWA Hiroyuki, cgroups,
linux-mm, linux-kernel
When two threads have the same badness score, it's preferable to kill the
thread group leader so that the actual process name is printed to the
kernel log rather than the thread group name which may be shared amongst
several processes.
This was the behavior when select_bad_process() used to do
for_each_process(), but it now iterates threads instead and leads to
ambiguity.
Signed-off-by: David Rientjes <rientjes@google.com>
---
mm/memcontrol.c | 18 +++++++++++-------
mm/oom_kill.c | 12 ++++++++----
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a815686..b482f49 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1841,13 +1841,17 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
break;
};
points = oom_badness(task, memcg, NULL, totalpages);
- if (points > chosen_points) {
- if (chosen)
- put_task_struct(chosen);
- chosen = task;
- chosen_points = points;
- get_task_struct(chosen);
- }
+ if (points < chosen_points)
+ continue;
+ /* Prefer thread group leaders for display purposes */
+ if (points == chosen_points &&
+ thread_group_leader(chosen))
+ continue;
+
+ if (chosen)
+ put_task_struct(chosen);
+ chosen = task;
+ chosen_points = points;
}
css_task_iter_end(&it);
}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 054ff47..1dca3d8 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -327,10 +327,14 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
break;
};
points = oom_badness(p, NULL, nodemask, totalpages);
- if (points > chosen_points) {
- chosen = p;
- chosen_points = points;
- }
+ if (points < chosen_points)
+ continue;
+ /* Prefer thread group leaders for display purposes */
+ if (points == chosen_points && thread_group_leader(chosen))
+ continue;
+
+ chosen = p;
+ chosen_points = points;
}
if (chosen)
get_task_struct(chosen);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [patch -mm] mm, oom: prefer thread group leaders for display purposes
2014-01-16 2:40 [patch -mm] mm, oom: prefer thread group leaders for display purposes David Rientjes
@ 2014-01-16 7:05 ` Johannes Weiner
2014-01-16 7:45 ` David Rientjes
0 siblings, 1 reply; 7+ messages in thread
From: Johannes Weiner @ 2014-01-16 7:05 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Michal Hocko, KAMEZAWA Hiroyuki, cgroups,
linux-mm, linux-kernel
On Wed, Jan 15, 2014 at 06:40:41PM -0800, David Rientjes wrote:
> When two threads have the same badness score, it's preferable to kill the
> thread group leader so that the actual process name is printed to the
> kernel log rather than the thread group name which may be shared amongst
> several processes.
>
> This was the behavior when select_bad_process() used to do
> for_each_process(), but it now iterates threads instead and leads to
> ambiguity.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
> mm/memcontrol.c | 18 +++++++++++-------
> mm/oom_kill.c | 12 ++++++++----
> 2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a815686..b482f49 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1841,13 +1841,17 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
> break;
> };
> points = oom_badness(task, memcg, NULL, totalpages);
> - if (points > chosen_points) {
> - if (chosen)
> - put_task_struct(chosen);
> - chosen = task;
> - chosen_points = points;
> - get_task_struct(chosen);
Where did that GET go?
> - }
> + if (points < chosen_points)
> + continue;
> + /* Prefer thread group leaders for display purposes */
> + if (points == chosen_points &&
> + thread_group_leader(chosen))
> + continue;
> +
> + if (chosen)
> + put_task_struct(chosen);
...boom.
> + chosen = task;
> + chosen_points = points;
> }
> css_task_iter_end(&it);
> }
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 054ff47..1dca3d8 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -327,10 +327,14 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
> break;
> };
> points = oom_badness(p, NULL, nodemask, totalpages);
> - if (points > chosen_points) {
> - chosen = p;
> - chosen_points = points;
> - }
> + if (points < chosen_points)
> + continue;
> + /* Prefer thread group leaders for display purposes */
> + if (points == chosen_points && thread_group_leader(chosen))
> + continue;
> +
> + chosen = p;
> + chosen_points = points;
> }
> if (chosen)
> get_task_struct(chosen);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch -mm] mm, oom: prefer thread group leaders for display purposes
2014-01-16 7:05 ` Johannes Weiner
@ 2014-01-16 7:45 ` David Rientjes
2014-01-16 7:46 ` [patch v2 " David Rientjes
0 siblings, 1 reply; 7+ messages in thread
From: David Rientjes @ 2014-01-16 7:45 UTC (permalink / raw)
To: Johannes Weiner
Cc: Andrew Morton, Michal Hocko, KAMEZAWA Hiroyuki, cgroups,
linux-mm, linux-kernel
On Thu, 16 Jan 2014, Johannes Weiner wrote:
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index a815686..b482f49 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1841,13 +1841,17 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
> > break;
> > };
> > points = oom_badness(task, memcg, NULL, totalpages);
> > - if (points > chosen_points) {
> > - if (chosen)
> > - put_task_struct(chosen);
> > - chosen = task;
> > - chosen_points = points;
> > - get_task_struct(chosen);
>
> Where did that GET go?
>
No idea, good catch! This patch was doomed from the GET-go.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [patch v2 -mm] mm, oom: prefer thread group leaders for display purposes
2014-01-16 7:45 ` David Rientjes
@ 2014-01-16 7:46 ` David Rientjes
2014-01-16 14:21 ` Michal Hocko
0 siblings, 1 reply; 7+ messages in thread
From: David Rientjes @ 2014-01-16 7:46 UTC (permalink / raw)
To: Andrew Morton
Cc: Johannes Weiner, Michal Hocko, KAMEZAWA Hiroyuki, cgroups,
linux-mm, linux-kernel
When two threads have the same badness score, it's preferable to kill the
thread group leader so that the actual process name is printed to the
kernel log rather than the thread group name which may be shared amongst
several processes.
This was the behavior when select_bad_process() used to do
for_each_process(), but it now iterates threads instead and leads to
ambiguity.
Signed-off-by: David Rientjes <rientjes@google.com>
---
v2: fixes missing get_task_struct() found by Johannes.
mm/memcontrol.c | 19 ++++++++++++-------
mm/oom_kill.c | 12 ++++++++----
2 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a815686..d69c4b3 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1841,13 +1841,18 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
break;
};
points = oom_badness(task, memcg, NULL, totalpages);
- if (points > chosen_points) {
- if (chosen)
- put_task_struct(chosen);
- chosen = task;
- chosen_points = points;
- get_task_struct(chosen);
- }
+ if (points < chosen_points)
+ continue;
+ /* Prefer thread group leaders for display purposes */
+ if (points == chosen_points &&
+ thread_group_leader(chosen))
+ continue;
+
+ if (chosen)
+ put_task_struct(chosen);
+ chosen = task;
+ chosen_points = points;
+ get_task_struct(chosen);
}
css_task_iter_end(&it);
}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 054ff47..1dca3d8 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -327,10 +327,14 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
break;
};
points = oom_badness(p, NULL, nodemask, totalpages);
- if (points > chosen_points) {
- chosen = p;
- chosen_points = points;
- }
+ if (points < chosen_points)
+ continue;
+ /* Prefer thread group leaders for display purposes */
+ if (points == chosen_points && thread_group_leader(chosen))
+ continue;
+
+ chosen = p;
+ chosen_points = points;
}
if (chosen)
get_task_struct(chosen);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [patch v2 -mm] mm, oom: prefer thread group leaders for display purposes
2014-01-16 7:46 ` [patch v2 " David Rientjes
@ 2014-01-16 14:21 ` Michal Hocko
2014-01-16 22:49 ` David Rientjes
0 siblings, 1 reply; 7+ messages in thread
From: Michal Hocko @ 2014-01-16 14:21 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Johannes Weiner, KAMEZAWA Hiroyuki, cgroups,
linux-mm, linux-kernel
On Wed 15-01-14 23:46:44, David Rientjes wrote:
> When two threads have the same badness score, it's preferable to kill the
> thread group leader so that the actual process name is printed to the
> kernel log rather than the thread group name which may be shared amongst
> several processes.
I am not sure I understand this. Is this about ->comm? If yes then why
couldn't the group leader do PR_SET_NAME?
> This was the behavior when select_bad_process() used to do
> for_each_process(), but it now iterates threads instead and leads to
> ambiguity.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
> v2: fixes missing get_task_struct() found by Johannes.
>
> mm/memcontrol.c | 19 ++++++++++++-------
> mm/oom_kill.c | 12 ++++++++----
> 2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a815686..d69c4b3 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1841,13 +1841,18 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
> break;
> };
> points = oom_badness(task, memcg, NULL, totalpages);
> - if (points > chosen_points) {
> - if (chosen)
> - put_task_struct(chosen);
> - chosen = task;
> - chosen_points = points;
> - get_task_struct(chosen);
> - }
> + if (points < chosen_points)
> + continue;
> + /* Prefer thread group leaders for display purposes */
> + if (points == chosen_points &&
> + thread_group_leader(chosen))
> + continue;
> +
> + if (chosen)
> + put_task_struct(chosen);
> + chosen = task;
> + chosen_points = points;
> + get_task_struct(chosen);
> }
> css_task_iter_end(&it);
> }
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 054ff47..1dca3d8 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -327,10 +327,14 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
> break;
> };
> points = oom_badness(p, NULL, nodemask, totalpages);
> - if (points > chosen_points) {
> - chosen = p;
> - chosen_points = points;
> - }
> + if (points < chosen_points)
> + continue;
> + /* Prefer thread group leaders for display purposes */
> + if (points == chosen_points && thread_group_leader(chosen))
> + continue;
> +
> + chosen = p;
> + chosen_points = points;
> }
> if (chosen)
> get_task_struct(chosen);
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch v2 -mm] mm, oom: prefer thread group leaders for display purposes
2014-01-16 14:21 ` Michal Hocko
@ 2014-01-16 22:49 ` David Rientjes
2014-01-17 9:17 ` Michal Hocko
0 siblings, 1 reply; 7+ messages in thread
From: David Rientjes @ 2014-01-16 22:49 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Johannes Weiner, KAMEZAWA Hiroyuki, cgroups,
linux-mm, linux-kernel
On Thu, 16 Jan 2014, Michal Hocko wrote:
> > When two threads have the same badness score, it's preferable to kill the
> > thread group leader so that the actual process name is printed to the
> > kernel log rather than the thread group name which may be shared amongst
> > several processes.
>
> I am not sure I understand this. Is this about ->comm? If yes then why
> couldn't the group leader do PR_SET_NAME?
>
Both comm and pid, we only display thread group leaders in the tasklist
dump of eligible processes, we want the killed message to specify from
which process.
You're suggesting a thread group leader do PR_SET_NAME of all its threads
for readable oom killer output? Lol.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch v2 -mm] mm, oom: prefer thread group leaders for display purposes
2014-01-16 22:49 ` David Rientjes
@ 2014-01-17 9:17 ` Michal Hocko
0 siblings, 0 replies; 7+ messages in thread
From: Michal Hocko @ 2014-01-17 9:17 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Johannes Weiner, KAMEZAWA Hiroyuki, cgroups,
linux-mm, linux-kernel
On Thu 16-01-14 14:49:25, David Rientjes wrote:
> On Thu, 16 Jan 2014, Michal Hocko wrote:
>
> > > When two threads have the same badness score, it's preferable to kill the
> > > thread group leader so that the actual process name is printed to the
> > > kernel log rather than the thread group name which may be shared amongst
> > > several processes.
> >
> > I am not sure I understand this. Is this about ->comm? If yes then why
> > couldn't the group leader do PR_SET_NAME?
> >
>
> Both comm and pid, we only display thread group leaders in the tasklist
> dump of eligible processes, we want the killed message to specify from
> which process.
OK, that makes sense now. I didn't think about dump_tasks and
consistency in the output.
> You're suggesting a thread group leader do PR_SET_NAME of all its threads
> for readable oom killer output? Lol.
No, I am not suggesting anything. I was just asking why is group leader
different in this regards becasue changelog didn't tell it and I didn't
put it together with dump_tasks.
Thanks!
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-01-17 9:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-16 2:40 [patch -mm] mm, oom: prefer thread group leaders for display purposes David Rientjes
2014-01-16 7:05 ` Johannes Weiner
2014-01-16 7:45 ` David Rientjes
2014-01-16 7:46 ` [patch v2 " David Rientjes
2014-01-16 14:21 ` Michal Hocko
2014-01-16 22:49 ` David Rientjes
2014-01-17 9:17 ` Michal Hocko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).