Re: poisoned pages do not play well in the buddy allocator

[Oscar Salvador]

On Fri, Aug 30, 2019 at 02:36:56PM +0200, Michal Hocko wrote:
> > - Free page: remove it from the buddy allocator and set it as PageReserved|PageHWPoison.
> > - Used page: migrate it and do not release it (skip put_page in unmap_and_move for MR_MEMORY_FAILURE
> > 	     reason). Set it as PageReserved|PageHWPoison.
> 
> But this will only cover mapped pages. What about page cache in general?
> Any reason why this cannot be handled in __free_one_page and simply skip
> the whole freeing of the HWPoisoned parts of the freed page (in case of
> higher order).

I forgot to mention that part.
pages that are in the page cache and are not mapped are being handled in
invalidate_inode_page:


	/*
         * Try to invalidate first. This should work for
         * non dirty unmapped page cache pages.
         */
        ret = invalidate_inode_page(page);

Once done, we free the page to the buddy (which is wrong).

My approach would be as we do when migrate a poisoned page, simply not release
the page, so it does not end up in the buddy and we have full control of it.

I am still playing with the way to go here in general, to see which approach
is better and more simple.
The implementation I have right works well, but it is true that we could explore
a way to

1) Set PageHWPoison bit on the page
1) Hook into the free routine and ignore any poisoned page

so the overall code could be easier.

I just want to see both codes in place and decide which one feels better.

> > The routine that handles this also sets the refcount of these pages to 1, so the unpoison
> > machinery will only have to check for PageHWPoison and to a put_page() to send
> > the page to the buddy allocator.
> > 
> > The Reserved bit is used because these pages will now __only__ be accessible through
> > pfn walkers, and pfn walkers should respect Reserved pages.
> > The PageHWPoison bit is used to remember that this page is poisoned, so the unpoison
> > machinery knows that it is valid to unpoison it.
> 
> Do we really need both bits? pfn walkers in general shouldn't handle
> pages they do not know about.

Well, I went for setting the Reserved bit to just be overprotective here,
like a way of "stay away from this page", and most of the pfn walkers
skip over Reserved pages as these are not meant to be touched for anyone
but the owner.
Setting the Poison bit is just for the unpoison routine, to check that
the page we are asked to unpoison, was really poisoned in the first place.

So, if that goes as planned, PageHWPoison check should only be neded in the
hwpoison code.
Maybe in the free routine if we decide to hook into that.

All in all, it is something that we will have to discuss at the time I will
send the RFC, as I am pretty sure there will be things to polish and change.

-- 
Oscar Salvador
SUSE L3