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

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

On Sat, Jul 11, 2020 at 11:18 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> If the current's MMF_OOM_SKIP is set, it means that the current is exiting
> or dying and likely to realease its address space. So we don't need to
> invoke the oom killer again. Otherwise that may cause some unexpected
> issues, for example, bellow is the issue found in our production
> environment.
>
> 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
>
> because MMF_OOM_SKIP
> is set, kill others
>
> The thread running on CPU-n received sigkill and it will select current as
> the victim and wakeup the oom reaper. Then oom reaper will reap its rss and
> free lots of pages, as a result, there will be many free pages.
> Although the multi-threaded task is exiting, the other threads will
> continue to kill others because of the check of MMF_OOM_SKIP in
> task_will_free_mem().
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  mm/oom_kill.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 6e94962..a8a155a 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -825,13 +825,6 @@ static bool task_will_free_mem(struct task_struct *task)
>         if (!__task_will_free_mem(task))
>                 return false;
>
> -       /*
> -        * This task has already been drained by the oom reaper so there are
> -        * only small chances it will free some more
> -        */
> -       if (test_bit(MMF_OOM_SKIP, &mm->flags))
> -               return false;
> -
>         if (atomic_read(&mm->mm_users) <= 1)
>                 return true;
>
> @@ -963,7 +956,8 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>          * so it can die quickly
>          */
>         task_lock(victim);
> -       if (task_will_free_mem(victim)) {
> +       if (!test_bit(MMF_OOM_SKIP, &victim->mm->flags) &&
> +           task_will_free_mem(victim)) {
>                 mark_oom_victim(victim);
>                 wake_oom_reaper(victim);
>                 task_unlock(victim);
> @@ -1056,6 +1050,10 @@ bool out_of_memory(struct oom_control *oc)
>                         return true;
>         }
>
> +       /* current has been already reapered */
> +       if (test_bit(MMF_OOM_SKIP, &current->mm->flags))
> +               return true;
> +

Oops.

Should check whether mm is NULL first:
if (mm && test_bit(MMF_OOM_SKIP, mm->flags))


>         /*
>          * If current has a pending SIGKILL or is exiting, then automatically
>          * select it.  The goal is to allow it to allocate so that it may
> --
> 1.8.3.1
>


-- 
Thanks
Yafang