Re: [PATCH] mm/oom: added option 'oom_dump_task_cmdline'

["Bujnak, Stepan" <stepan@pex.com>]

On Wed, Feb 20, 2019 at 7:49 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Wed 20-02-19 04:22:45, Stepan Bujnak wrote:
> > When oom_dump_tasks is enabled, this option will try to display task
> > cmdline instead of the command name in the system-wide task dump.
> >
> > This is useful in some cases e.g. on postgres server. If OOM killer is
> > invoked it will show a bunch of tasks called 'postgres'. With this
> > option enabled it will show additional information like the database
> > user, database name and what it is currently doing.
> >
> > Other example is python. Instead of just 'python' it will also show the
> > script name currently being executed.
>
> The size of OOM report output is quite large already and this will just
> add much more for some workloads and printing from this context is quite
> a problem already.
>

The option defaults to false so most workloads wouldn't be affected.
As an alternative the cmdline line can only be printed for the
victim task in the OOM summary.

> > Signed-off-by: Stepan Bujnak <stepan@pex.com>
> > ---
> >  Documentation/sysctl/vm.txt | 10 ++++++++++
> >  include/linux/oom.h         |  1 +
> >  kernel/sysctl.c             |  7 +++++++
> >  mm/oom_kill.c               | 20 ++++++++++++++++++--
> >  4 files changed, 36 insertions(+), 2 deletions(-)
> >
> [...]
> > @@ -404,9 +406,18 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask)
> >       pr_info("[  pid  ]   uid  tgid total_vm      rss pgtables_bytes swapents oom_score_adj name\n");
> >       rcu_read_lock();
> >       for_each_process(p) {
> > +             char *name, *cmd = NULL;
> > +
> >               if (oom_unkillable_task(p, memcg, nodemask))
> >                       continue;
> >
> > +             /*
> > +              * This needs to be done before calling find_lock_task_mm()
> > +              * since both grab a task lock which would result in deadlock.
> > +              */
> > +             if (sysctl_oom_dump_task_cmdline)
> > +                     cmd = kstrdup_quotable_cmdline(p, GFP_KERNEL);
> > +
> >               task = find_lock_task_mm(p);
> >               if (!task) {
> >                       /*
> You are trying to allocate from the OOM context. That is a big no no.
> Not to mention that this is deadlock prone because get_cmdline needs
> mmap_sem and the allocating context migh hold the lock already. So the
> patch is simply wrong.
>

Thanks for the notes. I understand how allocating from OOM context
is a problem. However I still believe that this would be helpful
for debugging OOM kills since task->comm is often not descriptive
enough. Would it help if instead of calling kstrdup_quotable_cmdline()
which allocates the buffer on heap I called get_cmdline() directly
passing it stack-allocated buffer of certain size e.g. 256?

> --
> Michal Hocko
> SUSE Labs