Re: [PATCH] mm, oom_reaper: Move oom_lock from __oom_reap_task_mm()to oom_reap_task().

[Michal Hocko <mhocko@suse.com>]

On Thu 22-12-16 19:41:41, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Mon 12-12-16 12:59:18, Michal Hocko wrote:
> > > On Mon 12-12-16 19:55:52, Tetsuo Handa wrote:
> > > > Since commit 862e3073b3eed13f
> > > > ("mm, oom: get rid of signal_struct::oom_victims")
> > > > changed to wait until MMF_OOM_SKIP is set rather than wait while
> > > > TIF_MEMDIE is set, rationale comment for commit e2fe14564d3316d1
> > > > ("oom_reaper: close race with exiting task") needs to be updated.
> > > 
> > > True.
> > > 
> > > > While holding oom_lock can make sure that other threads waiting for
> > > > oom_lock at __alloc_pages_may_oom() are given a chance to call
> > > > get_page_from_freelist() after the OOM reaper called unmap_page_range()
> > > > via __oom_reap_task_mm(), it can defer calling of __oom_reap_task_mm().
> > > > 
> > > > Therefore, this patch moves oom_lock from __oom_reap_task_mm() to
> > > > oom_reap_task() (without any functional change). By doing so, the OOM
> > > > killer can call __oom_reap_task_mm() if we don't want to defer calling
> > > > of __oom_reap_task_mm() (e.g. when oom_evaluate_task() aborted by
> > > > finding existing OOM victim's mm without MMF_OOM_SKIP).
> > > 
> > > But I fail to understand this part of the changelog. It sounds like a
> > > preparatory for other changes. There doesn't seem to be any other user
> > > of __oom_reap_task_mm in the current tree.
> 
> I'm planning to call __oom_reap_task_mm() from out_of_memory() if OOM
> situation is not solved immediately, after we made sure that we give
> enough CPU time to OOM killer and OOM reaper to run reclaim code by
> mutex_lock_killable(&oom_lock) change.

Whatever you plan to do, my objection to the patch was that it mixes two
things together. The comment removal makes sense on its own and it
should be a separate patch which I've acked already.

> > > Please send a patch which removes the comment which is no longer true
> > > on its own and feel free to add
> > > 
> > > Acked-by: Michal Hocko <mhocko@suse.com>
> > > 
> > > but do not make other changes if you do not have any follow up patch
> > > which would benefit from that.
> > 
> > Do you plan to pursue this?
> 
> Although I don't know whether we agree with mutex_lock_killable(&oom_lock)
> change, I think this patch alone can go as a cleanup.

No, we don't agree on that part. As this is a printk issue I do not want
to workaround it in the oom related code. That is just ridiculous. The
very same issue would be possible due to other continous source of log
messages.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>