Re: [PATCH v4 0/7] HWpoison: further fixes and cleanups

[HORIGUCHI NAOYA(堀口 直也)]

On Thu, Sep 17, 2020 at 03:40:06PM +0200, Oscar Salvador wrote:
> On Thu, Sep 17, 2020 at 03:09:52PM +0200, Oscar Salvador wrote:
> > static bool page_handle_poison(struct page *page, bool hugepage_or_freepage, bool release)
> > {
> >         if (release) {
> >                 put_page(page);
> >                 drain_all_pages(page_zone(page));
> >         }
> >
> > 	...
> >         SetPageHWPoison(page);
> >         page_ref_inc(page);
> >
> > 1) You are freeing the page first, which means it goes to buddy
> > 2) Then you set it as poisoned and you update its refcount.
> >
> > Now we have a page sitting in Buddy with a refcount = 1 and poisoned, and that is quite wrong.
>
> Hi Naoya,
>
> Ok, I tested it and with the following changes on top I cannot reproduce the issue:
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index f68cb5e3b320..4ffaaa5c2603 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -67,11 +67,6 @@ atomic_long_t num_poisoned_pages __read_mostly = ATOMIC_LONG_INIT(0);
>
>  static bool page_handle_poison(struct page *page, bool hugepage_or_freepage, bool release)
>  {
> -	if (release) {
> -		put_page(page);
> -		drain_all_pages(page_zone(page));
> -	}
> -
>  	if (hugepage_or_freepage) {
>  		/*
>  		 * Doing this check for free pages is also fine since dissolve_free_huge_page
> @@ -89,6 +84,12 @@ static bool page_handle_poison(struct page *page, bool hugepage_or_freepage, boo
>  	}
>
>  	SetPageHWPoison(page);
> +
> +	if (release) {
> +                put_page(page);
> +                drain_all_pages(page_zone(page));
> +        }
> +
>  	page_ref_inc(page);
>  	num_poisoned_pages_inc();
>  	return true;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0d9f9bd0e06c..8a6ab05f074c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1173,6 +1173,16 @@ static __always_inline bool free_pages_prepare(struct page *page,
>
>  	trace_mm_page_free(page, order);
>
> +	if (unlikely(PageHWPoison(page)) && !order) {
> +		/*
> +		 * Untie memcg state and reset page's owner
> +		 */
> +		if (memcg_kmem_enabled() && PageKmemcg(page))
> +			__memcg_kmem_uncharge_page(page, order);
> +		reset_page_owner(page, order);
> +		return false;
> +	}
> +

Sorry, I modified the patches based on the different assumption from yours.
I firstly thought of taking page off after confirming the error page
is freed back to buddy. This approach leaves the possibility of reusing
the error page (which is acceptable), but simpler and less invasive one.

Your approach removes the error page from page allocator's control in
freeing time. It has no possibility of reusing the error page but changes
are tightly coupled with page free code.

This is a tradeoff between complexity and completeness of soft offline,
Now I'm not sure I could persist on my own opinion without providing
working code, and it's OK for me to take your one.

>  	/*
>  	 * Check tail pages before head page information is cleared to
>  	 * avoid checking PageCompound for order-0 pages.
>
>

...
(let me reply to older email here...)
> static bool page_handle_poison(struct page *page, bool hugepage_or_freepage, bool release)
> {
>         if (release) {
>                 put_page(page);
>                 drain_all_pages(page_zone(page));
>         }
>
>       ...
>         SetPageHWPoison(page);
>         page_ref_inc(page);
>
> 1) You are freeing the page first, which means it goes to buddy
> 2) Then you set it as poisoned and you update its refcount.
>
> Now we have a page sitting in Buddy with a refcount = 1 and poisoned, and that is quite wrong.

This order was correct in my approach. Isolation operation is done
after confirming it's in the free list. This prevents PageHWPoison pages
from being in pcpclists. But the page_ref_inc() may not be necessary in my approach.


> # sh tmp_run_ksm_madv.sh
> p1 0x7f6b6b08e000
> p2 0x7f6b529ee000
> madvise(p1) 0
> madvise(p2) 0
> writing p1 ... done
> writing p2 ... done
> soft offline
> soft offline returns 0
> OK
>
>
> Can you try to re-run your tests and see if they come clean?

The test passed in my environment, so this is fine.

> If they do, I will try to see if Andrew can squezee above changes into [1],
> where they belong to.

Yes, proposing the fix for mmhwpoison-rework-soft-offline-for-in-use-pages.patch
seems fine to me.

Again, sorry for modifying code without asking.

Naoya Horiguchi