Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 2 Sep 2022 16:07:55 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     Muchun Song <songmuchun@bytedance.com>,
        David Hildenbrand <david@redhat.com>,
        Michal Hocko <mhocko@suse.com>, Peter Xu <peterx@redhat.com>,
        Naoya Horiguchi <naoya.horiguchi@linux.dev>,
        "Aneesh Kumar K . V" <aneesh.kumar@linux.vnet.ibm.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Prakash Sangappa <prakash.sangappa@oracle.com>,
        James Houghton <jthoughton@google.com>,
        Mina Almasry <almasrymina@google.com>,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Ray Fucillo <Ray.Fucillo@intersystems.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 8/8] hugetlb: use new vma_lock for pmd sharing
 synchronization
Message-ID: <YxKMy3sDsWPEOMMJ@monkey>
References: <20220824175757.20590-1-mike.kravetz@oracle.com>
 <20220824175757.20590-9-mike.kravetz@oracle.com>
 <08edc08e-08ab-0706-3c8d-804080f37bd7@huawei.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <08edc08e-08ab-0706-3c8d-804080f37bd7@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tM+HiRIv8wn7CKO1FWzqUbiyiaNvKID/I+X1VsxNr/VFrbHXWTQOji3RecCb?=
 =?us-ascii?Q?7u27xe2qSaDf7jSLRLkwMEGfNF1AJH7YpXfrPHK2xpMaK0nPMJyG8UL0GTKN?=
 =?us-ascii?Q?VfXduGLQx7UJ91ciPnYWnMbtFkFPTVHcbj2eRwEFmpIl0k/uwGi15lDA67CM?=
 =?us-ascii?Q?lgs1VsW59dLsPHbpmApw6QHh3HOq7DKo2kmKkq43TaiNxfEiLB44eFNput/I?=
 =?us-ascii?Q?5wuupTpZJu/CWGpcwNoL4NGMKRktpVyuS8MGVXj5PeYDRBtTwOnjFBZ38+Ks?=
 =?us-ascii?Q?2HZpKxVBZzSZ6ho+/LxZHgQSbmDhPPU47PNIilKFzrN8QmePxyyKMw6KR0PV?=
 =?us-ascii?Q?D/d2iJU7VagNSkUqvndrk8i9oykywhoQ9t35bkbTPKgaq4ctYFKPi9nXQVwP?=
 =?us-ascii?Q?7JBlknGxqAwHJydogMUlB8u0G6F0EgJjNb/64jMz0/j18hIX0m0qCq3YsPKR?=
 =?us-ascii?Q?zZ+jW20AOTgwD51IPfhcYBhvJAuN+8ZFFnWRbEJmxNy09oIENK8oIKTeDIYC?=
 =?us-ascii?Q?s/S7MLVIbBH8g11T1JplXQ3t0YAJMdBL2rxpkOWUPzevZy0WtehDXL01p0iU?=
 =?us-ascii?Q?VGnWus32wXsMm3We5Dhbv8cwV9dVoxNyIAFXYligydHsYsT63PuvxZYgQl0r?=
 =?us-ascii?Q?JZnToYxOQDWzIPdreS9PJ2tK0U8fUKD0Z3+zIPZ4hkUFYb9a5M0QvFc2X2MR?=
 =?us-ascii?Q?09d8RlWNIkoJnAX7tN0WgHiGflQqLkqjGKQeqIzSIoBSXMUM130UPCP3SF2+?=
 =?us-ascii?Q?dHnC1lhkWNqzXT7Ptc6FBpwEdOZvl9I//+WW6HdXB9o1w4yaPjwLUBTzi1sX?=
 =?us-ascii?Q?n+mci7vs2fv+GmA54/VedOexFcgM1qqotAmH/+q/p5UNmWAS9bw8DE/TeUZU?=
 =?us-ascii?Q?AIBUsaRrfKIRXvhZOjvZI3LTKcKF1yzhZHocQHVrUQl4K0mk+09TI0DcWS4j?=
 =?us-ascii?Q?xkk7Bb7Q3+9RhpPHVjRSLtD1cQquolHS6QDxcP88aLFmYmhuO6CO2bVh2swB?=
 =?us-ascii?Q?MWR2hLrA7ivQz2wz981a60yQI+pX3hwlOoJP3S/pqVTZldJu1YDW8WcW3B3n?=
 =?us-ascii?Q?In1jt1AAAhAFSYzluK8nd7j532shncSQqYxCgJvFPooepoNR47nhPAwNiXuU?=
 =?us-ascii?Q?tsQBAxefjpSZqmc+nesoIPpSnFNBPaXmN7MzX1dugOoVloVrvCFHPJD5Le39?=
 =?us-ascii?Q?5ZmII/PY7XPF1xEvSGOUqP+bm7fxeQav991mkcrWABzNOpm2YN3pMb4egVmv?=
 =?us-ascii?Q?/ZvZCMlW2gt5hbG/eksSom0LkHiv8dviinMANPoucA0FzJbNd5xVz0Tb1HBX?=
 =?us-ascii?Q?VJY9CUP/lOS0s2YDKI7Zuc+XPPKnFrIM2z9PbMeNI+ffPvWxVsr/8q2Dz+yH?=
 =?us-ascii?Q?m6Gg9qT7TvJSUsl8eZvisiOxcWGwjq7ao04+AKHnfs+sgNt5mBC/hg6Pv7b9?=
 =?us-ascii?Q?MaLoE3Qr/kBaf70gr39vTsjSLsew+S0rproO9MaD+UePBuR0qg84e1IOz0bk?=
 =?us-ascii?Q?OMaK0nYdMC3sSAt9kPy6ZXd8etj2NAsBIUoUIxfcE9JQOS9ZhaFun7GLG4JB?=
 =?us-ascii?Q?my0ky36CmmVEEHAQdDUBqKm76TNCQeMh69l069MNd5N0iJ5kQZ0xadff4p3z?=
 =?us-ascii?Q?NA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 949d7ada-241b-4524-15e8-08da8d37fa20
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Sep 2022 23:07:58.7189
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: L2vna1zyM2V7Puf5I/O0RlHxAIpR1ipg2D3N5gjWp7XAx5YleszhKpQ2zAbWvX9sC1+7Biw/42UNK5Fyo3oMtg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB6173
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-09-02_06,2022-08-31_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0 spamscore=0
 adultscore=0 mlxscore=0 mlxlogscore=999 suspectscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000
 definitions=main-2209020103
X-Proofpoint-ORIG-GUID: OcoXJHtujBBj5dsVIjBvrURQKoWCBIlV
X-Proofpoint-GUID: OcoXJHtujBBj5dsVIjBvrURQKoWCBIlV
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/30/22 10:02, Miaohe Lin wrote:
> On 2022/8/25 1:57, Mike Kravetz wrote:
> > The new hugetlb vma lock (rw semaphore) is used to address this race:
> > 
> > Faulting thread                                 Unsharing thread
> > ...                                                  ...
> > ptep = huge_pte_offset()
> >       or
> > ptep = huge_pte_alloc()
> > ...
> >                                                 i_mmap_lock_write
> >                                                 lock page table
> > ptep invalid   <------------------------        huge_pmd_unshare()
> > Could be in a previously                        unlock_page_table
> > sharing process or worse                        i_mmap_unlock_write
> > ...
> > 
> > The vma_lock is used as follows:
> > - During fault processing. the lock is acquired in read mode before
> >   doing a page table lock and allocation (huge_pte_alloc).  The lock is
> >   held until code is finished with the page table entry (ptep).
> > - The lock must be held in write mode whenever huge_pmd_unshare is
> >   called.
> > 
> > Lock ordering issues come into play when unmapping a page from all
> > vmas mapping the page.  The i_mmap_rwsem must be held to search for the
> > vmas, and the vma lock must be held before calling unmap which will
> > call huge_pmd_unshare.  This is done today in:
> > - try_to_migrate_one and try_to_unmap_ for page migration and memory
> >   error handling.  In these routines we 'try' to obtain the vma lock and
> >   fail to unmap if unsuccessful.  Calling routines already deal with the
> >   failure of unmapping.
> > - hugetlb_vmdelete_list for truncation and hole punch.  This routine
> >   also tries to acquire the vma lock.  If it fails, it skips the
> >   unmapping.  However, we can not have file truncation or hole punch
> >   fail because of contention.  After hugetlb_vmdelete_list, truncation
> >   and hole punch call remove_inode_hugepages.  remove_inode_hugepages
> >   check for mapped pages and call hugetlb_unmap_file_page to unmap them.
> >   hugetlb_unmap_file_page is designed to drop locks and reacquire in the
> >   correct order to guarantee unmap success.
> > 
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> > ---
> >  fs/hugetlbfs/inode.c |  46 +++++++++++++++++++
> >  mm/hugetlb.c         | 102 +++++++++++++++++++++++++++++++++++++++----
> >  mm/memory.c          |   2 +
> >  mm/rmap.c            | 100 +++++++++++++++++++++++++++---------------
> >  mm/userfaultfd.c     |   9 +++-
> >  5 files changed, 214 insertions(+), 45 deletions(-)
> > 
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index b93d131b0cb5..52d9b390389b 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -434,6 +434,8 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
> >  					struct folio *folio, pgoff_t index)
> >  {
> >  	struct rb_root_cached *root = &mapping->i_mmap;
> > +	unsigned long skipped_vm_start;
> > +	struct mm_struct *skipped_mm;
> >  	struct page *page = &folio->page;
> >  	struct vm_area_struct *vma;
> >  	unsigned long v_start;
> > @@ -444,6 +446,8 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
> >  	end = ((index + 1) * pages_per_huge_page(h));
> >  
> >  	i_mmap_lock_write(mapping);
> > +retry:
> > +	skipped_mm = NULL;
> >  
> >  	vma_interval_tree_foreach(vma, root, start, end - 1) {
> >  		v_start = vma_offset_start(vma, start);
> > @@ -452,11 +456,49 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
> >  		if (!hugetlb_vma_maps_page(vma, vma->vm_start + v_start, page))
> >  			continue;
> >  
> > +		if (!hugetlb_vma_trylock_write(vma)) {
> > +			/*
> > +			 * If we can not get vma lock, we need to drop
> > +			 * immap_sema and take locks in order.
> > +			 */
> > +			skipped_vm_start = vma->vm_start;
> > +			skipped_mm = vma->vm_mm;
> > +			/* grab mm-struct as we will be dropping i_mmap_sema */
> > +			mmgrab(skipped_mm);
> > +			break;
> > +		}
> > +
> >  		unmap_hugepage_range(vma, vma->vm_start + v_start, v_end,
> >  				NULL, ZAP_FLAG_DROP_MARKER);
> > +		hugetlb_vma_unlock_write(vma);
> >  	}
> >  
> >  	i_mmap_unlock_write(mapping);
> > +
> > +	if (skipped_mm) {
> > +		mmap_read_lock(skipped_mm);
> > +		vma = find_vma(skipped_mm, skipped_vm_start);
> > +		if (!vma || !is_vm_hugetlb_page(vma) ||
> > +					vma->vm_file->f_mapping != mapping ||
> > +					vma->vm_start != skipped_vm_start) {
> 
> i_mmap_lock_write(mapping) is missing here? Retry logic will do i_mmap_unlock_write(mapping) anyway.
> 

Yes, that is missing.  I will add here.

> > +			mmap_read_unlock(skipped_mm);
> > +			mmdrop(skipped_mm);
> > +			goto retry;
> > +		}
> > +
> 
> IMHO, above check is not enough. Think about the below scene:
> 
> CPU 1					CPU 2
> hugetlb_unmap_file_folio		exit_mmap
>   mmap_read_lock(skipped_mm);		  mmap_read_lock(mm);
>   check vma is wanted.
>   					  unmap_vmas
>   mmap_read_unlock(skipped_mm);		  mmap_read_unlock
>   					  mmap_write_lock(mm);
>   					  free_pgtables
>   					  remove_vma
> 					    hugetlb_vma_lock_free
>   vma, hugetlb_vma_lock is still *used after free*
>   					  mmap_write_unlock(mm);
> So we should check mm->mm_users == 0 to fix the above issue. Or am I miss something?

In the retry case, we are OK because go back and look up the vma again.  Right?

After taking mmap_read_lock, vma can not go away until we mmap_read_unlock.
Before that, we do the following:

> > +		hugetlb_vma_lock_write(vma);
> > +		i_mmap_lock_write(mapping);

IIUC, vma can not go away while we hold i_mmap_lock_write.  So, after this we
can.

> > +		mmap_read_unlock(skipped_mm);
> > +		mmdrop(skipped_mm);

We continue to hold i_mmap_lock_write as we goto retry.

I could be missing something as well.  This was how I intended to keep
vma valid while dropping and acquiring locks.

> > +
> > +		v_start = vma_offset_start(vma, start);
> > +		v_end = vma_offset_end(vma, end);
> > +		unmap_hugepage_range(vma, vma->vm_start + v_start, v_end,
> > +				NULL, ZAP_FLAG_DROP_MARKER);
> > +		hugetlb_vma_unlock_write(vma);
> > +
> > +		goto retry;
> 
> Should here be one cond_resched() here in case this function will take a really long time?
> 

I think we will at most retry once.

> > +	}
> >  }
> >  
> >  static void
> > @@ -474,11 +516,15 @@ hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end,
> >  		unsigned long v_start;
> >  		unsigned long v_end;
> >  
> > +		if (!hugetlb_vma_trylock_write(vma))
> > +			continue;
> > +
> >  		v_start = vma_offset_start(vma, start);
> >  		v_end = vma_offset_end(vma, end);
> >  
> >  		unmap_hugepage_range(vma, vma->vm_start + v_start, v_end,
> >  				     NULL, zap_flags);
> > +		hugetlb_vma_unlock_write(vma);
> >  	}
> 
> unmap_hugepage_range is not called under hugetlb_vma_lock in unmap_ref_private since it's private vma?
> Add a comment to avoid future confusion?
> 
> >  }

Sure, will add a comment before hugetlb_vma_lock.

> >  
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 6fb0bff2c7ee..5912c2b97ddf 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -4801,6 +4801,14 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
> >  		mmu_notifier_invalidate_range_start(&range);
> >  		mmap_assert_write_locked(src);
> >  		raw_write_seqcount_begin(&src->write_protect_seq);
> > +	} else {
> > +		/*
> > +		 * For shared mappings the vma lock must be held before
> > +		 * calling huge_pte_offset in the src vma. Otherwise, the
> 
> s/huge_pte_offset/huge_pte_alloc/, i.e. huge_pte_alloc could return shared pmd, not huge_pte_offset which
> might lead to confusion. But this is really trivial...

Actually, it is huge_pte_offset.  While looking up ptes in the source vma, we
do not want to race with other threads in the source process which could
be doing a huge_pmd_unshare.  Otherwise, the returned pte could be invalid.

FYI - Most of this code is now 'dead' because of bcd51a3c679d "Lazy page table
copies in fork()".  We will not copy shared mappigns at fork time.

> 
> Except from above comments, this patch looks good to me.
> 

Thank you! Thank you! Thank you!  For looking at this series and all
your comments.  I hope to send out v2 next week.
-- 
Mike Kravetz