* [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks
@ 2021-06-12 20:46 Aaron Tomlin
2021-06-13 23:47 ` David Rientjes
2021-06-14 6:44 ` Michal Hocko
0 siblings, 2 replies; 7+ messages in thread
From: Aaron Tomlin @ 2021-06-12 20:46 UTC (permalink / raw)
To: linux-mm; +Cc: akpm, vbabka, mhocko, penguin-kernel, llong, linux-kernel
Changes since v2:
- Use single character (e.g. 'R' for MMF_OOM_SKIP) as suggested
by Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
- Add new header to oom_dump_tasks documentation
At the present time, when showing potential OOM victims, we do not
exclude tasks which already have MMF_OOM_SKIP set; it is possible that
the last OOM killable victim was already OOM killed, yet the OOM
reaper failed to reclaim memory and set MMF_OOM_SKIP.
This can be confusing/or perhaps even misleading, to the reader of the
OOM report. Now, we already unconditionally display a task's
oom_score_adj_min value that can be set to OOM_SCORE_ADJ_MIN which is
indicative of an "unkillable" task i.e. is not eligible.
This patch provides a clear indication with regard to the OOM
eligibility of each displayed task.
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
Documentation/admin-guide/sysctl/vm.rst | 5 ++--
mm/oom_kill.c | 31 +++++++++++++++++++++----
2 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/Documentation/admin-guide/sysctl/vm.rst b/Documentation/admin-guide/sysctl/vm.rst
index 586cd4b86428..123be642bc7e 100644
--- a/Documentation/admin-guide/sysctl/vm.rst
+++ b/Documentation/admin-guide/sysctl/vm.rst
@@ -658,8 +658,9 @@ oom_dump_tasks
Enables a system-wide task dump (excluding kernel threads) to be produced
when the kernel performs an OOM-killing and includes such information as
pid, uid, tgid, vm size, rss, pgtables_bytes, swapents, oom_score_adj
-score, and name. This is helpful to determine why the OOM killer was
-invoked, to identify the rogue task that caused it, and to determine why
+score, oom eligibility status and name. This is helpful to determine why
+the OOM killer was invoked, to identify the rogue task that caused it, and
+to determine why
the OOM killer chose the task it did to kill.
If this is set to zero, this information is suppressed. On very
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index eefd3f5fde46..094b7b61d66f 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -160,6 +160,27 @@ static inline bool is_sysrq_oom(struct oom_control *oc)
return oc->order == -1;
}
+/**
+ * is_task_eligible_oom - determine if and why a task cannot be OOM killed
+ * @tsk: task to check
+ *
+ * Needs to be called with task_lock().
+ */
+static const char * is_task_oom_eligible(struct task_struct *p)
+{
+ long adj;
+
+ adj = (long)p->signal->oom_score_adj;
+ if (adj == OOM_SCORE_ADJ_MIN)
+ return "S";
+ else if (test_bit(MMF_OOM_SKIP, &p->mm->flags)
+ return "R";
+ else if (in_vfork(p))
+ return "V";
+ else
+ return "";
+}
+
/* return true if the task is not adequate as candidate victim task. */
static bool oom_unkillable_task(struct task_struct *p)
{
@@ -401,12 +422,13 @@ static int dump_task(struct task_struct *p, void *arg)
return 0;
}
- pr_info("[%7d] %5d %5d %8lu %8lu %8ld %8lu %5hd %s\n",
+ pr_info("[%7d] %5d %5d %8lu %8lu %8ld %8lu %5hd %13s %s\n",
task->pid, from_kuid(&init_user_ns, task_uid(task)),
task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
mm_pgtables_bytes(task->mm),
get_mm_counter(task->mm, MM_SWAPENTS),
- task->signal->oom_score_adj, task->comm);
+ task->signal->oom_score_adj, is_task_oom_eligible(task),
+ task->comm);
task_unlock(task);
return 0;
@@ -420,12 +442,13 @@ static int dump_task(struct task_struct *p, void *arg)
* memcg, not in the same cpuset, or bound to a disjoint set of mempolicy nodes
* are not shown.
* State information includes task's pid, uid, tgid, vm size, rss,
- * pgtables_bytes, swapents, oom_score_adj value, and name.
+ * pgtables_bytes, swapents, oom_score_adj value, oom eligibility status
+ * and name.
*/
static void dump_tasks(struct oom_control *oc)
{
pr_info("Tasks state (memory values in pages):\n");
- pr_info("[ pid ] uid tgid total_vm rss pgtables_bytes swapents oom_score_adj name\n");
+ pr_info("[ pid ] uid tgid total_vm rss pgtables_bytes swapents oom_score_adj oom eligible? name\n");
if (is_memcg_oom(oc))
mem_cgroup_scan_tasks(oc->memcg, dump_task, oc);
--
2.26.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks
2021-06-12 20:46 [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks Aaron Tomlin
@ 2021-06-13 23:47 ` David Rientjes
2021-06-15 12:02 ` Aaron Tomlin
2021-06-14 6:44 ` Michal Hocko
1 sibling, 1 reply; 7+ messages in thread
From: David Rientjes @ 2021-06-13 23:47 UTC (permalink / raw)
To: Aaron Tomlin
Cc: linux-mm, akpm, vbabka, mhocko, penguin-kernel, llong,
linux-kernel
On Sat, 12 Jun 2021, Aaron Tomlin wrote:
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index eefd3f5fde46..094b7b61d66f 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -160,6 +160,27 @@ static inline bool is_sysrq_oom(struct oom_control *oc)
> return oc->order == -1;
> }
>
> +/**
> + * is_task_eligible_oom - determine if and why a task cannot be OOM killed
> + * @tsk: task to check
> + *
> + * Needs to be called with task_lock().
> + */
> +static const char * is_task_oom_eligible(struct task_struct *p)
You should be able to just return a char.
> +{
> + long adj;
> +
> + adj = (long)p->signal->oom_score_adj;
> + if (adj == OOM_SCORE_ADJ_MIN)
> + return "S";
The value is already printed in the task dump, this doesn't look to add
any information.
> + else if (test_bit(MMF_OOM_SKIP, &p->mm->flags)
> + return "R";
We should be doing the task dump only when we're killing a victim (unless
we're panicking), so something else has been chosen. Since we would have
oom killed a process with MMF_OOM_SKIP already, can we simply choose to
not print a line for this process?
> + else if (in_vfork(p))
> + return "V";
Is this a transition state that we can simply disregard from the output as
well unless/until it becomes eligible?
> + else
> + return "";
> +}
> +
> /* return true if the task is not adequate as candidate victim task. */
> static bool oom_unkillable_task(struct task_struct *p)
> {
> @@ -401,12 +422,13 @@ static int dump_task(struct task_struct *p, void *arg)
> return 0;
> }
>
> - pr_info("[%7d] %5d %5d %8lu %8lu %8ld %8lu %5hd %s\n",
> + pr_info("[%7d] %5d %5d %8lu %8lu %8ld %8lu %5hd %13s %s\n",
13 characters for one char output?
> task->pid, from_kuid(&init_user_ns, task_uid(task)),
> task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
> mm_pgtables_bytes(task->mm),
> get_mm_counter(task->mm, MM_SWAPENTS),
> - task->signal->oom_score_adj, task->comm);
> + task->signal->oom_score_adj, is_task_oom_eligible(task),
> + task->comm);
> task_unlock(task);
>
> return 0;
> @@ -420,12 +442,13 @@ static int dump_task(struct task_struct *p, void *arg)
> * memcg, not in the same cpuset, or bound to a disjoint set of mempolicy nodes
> * are not shown.
> * State information includes task's pid, uid, tgid, vm size, rss,
> - * pgtables_bytes, swapents, oom_score_adj value, and name.
> + * pgtables_bytes, swapents, oom_score_adj value, oom eligibility status
> + * and name.
> */
> static void dump_tasks(struct oom_control *oc)
> {
> pr_info("Tasks state (memory values in pages):\n");
> - pr_info("[ pid ] uid tgid total_vm rss pgtables_bytes swapents oom_score_adj name\n");
> + pr_info("[ pid ] uid tgid total_vm rss pgtables_bytes swapents oom_score_adj oom eligible? name\n");
Field names are single words.
>
> if (is_memcg_oom(oc))
> mem_cgroup_scan_tasks(oc->memcg, dump_task, oc);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks
2021-06-12 20:46 [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks Aaron Tomlin
2021-06-13 23:47 ` David Rientjes
@ 2021-06-14 6:44 ` Michal Hocko
2021-06-15 11:51 ` Aaron Tomlin
1 sibling, 1 reply; 7+ messages in thread
From: Michal Hocko @ 2021-06-14 6:44 UTC (permalink / raw)
To: Aaron Tomlin; +Cc: linux-mm, akpm, vbabka, penguin-kernel, llong, linux-kernel
On Sat 12-06-21 21:46:34, Aaron Tomlin wrote:
> Changes since v2:
> - Use single character (e.g. 'R' for MMF_OOM_SKIP) as suggested
> by Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> - Add new header to oom_dump_tasks documentation
>
>
> At the present time, when showing potential OOM victims, we do not
> exclude tasks which already have MMF_OOM_SKIP set; it is possible that
> the last OOM killable victim was already OOM killed, yet the OOM
> reaper failed to reclaim memory and set MMF_OOM_SKIP.
> This can be confusing/or perhaps even misleading, to the reader of the
> OOM report. Now, we already unconditionally display a task's
> oom_score_adj_min value that can be set to OOM_SCORE_ADJ_MIN which is
> indicative of an "unkillable" task i.e. is not eligible.
Well, I have to say that I have a bit hard time understand the problem
statement here. First of all you are very likely basing your observation
on an old kernel which is missing a fix which should make the situation
impossible IIRC. You should be focusing on a justification why the new
information is helpful for the current tree.
Historically, all tasks eligible for the oom killing have been
printed. That includes also tasks which are excluded later in the
selection. E.g. OOMS_SCORE_ADJ_MIN which can be tricky indeed. IIRC the
primary reason was to have a sufficient amount of information to
evaluate whether the system is configured properly (e.g.
OOMS_SCORE_ADJ_MIN is not used too extensively). More excluded
criterion have been added due to implementation details
(e.g.MMF_OOM_SKIP or mm shared with otherwise ineligible task.
You are correctly pointing out that those internal states are not
exposed but you should focus on explanation why that gap really stands
in the way for the current upstream. Who is going to consume that
information and for what purpose?
> This patch provides a clear indication with regard to the OOM
> eligibility of each displayed task.
This should provide an example of the output with a clarification of the
meaning.
[...]
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks
2021-06-14 6:44 ` Michal Hocko
@ 2021-06-15 11:51 ` Aaron Tomlin
2021-06-15 12:42 ` Michal Hocko
0 siblings, 1 reply; 7+ messages in thread
From: Aaron Tomlin @ 2021-06-15 11:51 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm, akpm, vbabka, penguin-kernel, llong, linux-kernel
On Mon 2021-06-14 08:44 +0200, Michal Hocko wrote:
> Well, I have to say that I have a bit hard time understand the problem
> statement here. First of all you are very likely basing your observation
> on an old kernel which is missing a fix which should make the situation
> impossible IIRC. You should be focusing on a justification why the new
> information is helpful for the current tree.
Michal,
Not exactly.
See oom_reap_task(). Let's assume an OOM event occurred within the context
of a memcg and 'memory.oom.group' was not set. If I understand correctly,
once all attempts to OOM reap the specified task were "unsuccessful" then
MMF_OOM_SKIP is applied; and, the assumption is it will be terminated
shorty due to the pending fatal signal (see __oom_kill_process()) i.e. a
SIGKILL is sent to the "victim" before the OOM reaper is notified. Now
assuming the above task did not exited yet, another task, in the same
memcg, could also trigger an OOM event. Therefore, when showing potential
OOM victims the task above with MMF_OOM_SKIP set, will indeed be displayed.
I understanding the point on OOM_SCORE_ADJ_MIN. This can be easily
identified and is clear to the viewer. However, the same cannot be stated
for MMF_OOM_SKIP.
So, if we prefer to display rather than exclude such tasks, in my opinion
having a flag/or marker of some description might prove useful to avoid any
misunderstanding.
> This should provide an example of the output with a clarification of the
> meaning.
Fair enough.
Kind regards,
--
Aaron Tomlin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks
2021-06-13 23:47 ` David Rientjes
@ 2021-06-15 12:02 ` Aaron Tomlin
0 siblings, 0 replies; 7+ messages in thread
From: Aaron Tomlin @ 2021-06-15 12:02 UTC (permalink / raw)
To: David Rientjes
Cc: linux-mm, akpm, vbabka, mhocko, penguin-kernel, llong,
linux-kernel
On Sun 2021-06-13 16:47 -0700, David Rientjes wrote:
> On Sat, 12 Jun 2021, Aaron Tomlin wrote:
>
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index eefd3f5fde46..094b7b61d66f 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -160,6 +160,27 @@ static inline bool is_sysrq_oom(struct oom_control *oc)
> > return oc->order == -1;
> > }
> >
> > +/**
> > + * is_task_eligible_oom - determine if and why a task cannot be OOM killed
> > + * @tsk: task to check
> > + *
> > + * Needs to be called with task_lock().
> > + */
> > +static const char * is_task_oom_eligible(struct task_struct *p)
>
> You should be able to just return a char.
I see, sure.
> > +{
> > + long adj;
> > +
> > + adj = (long)p->signal->oom_score_adj;
> > + if (adj == OOM_SCORE_ADJ_MIN)
> > + return "S";
>
> The value is already printed in the task dump, this doesn't look to add
> any information.
I understand and perhaps it does not make sense; albeit, the reader might
not understand the meaning of OOM_SCORE_ADJ_MIN.
> > + else if (test_bit(MMF_OOM_SKIP, &p->mm->flags)
> > + return "R";
>
> We should be doing the task dump only when we're killing a victim (unless
> we're panicking), so something else has been chosen. Since we would have
> oom killed a process with MMF_OOM_SKIP already, can we simply choose to
> not print a line for this process?
I'd prefer not to show such tasks, when displaying potential OOM victims
(including those in_vfork()) as in my opinion, it can be misleading to the
reader. That said, a case has been made to maintain their inclusion.
However, should in_vfork() even be shown in the actual report?
>
> > @@ -401,12 +422,13 @@ static int dump_task(struct task_struct *p, void *arg)
> > return 0;
> > }
> >
> > - pr_info("[%7d] %5d %5d %8lu %8lu %8ld %8lu %5hd %s\n",
> > + pr_info("[%7d] %5d %5d %8lu %8lu %8ld %8lu %5hd %13s %s\n",
>
> 13 characters for one char output?
This was to maintain some alignment but fair enough.
> > static void dump_tasks(struct oom_control *oc)
> > {
> > pr_info("Tasks state (memory values in pages):\n");
> > - pr_info("[ pid ] uid tgid total_vm rss pgtables_bytes swapents oom_score_adj name\n");
> > + pr_info("[ pid ] uid tgid total_vm rss pgtables_bytes swapents oom_score_adj oom eligible? name\n");
>
> Field names are single words.
Understood.
Kind regards,
--
Aaron Tomlin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks
2021-06-15 11:51 ` Aaron Tomlin
@ 2021-06-15 12:42 ` Michal Hocko
2021-06-16 20:18 ` Aaron Tomlin
0 siblings, 1 reply; 7+ messages in thread
From: Michal Hocko @ 2021-06-15 12:42 UTC (permalink / raw)
To: Aaron Tomlin; +Cc: linux-mm, akpm, vbabka, penguin-kernel, llong, linux-kernel
On Tue 15-06-21 12:51:47, Aaron Tomlin wrote:
> On Mon 2021-06-14 08:44 +0200, Michal Hocko wrote:
> > Well, I have to say that I have a bit hard time understand the problem
> > statement here. First of all you are very likely basing your observation
> > on an old kernel which is missing a fix which should make the situation
> > impossible IIRC. You should be focusing on a justification why the new
> > information is helpful for the current tree.
>
> Michal,
>
> Not exactly.
>
> See oom_reap_task(). Let's assume an OOM event occurred within the context
> of a memcg and 'memory.oom.group' was not set. If I understand correctly,
> once all attempts to OOM reap the specified task were "unsuccessful" then
> MMF_OOM_SKIP is applied; and, the assumption is it will be terminated
> shorty due to the pending fatal signal (see __oom_kill_process()) i.e. a
> SIGKILL is sent to the "victim" before the OOM reaper is notified. Now
> assuming the above task did not exited yet, another task, in the same
> memcg, could also trigger an OOM event. Therefore, when showing potential
> OOM victims the task above with MMF_OOM_SKIP set, will indeed be displayed.
>
> I understanding the point on OOM_SCORE_ADJ_MIN. This can be easily
> identified and is clear to the viewer. However, the same cannot be stated
> for MMF_OOM_SKIP.
This is all true but it is not really clear why that is really a
problem. Kernel log already contains information about reaped processes
as they are reported to the log. I fully acknowledge that this is rather
spartan but on the other hand from years of experience reading oom
reports I have to say the dump_tasks is the least interesting part of
the report (while being the most verbose one).
All that being said, I am not really opposing extending the information
although I am a bit worried about leaking too much internal state to the
log. What I am asking for here is a justification why this addition is a
general improvement and how it helps uderstanding oom reports further.
So please focus on that part.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks
2021-06-15 12:42 ` Michal Hocko
@ 2021-06-16 20:18 ` Aaron Tomlin
0 siblings, 0 replies; 7+ messages in thread
From: Aaron Tomlin @ 2021-06-16 20:18 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm, akpm, vbabka, penguin-kernel, llong, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]
On Tue 2021-06-15 14:42 +0200, Michal Hocko wrote:
> This is all true but it is not really clear why that is really a
> problem. Kernel log already contains information about reaped processes
> as they are reported to the log. I fully acknowledge that this is rather
> spartan but on the other hand from years of experience reading oom
> reports I have to say the dump_tasks is the least interesting part of
> the report (while being the most verbose one).
I understand. I suppose, in a situation whereby dump_tasks() output is only
available, for whatever reason, it can provide at least some
insight into what tasks were actually considered not OOM eligible and why.
> All that being said, I am not really opposing extending the information
> although I am a bit worried about leaking too much internal state to the
> log.
Fair enough. That said, I still feel highlighting such "ineligible" tasks
could be useful to the viewer for troubleshooting purposes; we already
display OOM_SCORE_ADJ_MIN. Consider a situation when only a few tasks in
a memcg are displayed as possibly OOM eligible but one had MMF_OOM_SKIP
applied.
In my opinion, perhaps it is better to just exclude such details
altogether. That being said, as you know, we only provide this facility
when one is interested in it anyway i.e., if oom_dump_tasks is enabled.
> What I am asking for here is a justification why this addition is a
> general improvement and how it helps uderstanding oom reports further.
> So please focus on that part.
Sure; albeit, thinking about this more, it does not provide much
understanding in simple isolation.
Kind regards,
--
Aaron Tomlin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-16 20:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-12 20:46 [PATCH v2] mm/oom_kill: show oom eligibility when displaying the current memory state of all tasks Aaron Tomlin
2021-06-13 23:47 ` David Rientjes
2021-06-15 12:02 ` Aaron Tomlin
2021-06-14 6:44 ` Michal Hocko
2021-06-15 11:51 ` Aaron Tomlin
2021-06-15 12:42 ` Michal Hocko
2021-06-16 20:18 ` Aaron Tomlin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.