Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping

From: Yang Shi
Date: Wed Jul 11 2018 - 12:58:15 EST



On 7/11/18 3:33 AM, Michal Hocko wrote:
On Wed 11-07-18 07:34:06, Yang Shi wrote:
Background:
Recently, when we ran some vm scalability tests on machines with large memory,
we ran into a couple of mmap_sem scalability issues when unmapping large memory
space, please refer to https://lkml.org/lkml/2017/12/14/733 and
https://lkml.org/lkml/2018/2/20/576.


History:
Then akpm suggested to unmap large mapping section by section and drop mmap_sem
at a time to mitigate it (see https://lkml.org/lkml/2018/3/6/784).

V1 patch series was submitted to the mailing list per Andrew's suggestion
(see https://lkml.org/lkml/2018/3/20/786). Then I received a lot great feedback
and suggestions.

Then this topic was discussed on LSFMM summit 2018. In the summit, Michal Hocko
suggested (also in the v1 patches review) to try "two phases" approach. Zapping
pages with read mmap_sem, then doing via cleanup with write mmap_sem (for
discussion detail, see https://lwn.net/Articles/753269/)


Approach:
Zapping pages is the most time consuming part, according to the suggestion from
Michal Hocko [1], zapping pages can be done with holding read mmap_sem, like
what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas.

But, we can't call MADV_DONTNEED directly, since there are two major drawbacks:
* The unexpected state from PF if it wins the race in the middle of munmap.
It may return zero page, instead of the content or SIGSEGV.
* Canât handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, which
is a showstopper from akpm
I do not really understand why this is a showstopper. This is a mere
optimization. VM_LOCKED ranges are usually not that large. VM_HUGETLB
can be quite large alright but this should be doable on top. Is there
any reason to block any "cover most mappings first" patch?

And, some part may need write mmap_sem, for example, vma splitting. So, the
design is as follows:
acquire write mmap_sem
lookup vmas (find and split vmas)
set VM_DEAD flags
deal with special mappings
downgrade_write

zap pages
release mmap_sem

retake mmap_sem exclusively
cleanup vmas
release mmap_sem
Please explain why dropping the lock and then ratake it to cleanup vmas
is OK. This is really important because parallel thread could have
changed the underlying address space range.

Yes, the address space could be changed after retaking the lock. Actually, here do_munmap() is called in the new patch to do the cleanup work as Kirill suggested, which will re-lookup vmas and deal with any address space change.

If there is no address space change, actually it just clean up vmas.


Moreover

include/linux/mm.h | 8 +++
include/linux/oom.h | 20 -------
mm/huge_memory.c | 4 +-
mm/hugetlb.c | 5 ++
mm/memory.c | 57 ++++++++++++++++---
mm/mmap.c | 221 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
mm/shmem.c | 9 ++-
7 files changed, 255 insertions(+), 69 deletions(-)
this is not a small change for something that could be achieved
from the userspace trivially (just call madvise before munmap - library
can hide this). Most workloads will even not care about races because
they simply do not play tricks with mmaps and userspace MM. So why do we
want to put the additional complexity into the kernel?

Note that I am _not_ saying this is a wrong idea, we just need some
pretty sounds arguments to justify the additional complexity which is
mostly based on our fear that somebody might be doing something
(half)insane or dubious at best.

I agree with Kirill that we can't rely on sane userspace to handle kernel latency issue. Moreover, we even don't know if they are sane enough or not at all.

Yang