Re: [PATCH] mm, oom: don't invoke oom killer if current has been reapered

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

On Mon, Jul 13, 2020 at 8:45 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Mon 13-07-20 20:24:07, Yafang Shao wrote:
> > On Mon, Jul 13, 2020 at 2:21 PM Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > On Mon 13-07-20 08:01:57, Michal Hocko wrote:
> > > > On Fri 10-07-20 23:18:01, Yafang Shao wrote:
> > > [...]
> > > > > There're many threads of a multi-threaded task parallel running in a
> > > > > container on many cpus. Then many threads triggered OOM at the same time,
> > > > >
> > > > > CPU-1               CPU-2         ...        CPU-n
> > > > > thread-1        thread-2      ...        thread-n
> > > > >
> > > > > wait oom_lock   wait oom_lock ...        hold oom_lock
> > > > >
> > > > >                                          (sigkill received)
> > > > >
> > > > >                                          select current as victim
> > > > >                                          and wakeup oom reaper
> > > > >
> > > > >                                          release oom_lock
> > > > >
> > > > >                                          (MMF_OOM_SKIP set by oom reaper)
> > > > >
> > > > >                                          (lots of pages are freed)
> > > > > hold oom_lock
> > > >
> > > > Could you be more specific please? The page allocator never waits for
> > > > the oom_lock and keeps retrying instead. Also __alloc_pages_may_oom
> > > > tries to allocate with the lock held.
> > >
> > > I suspect that you are looking at memcg oom killer.
> >
> > Right, these threads were waiting the oom_lock in mem_cgroup_out_of_memory().
> >
> > > Because we do not do
> > > trylock there for some reason I do not immediatelly remember from top of
> > > my head. If this is really the case then I would recommend looking into
> > > how the page allocator implements this and follow the same pattern for
> > > memcg as well.
> > >
> >
> > That is a good suggestion.
> > But we can't try locking the global oom_lock here, because task ooming
> > in memcg foo may can't help the tasks in memcg bar.
>
> I do not follow. oom_lock is not about fwd progress. It is a big lock to
> synchronize against oom_disable logic.
>
> I have this in mind
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 248e6cad0095..29d1f8c2d968 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1563,8 +1563,10 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
>         };
>         bool ret;
>
> -       if (mutex_lock_killable(&oom_lock))
> +       if (!mutex_trylock(&oom_lock))
>                 return true;

                   root_mem_cgroup
            /                                                          \
memcg_a (16G)                                             memcg_b (32G)
|                                                                        |
process a_1 (reach memcg_a limit)                process b_1(reach
memcg_b limit)
hold oom_lock                                                  wait oom_lock

So we can find that process a_1 will try to kill process in memcg_a,
while process b_1 need to try to kill process in memcg_b.
IOW, the process killed in memcg_a can't help the processes in
memcg_b, so if process b_1 should not trylock oom_lock here.

While if the memcg tree is ,
                   target mem_cgroup (16G)
            /                                                          \
            |
              |
process a_1 (reach memcg_a limit)                process a_2(reach
memcg_a limit)
hold oom_lock                                                  wait oom_lock

Then, process a_2 can trylock oom_lock here. IOW, these processes
should in the same memcg.

That's why I said that we should introduce per-memcg oom_lock.

> +
> +
>         /*
>          * A few threads which were not waiting at mutex_lock_killable() can
>          * fail to bail out. Therefore, check again after holding oom_lock.
>
> But as I've said I would need to double check the history on why we
> differ here. Btw. I suspect that mem_cgroup_out_of_memory call in
> mem_cgroup_oom_synchronize is bogus and can no longer trigger after
> 29ef680ae7c21 but this needs double checking as well.
>
> > IOW, we need to introduce the per memcg oom_lock, like bellow,
>
> I do not see why. Besides that we already do have per oom memcg
> hierarchy lock.
>




-- 
Thanks
Yafang