Re: [PATCH] mm/memory-failure.c: bail out early if huge zero page

[Miaohe Lin <linmiaohe@huawei.com>]

On 2022/4/12 16:31, Oscar Salvador wrote:
> On Sun, Apr 10, 2022 at 11:22:34PM +0800, Xu Yu wrote:
>> Kernel panic when injecting memory_failure for the global huge_zero_page,
>> when CONFIG_DEBUG_VM is enabled, as follows.
> ... 
>> In fact, huge_zero_page is unhandlable currently in either soft offline
>> or memory failure injection.  With CONFIG_DEBUG_VM disabled,
>> huge_zero_page is bailed out when checking HWPoisonHandlable() in
>> get_any_page(), or checking page mapping in split_huge_page_to_list().
>>
>> This makes huge_zero_page bail out early in madvise_inject_error(), and
>> panic above won't happen again.
> 
> I would not special case this in madvise_inject_error() but rather
> handle it in memory-failure code.
> We do already have HWPoisonHandlable(), which tells us whether the page
> is of a type we can really do something about, so why not add another
> check in HWPoisonHandlable() for huge_zero_page(), and have that checked
> in memory_failure().

IIUC, this does not work. Because HWPoisonHandlable is only called in !MF_COUNT_INCREASED case.
But MF_COUNT_INCREASED is always set when called from madvise_inject_error, so HWPoisonHandlable
is not even called in this scene. Or am I miss something?

BTW: IIRC, LRU isn't set on huge_zero_page. So the origin HWPoisonHandlable can already filter out this page.

Thanks!

> Something like (untested):
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index dcb6bb9cf731..dccd0503f803 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1181,6 +1181,10 @@ static inline bool HWPoisonHandlable(struct page *page, unsigned long flags)
>  {
>  	bool movable = false;
>  
> +	/* Can't handle huge_zero_page() */
> +	if(is_huge_zero_page(compound_head(page)))
> +		return false;
> +
>  	/* Soft offline could mirgate non-LRU movable pages */
>  	if ((flags & MF_SOFT_OFFLINE) && __PageMovable(page))
>  		movable = true;
> @@ -1796,6 +1800,10 @@ int memory_failure(unsigned long pfn, int flags)
>  			res = -EBUSY;
>  			goto unlock_mutex;
>  		}
> +	} else if(!HWPoisonHandable(p)) {
> +		action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED);
> +		res = -EBUSY;
> +		goto unlock_mutex;
>  	}
>  
>  	if (PageTransHuge(hpage)) {
> 
> It can certainly be prettier, but you can get the idea.
> 
> 
>>
>> Reported-by: Abaci <abaci@linux.alibaba.com>
>> Signed-off-by: Xu Yu <xuyu@linux.alibaba.com>
>> ---
>>  mm/madvise.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 1873616a37d2..03ad50d222e0 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -1079,7 +1079,7 @@ static int madvise_inject_error(int behavior,
>>  
>>  	for (; start < end; start += size) {
>>  		unsigned long pfn;
>> -		struct page *page;
>> +		struct page *page, *head;
>>  		int ret;
>>  
>>  		ret = get_user_pages_fast(start, 1, 0, &page);
>> @@ -1087,12 +1087,21 @@ static int madvise_inject_error(int behavior,
>>  			return ret;
>>  		pfn = page_to_pfn(page);
>>  
>> +		head = compound_head(page);
>> +		if (unlikely(is_huge_zero_page(head))) {
>> +			pr_warn("Unhandlable attempt to %s pfn %#lx at process virtual address %#lx\n",
>> +				behavior == MADV_SOFT_OFFLINE ? "soft offline" :
>> +								"inject memory failure for",
>> +				pfn, start);
>> +			return -EINVAL;
>> +		}
>> +
>>  		/*
>>  		 * When soft offlining hugepages, after migrating the page
>>  		 * we dissolve it, therefore in the second loop "page" will
>>  		 * no longer be a compound page.
>>  		 */
>> -		size = page_size(compound_head(page));
>> +		size = page_size(head);
>>  
>>  		if (behavior == MADV_SOFT_OFFLINE) {
>>  			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
>> -- 
>> 2.20.1.2432.ga663e714
>>
>>
>