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
