On Tue, 24 Jul 2018, Michal Hocko wrote: > oom_reap_task_mm should return false when __oom_reap_task_mm return > false. This is what my patch did but it seems this changed by > http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-oom-remove-oom_lock-from-oom_reaper.patch > so that one should be fixed. > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 104ef4a01a55..88657e018714 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -565,7 +565,7 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm) > /* failed to reap part of the address space. Try again later */ > if (!__oom_reap_task_mm(mm)) { > up_read(&mm->mmap_sem); > - return true; > + return false; > } > > pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n", > > > On top of that the proposed cleanup looks as follows: > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 88657e018714..4e185a282b3d 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -541,8 +541,16 @@ bool __oom_reap_task_mm(struct mm_struct *mm) > return ret; > } > > +/* > + * Reaps the address space of the give task. > + * > + * Returns true on success and false if none or part of the address space > + * has been reclaimed and the caller should retry later. > + */ > static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm) > { > + bool ret = true; > + > if (!down_read_trylock(&mm->mmap_sem)) { > trace_skip_task_reaping(tsk->pid); > return false; > @@ -555,28 +563,28 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm) > * down_write();up_write() cycle in exit_mmap(). > */ > if (test_bit(MMF_OOM_SKIP, &mm->flags)) { > - up_read(&mm->mmap_sem); > trace_skip_task_reaping(tsk->pid); > - return true; > + goto out_unlock; > } > > trace_start_task_reaping(tsk->pid); > > /* failed to reap part of the address space. Try again later */ > - if (!__oom_reap_task_mm(mm)) { > - up_read(&mm->mmap_sem); > - return false; > - } > + ret = __oom_reap_task_mm(mm); > + if (!ret) > + goto out_finish; > > pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n", > task_pid_nr(tsk), tsk->comm, > K(get_mm_counter(mm, MM_ANONPAGES)), > K(get_mm_counter(mm, MM_FILEPAGES)), > K(get_mm_counter(mm, MM_SHMEMPAGES))); > +out_finish: > + trace_finish_task_reaping(tsk->pid); > +out_unlock: > up_read(&mm->mmap_sem); > > - trace_finish_task_reaping(tsk->pid); > - return true; > + return ret; > } > > #define MAX_OOM_REAP_RETRIES 10 I think we still want to trace when reaping was skipped to know that the oom reaper will retry again later. mm/oom_kill.c: clean up oom_reap_task_mm() fix indicate reaping has been partially skipped so we can expect future skips or another start before finish. Signed-off-by: David Rientjes <rientjes at google.com> --- mm/oom_kill.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mm/oom_kill.c b/mm/oom_kill.c --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -569,10 +569,12 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm) trace_start_task_reaping(tsk->pid); - /* failed to reap part of the address space. Try again later */ ret = __oom_reap_task_mm(mm); - if (!ret) + if (!ret) { + /* Failed to reap part of the address space. Try again later */ + trace_skip_task_reaping(tsk->pid); goto out_finish; + } pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n", task_pid_nr(tsk), tsk->comm,
