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

[Miaohe Lin <linmiaohe@huawei.com>]

On 2022/4/27 21:03, HORIGUCHI NAOYA(堀口 直也) wrote:
> 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@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 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.

Yes, that is not compatible with hugetlb_free_vmemmap.

> - 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.

If there are many errors inside one hugetlb page, there might be other serious problems.

> - 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.

The above ideas sound good. And last one might be more helpful in the memory re-online
case: We can restore the hwpoison info using this global data structure. :)

Thanks!

> 
>>
>>> path of shrinking hugepage pool used this info to try delayed dissolve.
>>>
>>> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
>>> ---
>>>  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
>