Re: [PATCH] mm, oom: show process exiting information in __oom_kill_process()

[Michal Hocko <mhocko@kernel.org>]

On Mon 20-07-20 18:36:53, Yafang Shao wrote:
> On Mon, Jul 20, 2020 at 3:16 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Sun 19-07-20 09:53:15, Yafang Shao wrote:
> > > When the OOM killer finding a victim and trying to kill it, if the victim
> > > is already exiting, the task mm will be NULL and no process will be killed.
> > > But the dump_header() has been already executed, so it will be strange to
> > > dump so many information without killing a process. We'd better show some
> > > helpful information to indicate why this happens.
> > >
> > > Suggested-by: David Rientjes <rientjes@google.com>
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Cc: Michal Hocko <mhocko@kernel.org>
> > > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> > > ---
> > >  mm/oom_kill.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index 6e94962..0480dde 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -863,9 +863,13 @@ static void __oom_kill_process(struct task_struct *victim, const char *message)
> > >
> > >       p = find_lock_task_mm(victim);
> > >       if (!p) {
> > > +             pr_info("Process %d (%s) is already exiting\n",
> > > +                     task_pid_nr(victim), victim->comm);
> > >               put_task_struct(victim);
> >
> > I do agree that a silent bail out is not the best thing to do. The above
> > message would be more useful if it also explained what the oom killer
> > does (or does not):
> >
> >         "OOM victim %d (%s) is already exiting. Skip killing the task\n"
> >
> 
> Sure.
> 
> > >               return;
> > > -     } else if (victim != p) {
> > > +     }
> > > +
> > > +     if (victim != p) {
> >
> > Why do we need this?
> >
> 
> Because I don't  like that code style.

We usually prefer to keep unrelated changes separate. Minor coding
style changes are usually questionable because many people have
different sense for the code.

> But it is not a big problem, I will not change it in the next version.

Thanks
-- 
Michal Hocko
SUSE Labs