On 9/11/18 2:16 PM, Matthew Wilcox wrote:

On Wed, Sep 12, 2018 at 04:58:11AM +0800, Yang Shi wrote:

quoted

  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