Re: [PATCH v2 1/2] mm: fix race on soft-offlining free huge pages

[Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>]

On Thu, Jul 19, 2018 at 10:27:43AM +0200, Michal Hocko wrote:
> On Thu 19-07-18 08:08:05, Naoya Horiguchi wrote:
> > On Thu, Jul 19, 2018 at 09:15:16AM +0200, Michal Hocko wrote:
> > > On Thu 19-07-18 06:19:45, Naoya Horiguchi wrote:
> > > > On Wed, Jul 18, 2018 at 10:50:32AM +0200, Michal Hocko wrote:
> [...]
> > > > > Why do we even need HWPoison flag here? Everything can be completely
> > > > > transparent to the application. It shouldn't fail from what I
> > > > > understood.
> > > > 
> > > > PageHWPoison flag is used to the 'remove from the allocator' part
> > > > which is like below:
> > > > 
> > > >   static inline
> > > >   struct page *rmqueue(
> > > >           ...
> > > >           do {
> > > >                   page = NULL;
> > > >                   if (alloc_flags & ALLOC_HARDER) {
> > > >                           page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
> > > >                           if (page)
> > > >                                   trace_mm_page_alloc_zone_locked(page, order, migratetype);
> > > >                   }
> > > >                   if (!page)
> > > >                           page = __rmqueue(zone, order, migratetype);
> > > >           } while (page && check_new_pages(page, order));
> > > > 
> > > > check_new_pages() returns true if the page taken from free list has
> > > > a hwpoison page so that the allocator iterates another round to get
> > > > another page.
> > > > 
> > > > There's no function that can be called from outside allocator to remove
> > > > a page in allocator.  So actual page removal is done at allocation time,
> > > > not at error handling time. That's the reason why we need PageHWPoison.
> > > 
> > > hwpoison is an internal mm functionality so why cannot we simply add a
> > > function that would do that?
> > 
> > That's one possible solution.
> 
> I would prefer that much more than add an overhead (albeit small) into
> the page allocator directly. HWPoison should be a really rare event so
> why should everybody pay the price? I would much rather see that the
> poison path pays the additional price.

Yes, that's more maintainable.

> 
> > I know about another downside in current implementation.
> > If a hwpoison page is found during high order page allocation,
> > all 2^order pages (not only hwpoison page) are removed from
> > buddy because of the above quoted code. And these leaked pages
> > are never returned to freelist even with unpoison_memory().
> > If we have a page removal function which properly splits high order
> > free pages into lower order pages, this problem is avoided.
> 
> Even more reason to move to a new scheme.
> 
> > OTOH PageHWPoison still has a role to report error to userspace.
> > Without it unpoison_memory() doesn't work.
> 
> Sure but we do not really need a special page flag for that. We know the
> page is not reachable other than via pfn walkers. If you make the page
> reserved and note the fact it has been poisoned in the past then you can
> emulate the missing functionality.
> 
> Btw. do we really need unpoisoning functionality? Who is really using
> it, other than some tests?

None, as long as I know.

> How does the memory become OK again?

For hard-offlined in-use pages which are assumed to be pinned,
we clear the PageHWPoison flag and unpin the page to return it to buddy.
For other cases, we simply clear the PageHWPoison flag.
Unless the page is checked by check_new_pages() before unpoison,
the page is reusable.

Sometimes error handling fails and the error page might turn into
unexpected state (like additional refcount/mapcount).
Unpoison just fails on such pages.

> Don't we
> really need to go through physical hotremove & hotadd to clean the
> poison status?

hotremove/hotadd can be a user of unpoison, but I think simply
reinitializing struct pages is easiler.

Thanks,
Naoya Horiguchi