Re: [RFC PATCH v1 1/4] mm, hwpoison, hugetlb: introduce SUBPAGE_INDEX_HWPOISON to save raw error page

[HORIGUCHI NAOYA(堀口 直也)]

On Wed, Apr 27, 2022 at 03:11:31PM +0800, Miaohe Lin wrote:
> On 2022/4/27 12:28, Naoya Horiguchi wrote:
> > From: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
> > 
> > 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 but that's all
> 
> s/so but/so/

Fixed, thank you.

> 
> > 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 page is lost.
> > 
> > Use the private field of a tail page to keep that information.  The code
> 
> Only one raw error page is saved now. Should this be ok? I think so as memory
> failure should be rare anyway?

This is a good point.  It might be rare, but maybe we need some consideration
on it. Some ideas in my mind below ...

- using struct page of all subpages is not compatible with hugetlb_free_vmemmap,
  so it's not desirable.
- defining a linked list starting from hpage[SUBPAGE_INDEX_HWPOISON].private
  might be a solution to save the multiple offsets.
- hacking bits in hpage[SUBPAGE_INDEX_HWPOISON].private field to save offset
  info in compressed format.  For example, for 2MB hugepage there could be
  512 offset numbers, so we can save one offset with 9 bits subfield.
  So we can save upto 7 offsets in the field.  This is not flexible and
  still can't handle many errors.
- maintaining global data structure to save the pfn of all hwpoison pages
  in the system. This might sound overkilling for the current purpose,
  but this data structure might be helpful for other purpose, so in the long
  run someone might get interested in it.

> 
> > path of shrinking hugepage pool used this info to try delayed dissolve.
> > 
> > Signed-off-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
> > ---
> >  include/linux/hugetlb.h | 24 ++++++++++++++++++++++++
> >  mm/hugetlb.c            |  9 +++++++++
> >  mm/memory-failure.c     |  2 ++
> >  3 files changed, 35 insertions(+)
> > 
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index ac2a1d758a80..689e69cb556b 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_CGROUP_HUGETLB
> > +	SUBPAGE_INDEX_HWPOISON,
> >  #endif
> 
> Do we rely on the CONFIG_CGROUP_HUGETLB to store the raw error page?

No. I meant CONFIG_MEMORY_FAILURE.
# I just copied and pasted the #ifdef line just above, and forget to update
# the CONFIG_* part :(

Thanks,
Naoya Horiguchi