Re: [mm-unstable PATCH v5 3/8] mm, hwpoison, hugetlb: support saving mechanism of raw error pages

["HORIGUCHI NAOYA(堀口 直也)" <naoya.horiguchi@nec.com>]

On Mon, Jul 11, 2022 at 11:26:34AM +0800, Miaohe Lin wrote:
> On 2022/7/8 13:36, Naoya Horiguchi wrote:
> > From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > 
> > When handling memory error on a hugetlb page, the error handler tries to
> > dissolve and turn it into 4kB pages.  If it's successfully dissolved,
> > PageHWPoison flag is moved to the raw error page, so that's all right.
> > However, dissolve sometimes fails, then the error page is left as
> > hwpoisoned hugepage. It's useful if we can retry to dissolve it to save
> > healthy pages, but that's not possible now because the information about
> > where the raw error pages is lost.
> > 
> > Use the private field of a few tail pages to keep that information.  The
> > code path of shrinking hugepage pool uses this info to try delayed dissolve.
> > In order to remember multiple errors in a hugepage, a singly-linked list
> > originated from SUBPAGE_INDEX_HWPOISON-th tail page is constructed.  Only
> > simple operations (adding an entry or clearing all) are required and the
> > list is assumed not to be very long, so this simple data structure should
> > be enough.
> > 
> > If we failed to save raw error info, the hwpoison hugepage has errors on
> > unknown subpage, then this new saving mechanism does not work any more,
> > so disable saving new raw error info and freeing hwpoison hugepages.
> > 
> > Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> > v4 -> v5:
> > - fixed build error (reported by kernel test robot).
> > - do not try to undo remove_hugetlb_page() when HPageRawHwpUnreliable is true,
> > - check HPageRawHwpUnreliable() before hugetlb_vmemmap_restore(),
> > - call num_poisoned_pages_inc() in hugetlb_set_page_hwpoison() when kalloc
> >   succeeds,
> > - remove "inline" in the definition of hugetlb_clear_page_hwpoison().
> > 
> > v3 -> v4:
> > - resolve conflict with "mm: hugetlb_vmemmap: improve hugetlb_vmemmap
> >   code readability", use hugetlb_vmemmap_restore() instead of
> >   hugetlb_vmemmap_alloc().
> > 
> > v2 -> v3:
> > - remove duplicate "return ret" lines,
> > - use GFP_ATOMIC instead of GFP_KERNEL,
> > - introduce HPageRawHwpUnreliable pseudo flag (suggested by Muchun),
> > - hugetlb_clear_page_hwpoison removes raw_hwp_page list even if
> >   HPageRawHwpUnreliable is true, (by Miaohe)
> > 
> > v1 -> v2:
> > - support hwpoison hugepage with multiple errors,
> > - moved the new interface functions to mm/memory-failure.c,
> > - define additional subpage index SUBPAGE_INDEX_HWPOISON_UNRELIABLE,
> > - stop freeing/dissolving hwpoison hugepages with unreliable raw error info,
> > - drop hugetlb_clear_page_hwpoison() in dissolve_free_huge_page() because
> >   that's done in update_and_free_page(),
> > - move setting/clearing PG_hwpoison flag to the new interfaces,
> > - checking already hwpoisoned or not on a subpage basis.
> > 
> > ChangeLog since previous post on 4/27:
> > - fixed typo in patch description (by Miaohe)
> > - fixed config value in #ifdef statement (by Miaohe)
> > - added sentences about "multiple hwpoison pages" scenario in patch
> >   description
> > ---
> >  include/linux/hugetlb.h | 18 +++++++++-
> >  mm/hugetlb.c            | 32 ++++++++++++-----
> >  mm/memory-failure.c     | 79 +++++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 116 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index 6d0620edf0a6..6fd128b80d57 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -42,6 +42,9 @@ enum {
> >  	SUBPAGE_INDEX_CGROUP,		/* reuse page->private */
> >  	SUBPAGE_INDEX_CGROUP_RSVD,	/* reuse page->private */
> >  	__MAX_CGROUP_SUBPAGE_INDEX = SUBPAGE_INDEX_CGROUP_RSVD,
> > +#endif
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +	SUBPAGE_INDEX_HWPOISON,
> >  #endif
> >  	__NR_USED_SUBPAGE,
> >  };
> > @@ -551,7 +554,7 @@ generic_hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> >   *	Synchronization:  Initially set after new page allocation with no
> >   *	locking.  When examined and modified during migration processing
> >   *	(isolate, migrate, putback) the hugetlb_lock is held.
> > - * HPG_temporary - - Set on a page that is temporarily allocated from the buddy
> > + * HPG_temporary - Set on a page that is temporarily allocated from the buddy
> >   *	allocator.  Typically used for migration target pages when no pages
> >   *	are available in the pool.  The hugetlb free page path will
> >   *	immediately free pages with this flag set to the buddy allocator.
> > @@ -561,6 +564,8 @@ generic_hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> >   * HPG_freed - Set when page is on the free lists.
> >   *	Synchronization: hugetlb_lock held for examination and modification.
> >   * HPG_vmemmap_optimized - Set when the vmemmap pages of the page are freed.
> > + * HPG_raw_hwp_unreliable - Set when the hugetlb page has a hwpoison sub-page
> > + *     that is not tracked by raw_hwp_page list.
> >   */
> >  enum hugetlb_page_flags {
> >  	HPG_restore_reserve = 0,
> > @@ -568,6 +573,7 @@ enum hugetlb_page_flags {
> >  	HPG_temporary,
> >  	HPG_freed,
> >  	HPG_vmemmap_optimized,
> > +	HPG_raw_hwp_unreliable,
> >  	__NR_HPAGEFLAGS,
> >  };
> >  
> > @@ -614,6 +620,7 @@ HPAGEFLAG(Migratable, migratable)
> >  HPAGEFLAG(Temporary, temporary)
> >  HPAGEFLAG(Freed, freed)
> >  HPAGEFLAG(VmemmapOptimized, vmemmap_optimized)
> > +HPAGEFLAG(RawHwpUnreliable, raw_hwp_unreliable)
> >  
> >  #ifdef CONFIG_HUGETLB_PAGE
> >  
> > @@ -796,6 +803,15 @@ extern int dissolve_free_huge_page(struct page *page);
> >  extern int dissolve_free_huge_pages(unsigned long start_pfn,
> >  				    unsigned long end_pfn);
> >  
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +extern int hugetlb_clear_page_hwpoison(struct page *hpage);
> > +#else
> > +static inline int hugetlb_clear_page_hwpoison(struct page *hpage)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  #ifdef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
> >  #ifndef arch_hugetlb_migration_supported
> >  static inline bool arch_hugetlb_migration_supported(struct hstate *h)
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 77119d93a0f9..3956494cc5fb 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1442,6 +1442,15 @@ static void __remove_hugetlb_page(struct hstate *h, struct page *page,
> >  		h->surplus_huge_pages_node[nid]--;
> >  	}
> >  
> > +	/*
> > +	 * This leaves HPageRawHwpUnreliable pages as leaked hugepages, not
> > +	 * as leaked generic-compound pages.  Otherwise page_mapped() or
> > +	 * folio_mapped() gets slow because for-loop for each subpage is
> > +	 * called.
> > +	 */
> > +	if (HPageRawHwpUnreliable(page))
> > +		return;
> > +
> 
> This patch looks good to me with below several possible problems:
> 
> Should "nr_huge_pages" and "nr_huge_pages_node" be adjusted too? If it's called from dissolve_free_huge_page
> and hugetlb_vmemmap_restore fails, add_hugetlb_page will be called:
> 
> add_hugetlb_page:
> 	...
> 	h->nr_huge_pages++;
> 	h->nr_huge_pages_node[nid]++;
> 	   ^^^^^^^^1. the "nr_huge_pages" and "nr_huge_pages_node" might be incorrect?
> 	...
> 	zeroed = put_page_testzero(page);
> 		 ^^^^^^^^2. VM_BUG_ON_PAGE(page_ref_count(page) == 0, page); will be triggered?
> 
> Or am I miss something?

No, this code breaks the specific case, so I'd like to simply drop this if.
HPageRawHwpUnreliable hugepage should be very rare, and calling page_mapped()
for such a leaked page should be less common, so the impact of the slowdown
should be minimal.

Thanks,
Naoya Horiguchi