Re: [RFC v9 PATCH 2/4] mm: mmap: zap pages with read mmap_sem in munmap

From: Yang Shi
Date: Tue Sep 11 2018 - 19:35:28 EST




On 9/11/18 2:16 PM, Matthew Wilcox wrote:
On Wed, Sep 12, 2018 at 04:58:11AM +0800, Yang Shi wrote:
mm/mmap.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
I really think you're going about this the wrong way by duplicating
vm_munmap().

If we don't duplicate vm_munmap() or do_munmap(), we need pass an extra parameter to them to tell when it is fine to downgrade write lock or if the lock has been acquired outside it (i.e. in mmap()/mremap()), right? But, vm_munmap() or do_munmap() is called not only by mmap-related, but also some other places, like arch-specific places, which don't need downgrade write lock or are not safe to do so.

Actually, I did this way in the v1 patches, but it got pushed back by tglx who suggested duplicate the code so that the change could be done in mm only without touching other files, i.e. arch-specific stuff. I didn't have strong argument to convince him.

And, Michal prefers have VM_HUGETLB and VM_PFNMAP handled separately for safe and bisectable sake, which needs call the regular do_munmap().

In addition to this, I just found mpx code may call do_munmap() recursively when I was looking into the mpx code.

We might be able to handle these by the extra parameter, but it sounds it make the code hard to understand and error prone.

Thanks,
Yang