On Wed, Aug 16, 2023 at 04:14:28PM -0700, Mike Kravetz wrote: > commit 32c877191e022b55fe3a374f3d7e9fb5741c514d upstream. > > Patch series "Fix hugetlb free path race with memory errors". > > In the discussion of Jiaqi Yan's series "Improve hugetlbfs read on > HWPOISON hugepages" the race window was discovered. > https://lore.kernel.org/linux-mm/20230616233447.GB7371@monkey/ > > Freeing a hugetlb page back to low level memory allocators is performed > in two steps. > 1) Under hugetlb lock, remove page from hugetlb lists and clear destructor > 2) Outside lock, allocate vmemmap if necessary and call low level free > Between these two steps, the hugetlb page will appear as a normal > compound page. However, vmemmap for tail pages could be missing. > If a memory error occurs at this time, we could try to update page > flags non-existant page structs. > > A much more detailed description is in the first patch. > > The first patch addresses the race window. However, it adds a > hugetlb_lock lock/unlock cycle to every vmemmap optimized hugetlb page > free operation. This could lead to slowdowns if one is freeing a large > number of hugetlb pages. > > The second path optimizes the update_and_free_pages_bulk routine to only > take the lock once in bulk operations. > > The second patch is technically not a bug fix, but includes a Fixes tag > and Cc stable to avoid a performance regression. It can be combined with > the first, but was done separately make reviewing easier. > > This patch (of 2): > > Freeing a hugetlb page and releasing base pages back to the underlying > allocator such as buddy or cma is performed in two steps: > - remove_hugetlb_folio() is called to remove the folio from hugetlb > lists, get a ref on the page and remove hugetlb destructor. This > all must be done under the hugetlb lock. After this call, the page > can be treated as a normal compound page or a collection of base > size pages. > - update_and_free_hugetlb_folio() is called to allocate vmemmap if > needed and the free routine of the underlying allocator is called > on the resulting page. We can not hold the hugetlb lock here. > > One issue with this scheme is that a memory error could occur between > these two steps. In this case, the memory error handling code treats > the old hugetlb page as a normal compound page or collection of base > pages. It will then try to SetPageHWPoison(page) on the page with an > error. If the page with error is a tail page without vmemmap, a write > error will occur when trying to set the flag. > > Address this issue by modifying remove_hugetlb_folio() and > update_and_free_hugetlb_folio() such that the hugetlb destructor is not > cleared until after allocating vmemmap. Since clearing the destructor > requires holding the hugetlb lock, the clearing is done in > remove_hugetlb_folio() if the vmemmap is present. This saves a > lock/unlock cycle. Otherwise, destructor is cleared in > update_and_free_hugetlb_folio() after allocating vmemmap. > > Note that this will leave hugetlb pages in a state where they are marked > free (by hugetlb specific page flag) and have a ref count. This is not > a normal state. The only code that would notice is the memory error > code, and it is set up to retry in such a case. > > A subsequent patch will create a routine to do bulk processing of > vmemmap allocation. This will eliminate a lock/unlock cycle for each > hugetlb page in the case where we are freeing a large number of pages. > > Link: https://lkml.kernel.org/r/20230711220942.43706-1-mike.kravetz@xxxxxxxxxx > Link: https://lkml.kernel.org/r/20230711220942.43706-2-mike.kravetz@xxxxxxxxxx > Fixes: ad2fa3717b74 ("mm: hugetlb: alloc the vmemmap pages associated with each HugeTLB page") > Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx> > Reviewed-by: Muchun Song <songmuchun@xxxxxxxxxxxxx> > Tested-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx> > Cc: Axel Rasmussen <axelrasmussen@xxxxxxxxxx> > Cc: James Houghton <jthoughton@xxxxxxxxxx> > Cc: Jiaqi Yan <jiaqiyan@xxxxxxxxxx> > Cc: Miaohe Lin <linmiaohe@xxxxxxxxxx> > Cc: Michal Hocko <mhocko@xxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > > Ported upstream commit 32c877191e02 as it depends on folio based > interfaces not present in v6.1.y. Now queued up, thanks. greg k-h
