On 02/16/2017 10:41 AM, Andrea Arcangeli wrote:

On Wed, Feb 15, 2017 at 01:46:50PM -0800, Mike Kravetz wrote:

quoted

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d0d1d08..41f6c51 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c

@@ -4029,6 +4029,18 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	__SetPageUptodate(page);
 	set_page_huge_active(page);
 
+	/*
+	 * If shared, add to page cache
+	 */
+	if (dst_vma->vm_flags & VM_SHARED) {

Minor nitpick, this could be a:

      int vm_shared = dst_vma->vm_flags & VM_SHARED;

(int faster than bool here as VM_SHARED won't have to be converted into 0|1)

quoted

@@ -386,7 +413,8 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
 		goto out_unlock;
 
 	err = -EINVAL;
-	if (!vma_is_shmem(dst_vma) && dst_vma->vm_flags & VM_SHARED)
+	if (!vma_is_shmem(dst_vma) && !is_vm_hugetlb_page(dst_vma) &&
+	    dst_vma->vm_flags & VM_SHARED)
 		goto out_unlock;

Other minor nitpick, this could have been faster as:

     if (vma_is_anonymous(dst_vma) && dst_vma->vm_flags & VM_SHARED)

Thinking twice, the only case we need to rule out is shmem_zero_setup
(it's not anon vmas can be really VM_SHARED or they wouldn't be anon
vmas in the first place) so even the above is superfluous because
shmem_zero_setup does:

	vma->vm_ops = &shmem_vm_ops;

So I would turn it into:


     /*
      * shmem_zero_setup is invoked in mmap for MAP_ANONYMOUS|MAP_SHARED but
      * it will overwrite vm_ops, so vma_is_anonymous must return false.
      */
     if (WARN_ON_ONCE(vma_is_anonymous(dst_vma) && dst_vma->vm_flags & VM_SHARED))
 		goto out_unlock;


Reviewed-by: Andrea Arcangeli <redacted>
---

Thanks Andrea, I incorporated your suggestions into a new version of the patch. 
While changing (dst_vma->vm_flags & VM_SHARED) to integers, I noticed an issue
in the error path of __mcopy_atomic_hugetlb().

               */
-             ClearPagePrivate(page);
+             if (dst_vma->vm_flags & VM_SHARED)
+                     SetPagePrivate(page);
+             else
+                     ClearPagePrivate(page);
              put_page(page);

We can not use dst_vma here as it may be different than the vma for which the
page was originally allocated, or even NULL.  Remember, we may drop mmap_sem
and look up dst_vma again.  Therefore, we need to save the value of
(dst_vma->vm_flags & VM_SHARED) for the vma which was used when the page was
allocated.  This change as well as your suggestions are in the patch below: