Re: [RFC] Checking for error code in __offline_pages

[Michal Hocko <mhocko@suse.com>]

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.
3) migrate_pages should really tell us whether the failure is temporal
   or permanent. I am not sure we can do that easily though.
-- 
Michal Hocko
SUSE Labs