Re: [RFC] Checking for error code in __offline_pages

[Oscar Salvador <osalvador@techadventures.net>]

On Wed, May 23, 2018 at 09:52:39AM +0200, Michal Hocko wrote:
> On Wed 23-05-18 09:35:47, Oscar Salvador wrote:
> > Hi,
> > 
> > This is something I spotted while testing offlining memory.
> > 
> > __offline_pages() calls do_migrate_range() to try to migrate a range,
> > but we do not actually check for the error code.
> 
> Yes, this is intentional. do_migrate_range doesn't distinguish between
> temporal and permanent migration failure. Getting EBUSY would be just
> too easy and that is why we retry. We rely on start_isolate_page_range
> to tell us about any non-migrateable pages and we consider all other
> failures as temporal.
> 
> > This, besides of ignoring underlying failures, can led to a situations
> > where we never break up the loop because we are totally unaware of
> > what is going on.
> 
> This shouldn't happen. If it does then start_isolate_page_range should
> handle those non-migrateable pages.
> 
> > They way I spotted this was when trying to offline all memblocks belonging
> > to a node.
> > Due to an unfortunate setting with movablecore, memblocks containing bootmem
> > memory (pages marked by get_page_bootmem()) ended up marked in zone_movable.
> 
> This is a bug as well. Zone movable shouldn't contain any
> non-migrateable pages.
> 
> [...]
> 
> > Since the pages from bootmem are not LRU, we call isolate_movable_page()
> > but we fail when checking for __PageMovable().
> > Since the page_count is more than 0 we return -EBUSY, but we do not check this
> > in our caller, so we keep trying to migrate this memory over and over:
> > 
> > repeat:
> > ...
> >         pfn = scan_movable_pages(start_pfn, end_pfn);
> >         if (pfn) { /* We have movable pages */
> >                 ret = do_migrate_range(pfn, end_pfn);
> >                 goto repeat;
> >         }
> > 
> > But this is not only situation where we can get stuck.
> > For example, if we fail with -ENOMEM in
> > migrate_pages()->unmap_and_move()/unmap_and_move_huge_page(), we will keep trying as well.
> 
> ENOMEM is highly unlikely because we are should be allocating only small
> order pages and those do not fail unless the originator is killed by the
> oom killer and we would break out of the loop in such a cace because of
> signals pending.
> 
> > I think we should really detect these cases and fail with "goto failed_removal".
> > Something like
> > 
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1651,6 +1651,11 @@ static int __ref __offline_pages(unsigned long start_pfn,
> >         pfn = scan_movable_pages(start_pfn, end_pfn);
> >         if (pfn) { /* We have movable pages */
> >                 ret = do_migrate_range(pfn, end_pfn);
> > +               if (ret) {
> > +                       if (ret != -ENOMEM)
> > +                               ret = -EBUSY;
> > +                       goto failed_removal;
> > +               }
> >                 goto repeat;
> >         }
> 
> no, not really. As explained above this would allow to fail the
> offlining way too easily. Yeah, the current code is far from optimal. We
> used to have a retry count but that one was removed exactly because of
> premature failures. There are three things here
> 1) zone_movable should contain any bootmem or otherwise non-migrateable
>    pages
> 2) start_isolate_page_range should fail when seeing such pages - maybe
>    has_unmovable_pages is overly optimistic and it should check all
>    pages even in movable zones.

I will see if I can work this out.

> 3) migrate_pages should really tell us whether the failure is temporal
>    or permanent. I am not sure we can do that easily though.

AFAIU, permament errors are things like -EBUSY, -ENOSYS, -ENOMEM,
and a temporary one would be -EAGAIN?
Maybe it is overcomplicated, but what about adding another parameter to
migrate_pages() where we set the real error.
something like:

int migrate_pages(struct list_head *from, new_page_t get_new_page,
		free_page_t put_new_page, unsigned long private,
		enum migrate_mode mode, int reason, int *error)

Now it is not possible to find out why did we fail there.
We just get the number of pages that were not migrated (unless it is -ENOMEM, 
which completely bails out and returns that)
For -EBUSY,-ENOSYS and -EAGAIN we just increment some value and return it.

Although as I said, this might be overcomplicating things.

Oscar Salvador