Re: [patch v2] mm, oom: fix concurrent munlock and oom reaper unmap

[Michal Hocko <mhocko@kernel.org>]

On Wed 18-04-18 22:25:54, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > > Can we try a simpler way and get back to what I was suggesting before
> > > > [1] and simply not play tricks with
> > > > 		down_write(&mm->mmap_sem);
> > > > 		up_write(&mm->mmap_sem);
> > > > 
> > > > and use the write lock in exit_mmap for oom_victims?
> > > 
> > > You mean something like this?
> > 
> > or simply hold the write lock until we unmap and free page tables.
> 
> That increases possibility of __oom_reap_task_mm() giving up reclaim and
> setting MMF_OOM_SKIP when exit_mmap() is making forward progress, doesn't it?

Yes it does. But it is not that likely and easily noticeable from the
logs so we can make the locking protocol more complex if this really
hits two often.

> I think that it is better that __oom_reap_task_mm() does not give up when
> exit_mmap() can make progress. In that aspect, the section protected by
> mmap_sem held for write should be as short as possible.

Sure, but then weight the complexity on the other side and try to think
whether simpler code which works most of the time is better than a buggy
complex one. The current protocol has 2 followup fixes which speaks for
itself.
 
[...]
> > > Then, I'm tempted to call __oom_reap_task_mm() before holding mmap_sem for write.
> > > It would be OK to call __oom_reap_task_mm() at the beginning of __mmput()...
> > 
> > I am not sure I understand.
> 
> To reduce possibility of __oom_reap_task_mm() giving up reclaim and
> setting MMF_OOM_SKIP.

Still do not understand. Do you want to call __oom_reap_task_mm from
__mmput? If yes why would you do so when exit_mmap does a stronger
version of it?

-- 
Michal Hocko
SUSE Labs