| Message ID | 20180523073547.GA29266@techadventures.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
On Wed 23-05-18 10:16:49, Oscar Salvador wrote: [...] > AFAIU, permament errors are things like -EBUSY, -ENOSYS, -ENOMEM, > and a temporary one would be -EAGAIN? It would be really great to have EBUSY as permanent and ENOMEM and EAGAIN as temporary failures. But this is not so easy. The migration code failes on the elevated ref count usually and we simply do not know whether this is a short term pin or somebody holding the reference basically for ever (from the migration POV). There was some discussion about longterm pins on pages at LSFMM this year but it will take quite some time before we will get some working solution. > 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) I am not sure we really need a new parameter. migrate_pages will tell us the failure. We just do not know _which_ error to return currently.
On 23.05.2018 09:35, 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. > 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. > > 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. > So while trying to remove that memory, the system failed in: > > do_migrate_range() > { > ... > if (PageLRU(page)) > ret = isolate_lru_page(page); > else > ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE); > > if (!ret) > // success: do something > else > if (page_count(page)) > ret = -EBUSY; > ... > } > > 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. > 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; > } > > Now, unless I overlooked something > migrate_pages()->unmap_and_move()/unmap_and_move_huge_page() can return: > -ENOMEM > -EAGAIN > -EBUSY > -ENOSYS. > > I am not sure if we should differentiate betweeen those errors. > For example, it is possible that in migrate_pages() we just get -EAGAIN, > and we return the number of "retry" we tried without having really failed. > Although, since we do 10 passes it might be considered as failed. > > And I am not sure either if we want to propagate the error codes, or in case we fail > in migrate_pages(), whatever the error was (-ENOMEM, -EBUSY, etc.), we > just return -EBUSY. > > What do you think? Hi, While working on onlining/offlining of 4MB subsections I also stumbled over the return value of offline_pages(). It would be nice if the interface could actually indicate if an error is permanent or only temporary. For now I have to live with the assumption, that whenever this function is not -EAGAIN or 0, that I simply have to retry later. David > > Thanks > Oscar Salvador >
--- 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; }
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. 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. 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. So while trying to remove that memory, the system failed in: do_migrate_range() { ... if (PageLRU(page)) ret = isolate_lru_page(page); else ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE); if (!ret) // success: do something else if (page_count(page)) ret = -EBUSY; ... } 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. I think we should really detect these cases and fail with "goto failed_removal". Something like Now, unless I overlooked something migrate_pages()->unmap_and_move()/unmap_and_move_huge_page() can return: -ENOMEM -EAGAIN -EBUSY -ENOSYS. I am not sure if we should differentiate betweeen those errors. For example, it is possible that in migrate_pages() we just get -EAGAIN, and we return the number of "retry" we tried without having really failed. Although, since we do 10 passes it might be considered as failed. And I am not sure either if we want to propagate the error codes, or in case we fail in migrate_pages(), whatever the error was (-ENOMEM, -EBUSY, etc.), we just return -EBUSY. What do you think? Thanks Oscar Salvador