Re: [PATCH v3 3/3] mm/memory-failure: refactor page_handle_poison()

[Miaohe Lin <linmiaohe@huawei.com>]

On 2026/1/12 8:49, Jiaqi Yan wrote:
> Now that HWPoison page(s) within HugeTLB page will be rejected by
> buddy allocator during dissolve_free_hugetlb_folio(), there is no
> need to drain_all_pages() and take_page_off_buddy() anymore. In fact,
> calling take_page_off_buddy() after dissolve_free_hugetlb_folio()
> succeeded returns false, making caller think page_handl_poion() failed.

s/page_handl_poion/page_handle_poison/

> 
> On the other hand, for hardware corrupted pages in buddy allocator,
> take_page_off_buddy() is still a must-have.
> 
> Given hugepage and free buddy page should be treated differently,
> refactor page_handle_poison() and __page_handle_poison():
> 
> - __page_handle_poison() is unwind into page_handle_poison().
> 
> - Callers of page_handle_poison() also need to explicitly tell if
>   page is HugeTLB hugepage or free buddy page.
> 
> - Add helper hugepage_handle_poison() for several existing HugeTLB
>   specific callsites.
> 
> Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> ---
>  mm/memory-failure.c | 84 ++++++++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 43 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index d204de6c9792a..1fdaee1e48bb8 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -162,54 +162,48 @@ static struct rb_root_cached pfn_space_itree = RB_ROOT_CACHED;
>  
>  static DEFINE_MUTEX(pfn_space_lock);
>  
> -/*
> - * Return values:
> - *   1:   the page is dissolved (if needed) and taken off from buddy,
> - *   0:   the page is dissolved (if needed) and not taken off from buddy,
> - *   < 0: failed to dissolve.
> +/**
> + * Handle the HugeTLB hugepage that @page belongs to. Return values:
> + *   = 0: the hugepage is free hugepage and is dissolved.

In soft offline scene, dissolve_free_hugetlb_folio would return 0 when the page becomes
a normal page due to race.

> + *   < 0: hugepage is in-use or failed to dissolve.
>   */
> -static int __page_handle_poison(struct page *page)
> +static int hugepage_handle_poison(struct page *page)
>  {
> -	int ret;
> +	return dissolve_free_hugetlb_folio(page_folio(page));
> +}
> +
> +/**
> + * Helper at the end of handling @page having hardware errors.
> + * @huge: @page is part of a HugeTLB hugepage.
> + * @free: @page is free buddy page.
> + * @release: memory-failure module should release a pending refcount.
> + */
> +static bool page_handle_poison(struct page *page, bool huge, bool free,
> +			       bool release)
> +{
> +	int ret = 0;
>  
>  	/*
> -	 * zone_pcp_disable() can't be used here. It will
> -	 * hold pcp_batch_high_lock and dissolve_free_hugetlb_folio() might hold
> -	 * cpu_hotplug_lock via static_key_slow_dec() when hugetlb vmemmap
> -	 * optimization is enabled. This will break current lock dependency
> -	 * chain and leads to deadlock.
> -	 * Disabling pcp before dissolving the page was a deterministic
> -	 * approach because we made sure that those pages cannot end up in any
> -	 * PCP list. Draining PCP lists expels those pages to the buddy system,
> -	 * but nothing guarantees that those pages do not get back to a PCP
> -	 * queue if we need to refill those.
> +	 * Buddy allocator will exclude the HWPoison page after hugepage
> +	 * is successfully dissolved.
>  	 */
> -	ret = dissolve_free_hugetlb_folio(page_folio(page));
> -	if (!ret) {
> +	if (huge)
> +		ret = hugepage_handle_poison(page);
> +
> +	if (free) {

Nit: huge and free won't be both true. So we could write it as:
	if (huge) {
		...
	} else if (free) {

>  		drain_all_pages(page_zone(page));
> -		ret = take_page_off_buddy(page);
> +		ret = take_page_off_buddy(page) ? 0 : -1;
>  	}
>  
> -	return ret;
> -}
> -
> -static bool page_handle_poison(struct page *page, bool hugepage_or_freepage, bool release)
> -{
> -	if (hugepage_or_freepage) {
> +	if ((huge || free) && ret < 0)

Nit: ret won't be <0 if both huge and free are false. So I think we might simplify it as:

	if (ret < 0)

>  		/*
> -		 * Doing this check for free pages is also fine since
> -		 * dissolve_free_hugetlb_folio() returns 0 for non-hugetlb folios as well.
> +		 * We could fail to take off the target page from buddy
> +		 * for example due to racy page allocation, but that's
> +		 * acceptable because soft-offlined page is not broken
> +		 * and if someone really want to use it, they should
> +		 * take it.
>  		 */
> -		if (__page_handle_poison(page) <= 0)
> -			/*
> -			 * We could fail to take off the target page from buddy
> -			 * for example due to racy page allocation, but that's
> -			 * acceptable because soft-offlined page is not broken
> -			 * and if someone really want to use it, they should
> -			 * take it.
> -			 */
> -			return false;
> -	}
> +		return false;
>  
>  	SetPageHWPoison(page);
>  	if (release)
> @@ -1174,7 +1168,7 @@ static int me_huge_page(struct page_state *ps, struct page *p)
>  		 * subpages.
>  		 */
>  		folio_put(folio);
> -		if (__page_handle_poison(p) > 0) {
> +		if (!hugepage_handle_poison(p)) {
>  			page_ref_inc(p);
>  			res = MF_RECOVERED;
>  		} else {
> @@ -2067,7 +2061,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>  	 */
>  	if (res == 0) {
>  		folio_unlock(folio);
> -		if (__page_handle_poison(p) > 0) {
> +		if (!hugepage_handle_poison(p)) {
>  			page_ref_inc(p);
>  			res = MF_RECOVERED;
>  		} else {
> @@ -2815,7 +2809,7 @@ static int soft_offline_in_use_page(struct page *page)
>  
>  	if (ret) {
>  		pr_info("%#lx: invalidated\n", pfn);
> -		page_handle_poison(page, false, true);
> +		page_handle_poison(page, false, false, true);
>  		return 0;
>  	}
>  
> @@ -2836,7 +2830,7 @@ static int soft_offline_in_use_page(struct page *page)
>  		if (!ret) {
>  			bool release = !huge;
>  
> -			if (!page_handle_poison(page, huge, release))
> +			if (!page_handle_poison(page, huge, false, release))

This might not work for soft offline. PageHWPoison is not yet set so folio_clear_hugetlb_hwpoison
won't be called when dissolve hugetlb hugepages...

>  				ret = -EBUSY;
>  		} else {
>  			if (!list_empty(&pagelist))
> @@ -2884,6 +2878,8 @@ int soft_offline_page(unsigned long pfn, int flags)
>  {
>  	int ret;
>  	bool try_again = true;
> +	bool huge;
> +	bool free;
>  	struct page *page;
>  
>  	if (!pfn_valid(pfn)) {
> @@ -2929,7 +2925,9 @@ int soft_offline_page(unsigned long pfn, int flags)
>  	if (ret > 0) {
>  		ret = soft_offline_in_use_page(page);
>  	} else if (ret == 0) {
> -		if (!page_handle_poison(page, true, false)) {
> +		huge = folio_test_hugetlb(page_folio(page));

folio_test_hugetlb check is racy because there's no guarantee that hugetlb hugepage won't
be dissolved before calling page_handle_poison. That will lead to problem...

soft_offline_page
  folio_test_hugetlb -- true now
  page_handle_poison
  /* Hugepage is dissolved somewhere. */
    hugepage_handle_poison -- return 0 because page is normal page or free buddy page.
    SetPageHWPoison(page);
    page_ref_inc(page); -- refcnt is increased while page might be on buddy...

> +		free = is_free_buddy_page(page);
> +		if (!page_handle_poison(page, huge, free, false)) {

We assume free is always true due to ret is 0. So we can write it as:
	if (!page_handle_poison(page, huge, true, false)) {

>  			if (try_again) {
>  				try_again = false;
>  				flags &= ~MF_COUNT_INCREASED;
> 

Thanks.
.