Re: [PATCH] mm/oom: Add oom_score_adj value to oom Killed process message

[Michal Hocko <mhocko@kernel.org>]

On Wed 21-08-19 15:25:13, Edward Chron wrote:
> On Tue, Aug 20, 2019 at 8:25 PM David Rientjes <rientjes@google.com> wrote:
> >
> > On Tue, 20 Aug 2019, Edward Chron wrote:
> >
> > > For an OOM event: print oom_score_adj value for the OOM Killed process to
> > > document what the oom score adjust value was at the time the process was
> > > OOM Killed. The adjustment value can be set by user code and it affects
> > > the resulting oom_score so it is used to influence kill process selection.
> > >
> > > When eligible tasks are not printed (sysctl oom_dump_tasks = 0) printing
> > > this value is the only documentation of the value for the process being
> > > killed. Having this value on the Killed process message documents if a
> > > miscconfiguration occurred or it can confirm that the oom_score_adj
> > > value applies as expected.
> > >
> > > An example which illustates both misconfiguration and validation that
> > > the oom_score_adj was applied as expected is:
> > >
> > > Aug 14 23:00:02 testserver kernel: Out of memory: Killed process 2692
> > >  (systemd-udevd) total-vm:1056800kB, anon-rss:1052760kB, file-rss:4kB,
> > >  shmem-rss:0kB oom_score_adj:1000
> > >
> > > The systemd-udevd is a critical system application that should have an
> > > oom_score_adj of -1000. Here it was misconfigured to have a adjustment
> > > of 1000 making it a highly favored OOM kill target process. The output
> > > documents both the misconfiguration and the fact that the process
> > > was correctly targeted by OOM due to the miconfiguration. Having
> > > the oom_score_adj on the Killed message ensures that it is documented.
> > >
> > > Signed-off-by: Edward Chron <echron@arista.com>
> > > Acked-by: Michal Hocko <mhocko@suse.com>
> >
> > Acked-by: David Rientjes <rientjes@google.com>
> >
> > vm.oom_dump_tasks is pretty useful, however, so it's curious why you
> > haven't left it enabled :/
> >
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index eda2e2a0bdc6..c781f73b6cd6 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -884,12 +884,13 @@ static void __oom_kill_process(struct task_struct *victim, const char *message)
> > >        */
> > >       do_send_sig_info(SIGKILL, SEND_SIG_PRIV, victim, PIDTYPE_TGID);
> > >       mark_oom_victim(victim);
> > > -     pr_err("%s: Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
> > > +     pr_err("%s: Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB oom_score_adj:%ld\n",
> > >               message, task_pid_nr(victim), victim->comm,
> > >               K(victim->mm->total_vm),
> > >               K(get_mm_counter(victim->mm, MM_ANONPAGES)),
> > >               K(get_mm_counter(victim->mm, MM_FILEPAGES)),
> > > -             K(get_mm_counter(victim->mm, MM_SHMEMPAGES)));
> > > +             K(get_mm_counter(victim->mm, MM_SHMEMPAGES)),
> > > +             (long)victim->signal->oom_score_adj);
> > >       task_unlock(victim);
> > >
> > >       /*
> >
> > Nit: why not just use %hd and avoid the cast to long?
> 
> Sorry I may have accidently top posted my response to this. Here is
> where my response should go:
> -----------------------------------------------------------------------------------------------------------------------------------
> 
> Good point, I can post this with your correction.
> 
> I will add your Acked-by: David Rientjes <rientjes@google.com>
> 
> I am adding your Acked-by to the revised patch as this is what Michal
> asked me to do (so I assume that is what I should do).
> 
> Should I post as a separate fix again or simply post here?

Andrew usually folds these small fixups automagically. If that doesn't
happen here for some reason then just repost with acks and the fixup.

Thanks!

-- 
Michal Hocko
SUSE Labs