On 10/13/23 21:58, Naoya Horiguchi wrote: > On Tue, Sep 05, 2023 at 02:44:00PM -0700, Mike Kravetz wrote: > > Currently, vmemmap optimization of hugetlb pages is performed before the > > hugetlb flag (previously hugetlb destructor) is set identifying it as a > > hugetlb folio. This means there is a window of time where an ordinary > > folio does not have all associated vmemmap present. The core mm only > > expects vmemmap to be potentially optimized for hugetlb and device dax. > > This can cause problems in code such as memory error handling that may > > want to write to tail struct pages. > > > > There is only one call to perform hugetlb vmemmap optimization today. > > To fix this issue, simply set the hugetlb flag before that call. > > > > There was a similar issue in the free hugetlb path that was previously > > addressed. The two routines that optimize or restore hugetlb vmemmap > > should only be passed hugetlb folios/pages. To catch any callers not > > following this rule, add VM_WARN_ON calls to the routines. In the > > hugetlb free code paths, some calls could be made to restore vmemmap > > after clearing the hugetlb flag. This was 'safe' as in these cases > > vmemmap was already present and the call was a NOOP. However, for > > consistency these calls where eliminated so that we can add the > > VM_WARN_ON checks. > > > > Fixes: f41f2ed43ca5 ("mm: hugetlb: free the vmemmap pages associated with each HugeTLB page") > > Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx> > > I saw that VM_WARN_ON_ONCE() in hugetlb_vmemmap_restore is triggered when > memory_failure() is called on a free hugetlb page with vmemmap optimization > disabled (the warning is not triggered if vmemmap optimization is enabled). > I think that we need check folio_test_hugetlb() before dissolve_free_huge_page() > calls hugetlb_vmemmap_restore_folio(). > > Could you consider adding some diff like below? Thanks! That case was indeed overlooked. Andrew, this patch is currently in mm-stable. How would you like to update? - A new version of the patch - An patch to the original patch - Something else -- Mike Kravetz > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2312,15 +2312,16 @@ int dissolve_free_huge_page(struct page *page) > * Attempt to allocate vmemmmap here so that we can take > * appropriate action on failure. > */ > - rc = hugetlb_vmemmap_restore_folio(h, folio); > - if (!rc) { > - update_and_free_hugetlb_folio(h, folio, false); > - } else { > - spin_lock_irq(&hugetlb_lock); > - add_hugetlb_folio(h, folio, false); > - h->max_huge_pages++; > - spin_unlock_irq(&hugetlb_lock); > + if (folio_test_hugetlb(folio)) { > + rc = hugetlb_vmemmap_restore_folio(h, folio); > + if (rc) { > + spin_lock_irq(&hugetlb_lock); > + add_hugetlb_folio(h, folio, false); > + h->max_huge_pages++; > + goto out; > + } > } > + update_and_free_hugetlb_folio(h, folio, false); > > return rc; > } > > > Thanks, > Naoya Horiguchi