Re: [PATCH] mm/oom_kill: set oc->constraint in constrained_alloc()

[Yafang Shao <laoar.shao@gmail.com>]

On Fri, Jun 14, 2019 at 4:22 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Fri 14-06-19 13:58:11, Yafang Shao wrote:
> > On Fri, Jun 14, 2019 at 2:56 AM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Thu 13-06-19 21:55:50, Yafang Shao wrote:
> > > > In dump_oom_summary() oc->constraint is used to show
> > > > oom_constraint_text, but it hasn't been set before.
> > > > So the value of it is always the default value 0.
> > > > We should set it in constrained_alloc().
> > >
> > > Thanks for catching that.
> > >
> > > > Bellow is the output when memcg oom occurs,
> > > >
> > > > before this patch:
> > > > [  133.078102] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),
> > > > cpuset=/,mems_allowed=0,oom_memcg=/foo,task_memcg=/foo,task=bash,pid=7997,uid=0
> > > >
> > > > after this patch:
> > > > [  952.977946] oom-kill:constraint=CONSTRAINT_MEMCG,nodemask=(null),
> > > > cpuset=/,mems_allowed=0,oom_memcg=/foo,task_memcg=/foo,task=bash,pid=13681,uid=0
> > > >
> > >
> > > unless I am missing something
> > > Fixes: ef8444ea01d7 ("mm, oom: reorganize the oom report in dump_header")
> > >
> > > The patch looks correct but I think it is more complicated than it needs
> > > to be. Can we do the following instead?
> > >
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index 5a58778c91d4..f719b64741d6 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -987,8 +987,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
> > >  /*
> > >   * Determines whether the kernel must panic because of the panic_on_oom sysctl.
> > >   */
> > > -static void check_panic_on_oom(struct oom_control *oc,
> > > -                              enum oom_constraint constraint)
> > > +static void check_panic_on_oom(struct oom_control *oc)
> > >  {
> > >         if (likely(!sysctl_panic_on_oom))
> > >                 return;
> > > @@ -998,7 +997,7 @@ static void check_panic_on_oom(struct oom_control *oc,
> > >                  * does not panic for cpuset, mempolicy, or memcg allocation
> > >                  * failures.
> > >                  */
> > > -               if (constraint != CONSTRAINT_NONE)
> > > +               if (oc->constraint != CONSTRAINT_NONE)
> > >                         return;
> > >         }
> > >         /* Do not panic for oom kills triggered by sysrq */
> > > @@ -1035,7 +1034,6 @@ EXPORT_SYMBOL_GPL(unregister_oom_notifier);
> > >  bool out_of_memory(struct oom_control *oc)
> > >  {
> > >         unsigned long freed = 0;
> > > -       enum oom_constraint constraint = CONSTRAINT_NONE;
> > >
> > >         if (oom_killer_disabled)
> > >                 return false;
> > > @@ -1071,10 +1069,10 @@ bool out_of_memory(struct oom_control *oc)
> > >          * Check if there were limitations on the allocation (only relevant for
> > >          * NUMA and memcg) that may require different handling.
> > >          */
> > > -       constraint = constrained_alloc(oc);
> > > -       if (constraint != CONSTRAINT_MEMORY_POLICY)
> > > +       oc->constraint = constrained_alloc(oc);
> > > +       if (oc->constraint != CONSTRAINT_MEMORY_POLICY)
> > >                 oc->nodemask = NULL;
> > > -       check_panic_on_oom(oc, constraint);
> > > +       check_panic_on_oom(oc);
> > >
> > >         if (!is_memcg_oom(oc) && sysctl_oom_kill_allocating_task &&
> > >             current->mm && !oom_unkillable_task(current, NULL, oc->nodemask) &&
> > >
> > > I guess the current confusion comes from the fact that we have
> > > constraint both in the oom_control and a local variable so I would
> > > rather remove that. What do you think?
> >
> > Remove the local variable is fine by me.
>
> Could you repost the patch with the changelog mentioning Fixes and the
> simpler diff please?
>
> You can then add
> Acked-by: Michal Hocko <mhocko@suse.com>
>

Sure, I will.

Thanks

Yafang