On 09/19, Linus Torvalds wrote:

On Sat, Sep 19, 2015 at 8:03 AM, Oleg Nesterov [off-list ref] wrote:

quoted

+
+static void oom_unmap_func(struct work_struct *work)
+{
+       struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
+
+       if (!atomic_inc_not_zero(&mm->mm_users))
+               return;
+
+       // If this is not safe we can do use_mm() + unuse_mm()
+       down_read(&mm->mmap_sem);

I don't think this is safe.

What makes you sure that we might not deadlock on the mmap_sem here?
For all we know, the process that is going out of memory is in the
middle of a mmap(), and already holds the mmap_sem for writing. No?

In this case the workqueue thread will block. But it can not block
forever. I mean if it can then the killed process will never exit
(exit_mm does down_read) and release its memory, so we lose anyway.

But let me repeat this patch is obviously not complete/etc,

So at the very least that needs to be a trylock, I think.

And we want to avoid using workqueues when the caller can do this
directly. And in this case we certainly need trylock. But this needs
some refactoring: we do not want to do this under oom_lock, otoh it
makes sense to do this from mark_oom_victim() if current && killed,
and a lot more details.

The workqueue thread has other reasons for trylock, but probably not
in the initial version of this patch. And perhaps we should use a
dedicated kthread and do not use workqueues at all. And yes, a single
"mm_struct *oom_unmap_mm" is ugly, it should be the list of mm's to
unmap, but then at least we need MMF_MEMDIE.

And I'm not
sure zap_page_range() is ok with the mmap_sem only held for reading.
Normally our rule is that you can *populate* the page tables
concurrently, but you can't tear the down.

Well, according to madvise_need_mmap_write() MADV_DONTNEED does this
under down_read().

But yes, yes, this is probably not right anyway. Say, VM_LOCKED...
That is why I mentioned that perhaps this should only unmap the
anonymous pages. We can probably add zap_details->for_oom hint.



Another question if it is safe to abuse the foreign mm this way.
Well, zap_page_range_single() does this, so this is probably safe.
But we can do use_mm().

Oleg.

--
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>