Re: [PATCH 1/1] mm: memory-failure: Re-split hw-poisoned huge page on -EAGAIN

[Miaohe Lin <linmiaohe@huawei.com>]

On 2023/12/15 16:12, Qiuxu Zhuo wrote:
> During the process of splitting a hw-poisoned huge page, it is possible
> for the reference count of the huge page to be increased by the threads
> within the affected process, leading to a failure in splitting the
> hw-poisoned huge page with an error code of -EAGAIN.
> 
> This issue can be reproduced when doing memory error injection to a
> multiple-thread process, and the error occurs within a huge page.
> The call path with the returned -EAGAIN during the testing is shown below:
> 
>   memory_failure()
>     try_to_split_thp_page()
>       split_huge_page()
>         split_huge_page_to_list() {
>           ...
>           Step A: can_split_folio() - Checked that the thp can be split.
>           Step B: unmap_folio()
>           Step C: folio_ref_freeze() - Failed and returned -EAGAIN.
>           ...
>         }
> 
> The testing logs indicated that some huge pages were split successfully
> via the call path above (Step C was successful for these huge pages).
> However, some huge pages failed to split due to a failure at Step C, and
> it was observed that the reference count of the huge page increased between
> Step A and Step C.
> 
> Testing has shown that after receiving -EAGAIN, simply re-splitting the
> hw-poisoned huge page within memory_failure() always results in the same
> -EAGAIN. This is possible because memory_failure() is executed in the
> currently affected process. Before this process exits memory_failure() and
> is terminated, its threads could increase the reference count of the
> hw-poisoned page.
> 
> To address this issue, employ the kernel worker to re-split the hw-poisoned
> huge page. By the time this worker begins re-splitting the hw-poisoned huge
> page, the affected process has already been terminated, preventing its
> threads from increasing the reference count. Experimental results have
> consistently shown that this worker successfully re-splits these
> hw-poisoned huge pages on its first attempt.
> 
> The kernel log (before):
>   [ 1116.862895] Memory failure: 0x4097fa7: recovery action for unsplit thp: Ignored
> 
> The kernel log (after):
>   [  793.573536] Memory failure: 0x2100dda: recovery action for unsplit thp: Delayed
>   [  793.574666] Memory failure: 0x2100dda: split unsplit thp successfully.
> 
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Thanks for your patch. Except for the comment from Naoya, I have some questions about the code itself.

> ---
>  mm/memory-failure.c | 73 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 660c21859118..0db4cf712a78 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -72,6 +72,60 @@ atomic_long_t num_poisoned_pages __read_mostly = ATOMIC_LONG_INIT(0);
>  
>  static bool hw_memory_failure __read_mostly = false;
>  
> +#define SPLIT_THP_MAX_RETRY_CNT		10
> +#define SPLIT_THP_INIT_DELAYED_MS	1
> +
> +static bool split_thp_pending;
> +
> +struct split_thp_req {
> +	struct delayed_work work;
> +	struct page *thp;
> +	int retries;
> +};
> +
> +static void split_thp_work_fn(struct work_struct *work)
> +{
> +	struct split_thp_req *req = container_of(work, typeof(*req), work.work);
> +	int ret;
> +
> +	/* Split the thp. */
> +	get_page(req->thp);

Can req->thp be freed when split_thp_work_fn is scheduled ?

> +	lock_page(req->thp);
> +	ret = split_huge_page(req->thp);
> +	unlock_page(req->thp);
> +	put_page(req->thp);
> +
> +	/* Retry with an exponential backoff. */
> +	if (ret && ++req->retries < SPLIT_THP_MAX_RETRY_CNT) {
> +		schedule_delayed_work(to_delayed_work(work),
> +				      msecs_to_jiffies(SPLIT_THP_INIT_DELAYED_MS << req->retries));
> +		return;
> +	}
> +
> +	pr_err("%#lx: split unsplit thp %ssuccessfully.\n", page_to_pfn(req->thp), ret ? "un" : "");
> +	kfree(req);
> +	split_thp_pending = false;

split_thp_pending is not protected against split_thp_delayed? Though this race should be benign.

Thanks.