Re: [PATCH v3 3/5] mm/memory-failure: improve memory failure action_result messages

[Miaohe Lin]

On 2024/5/22 7:54, Jane Chu wrote:
> Added two explicit MF_MSG messages describing failure in get_hwpoison_page.
> Attemped to document the definition of various action names, and made a few
> adjustment to the action_result() calls.
> 
> Signed-off-by: Jane Chu <jane.chu@xxxxxxxxxx>

Thanks for your patch. This really improves the code.

> ---
>  include/linux/mm.h      |  2 ++
>  include/ras/ras_event.h |  2 ++
>  mm/memory-failure.c     | 38 +++++++++++++++++++++++++++++++++-----
>  3 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 9849dfda44d4..b4598c6a393a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4111,6 +4111,7 @@ enum mf_action_page_type {
>  	MF_MSG_DIFFERENT_COMPOUND,
>  	MF_MSG_HUGE,
>  	MF_MSG_FREE_HUGE,
> +	MF_MSG_GET_HWPOISON,
>  	MF_MSG_UNMAP_FAILED,
>  	MF_MSG_DIRTY_SWAPCACHE,
>  	MF_MSG_CLEAN_SWAPCACHE,
> @@ -4124,6 +4125,7 @@ enum mf_action_page_type {
>  	MF_MSG_BUDDY,
>  	MF_MSG_DAX,
>  	MF_MSG_UNSPLIT_THP,
> +	MF_MSG_ALREADY_POISONED,
>  	MF_MSG_UNKNOWN,
>  };
>  
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index c011ea236e9b..b3f6832a94fe 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -360,6 +360,7 @@ TRACE_EVENT(aer_event,
>  	EM ( MF_MSG_DIFFERENT_COMPOUND, "different compound page after locking" ) \
>  	EM ( MF_MSG_HUGE, "huge page" )					\
>  	EM ( MF_MSG_FREE_HUGE, "free huge page" )			\
> +	EM ( MF_MSG_GET_HWPOISON, "get hwpoison page" )			\
>  	EM ( MF_MSG_UNMAP_FAILED, "unmapping failed page" )		\
>  	EM ( MF_MSG_DIRTY_SWAPCACHE, "dirty swapcache page" )		\
>  	EM ( MF_MSG_CLEAN_SWAPCACHE, "clean swapcache page" )		\
> @@ -373,6 +374,7 @@ TRACE_EVENT(aer_event,
>  	EM ( MF_MSG_BUDDY, "free buddy page" )				\
>  	EM ( MF_MSG_DAX, "dax page" )					\
>  	EM ( MF_MSG_UNSPLIT_THP, "unsplit thp" )			\
> +	EM ( MF_MSG_ALREADY_POISONED, "already poisoned" )		\
>  	EMe ( MF_MSG_UNKNOWN, "unknown page" )
>  
>  /*
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 739311e121af..1e22d73c9329 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -879,6 +879,28 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn,
>  	return ret > 0 ? -EHWPOISON : -EFAULT;
>  }
>  
> +/*
> + * MF_IGNORED - The m-f() handler marks the page as PG_hwpoisoned'ed.
> + * But it could not do more to isolate the page from being accessed again,
> + * nor does it kill the process. This is extremely rare and one of the
> + * potential causes is that the page state has been changed due to
> + * underlying race condition. This is the most severe outcomes.
> + *
> + * MF_FAILED - The m-f() handler marks the page as PG_hwpoisoned'ed. It
> + * should have killed the process, but it can't isolate the page, due to
> + * conditions such as extra pin, unmap failure, etc. Accessing the page
> + * again will trigger another MCE and the process will be killed by the
> + * m-f() handler immediately.
> + *
> + * MF_DELAYED - The m-f() handler marks the page as PG_hwpoisoned'ed. The
> + * page is unmapped, but perhaps remains in LRU or file mapping. An attempt

Would the page remain in LRU or file mapping? IIUC, MF_DELAYED is returned from two functions:
1. me_swapcache_dirty. Page lives in swap cache and removed from LRU.
2. kvm_gmem_error_folio. Page range is unmapped. It seems page won't be in the LRU or page cache.
Or am I miss something?

> + * to access the page again will trigger page fault and the PF handler
> + * will kill the process.
> + *
> + * MF_RECOVERED - The m-f() handler marks the page as PG_hwpoisoned'ed.
> + * The page has been completely isolated, that is, unmapped, taken out of
> + * the buddy system, or hole-punnched out of the file mapping.
> + */
>  static const char *action_name[] = {
>  	[MF_IGNORED] = "Ignored",
>  	[MF_FAILED] = "Failed",
> @@ -893,6 +915,7 @@ static const char * const action_page_types[] = {
>  	[MF_MSG_DIFFERENT_COMPOUND]	= "different compound page after locking",
>  	[MF_MSG_HUGE]			= "huge page",
>  	[MF_MSG_FREE_HUGE]		= "free huge page",
> +	[MF_MSG_GET_HWPOISON]		= "get hwpoison page",
>  	[MF_MSG_UNMAP_FAILED]		= "unmapping failed page",
>  	[MF_MSG_DIRTY_SWAPCACHE]	= "dirty swapcache page",
>  	[MF_MSG_CLEAN_SWAPCACHE]	= "clean swapcache page",
> @@ -906,6 +929,7 @@ static const char * const action_page_types[] = {
>  	[MF_MSG_BUDDY]			= "free buddy page",
>  	[MF_MSG_DAX]			= "dax page",
>  	[MF_MSG_UNSPLIT_THP]		= "unsplit thp",
> +	[MF_MSG_ALREADY_POISONED]	= "already poisoned",
>  	[MF_MSG_UNKNOWN]		= "unknown page",
>  };
>  
> @@ -1013,12 +1037,13 @@ static int me_kernel(struct page_state *ps, struct page *p)
>  
>  /*
>   * Page in unknown state. Do nothing.
> + * This is a catch-all in case we fail to make sense of the page state.
>   */
>  static int me_unknown(struct page_state *ps, struct page *p)
>  {
>  	pr_err("%#lx: Unknown page state\n", page_to_pfn(p));
>  	unlock_page(p);
> -	return MF_FAILED;
> +	return MF_IGNORED;
>  }
>  
>  /*
> @@ -2055,6 +2080,8 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>  		if (flags & MF_ACTION_REQUIRED) {
>  			folio = page_folio(p);
>  			res = kill_accessing_process(current, folio_pfn(folio), flags);
> +			action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
> +			return res;

We might reuse the below "return res;"?

>  		}
>  		return res;

Besides from the above possible nits, this patch looks good to me.
Acked-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
Thanks.
.