| Message ID | 20201103130334.13468-6-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: misc migrate cleanup and improvement | expand |
On 3 Nov 2020, at 8:03, Yang Shi wrote: > In the current implementation unmap_and_move() would return -ENOMEM if > THP migration is unsupported, then the THP will be split. If split is > failed just exit without trying to migrate other pages. It doesn't make > too much sense since there may be enough free memory to migrate other > pages and there may be a lot base pages on the list. > > Return -ENOSYS to make consistent with hugetlb. And if THP split is > failed just skip and try other pages on the list. > > Just skip the whole list and exit when free memory is really low. > > Signed-off-by: Yang Shi <shy828301@gmail.com> > --- > mm/migrate.c | 62 ++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 46 insertions(+), 16 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 8f6a61c9274b..b3466d8c7f03 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1172,7 +1172,7 @@ static int unmap_and_move(new_page_t get_new_page, > struct page *newpage = NULL; > > if (!thp_migration_supported() && PageTransHuge(page)) > - return -ENOMEM; > + return -ENOSYS; > > if (page_count(page) == 1) { > /* page was freed from under us. So we are done. */ > @@ -1376,6 +1376,20 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, > return rc; > } > > +static inline int try_split_thp(struct page *page, struct page *page2, > + struct list_head *from) > +{ > + int rc = 0; > + > + lock_page(page); > + rc = split_huge_page_to_list(page, from); > + unlock_page(page); > + if (!rc) > + list_safe_reset_next(page, page2, lru); This does not work as expected, right? After macro expansion, we have page2 = list_next_entry(page, lru). Since page2 is passed as a pointer, the change does not return back the caller. You need to use the pointer to page2 here. > + > + return rc; > +} > + > /* > * migrate_pages - migrate the pages specified in a list, to the free pages > * supplied as the target for the page migration > @@ -1445,24 +1459,40 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > reason, &ret_pages); > > switch(rc) { > + /* > + * THP migration might be unsupported or the > + * allocation could've failed so we should > + * retry on the same page with the THP split > + * to base pages. > + * > + * Head page is retried immediately and tail > + * pages are added to the tail of the list so > + * we encounter them after the rest of the list > + * is processed. > + */ > + case -ENOSYS: > + /* THP migration is unsupported */ > + if (is_thp) { > + if (!try_split_thp(page, page2, from)) { > + nr_thp_split++; > + goto retry; > + } > + > + nr_thp_failed++; > + nr_failed += nr_subpages; > + break; > + } > + > + /* Hugetlb migration is unsupported */ > + nr_failed++; > + break; > case -ENOMEM: > /* > - * THP migration might be unsupported or the > - * allocation could've failed so we should > - * retry on the same page with the THP split > - * to base pages. > - * > - * Head page is retried immediately and tail > - * pages are added to the tail of the list so > - * we encounter them after the rest of the list > - * is processed. > + * When memory is low, don't bother to try to migrate > + * other pages, just exit. The comment does not match the code below. For THPs, the code tries to split the THP and migrate the base pages if the split is successful. > */ > if (is_thp) { > - lock_page(page); > - rc = split_huge_page_to_list(page, from); > - unlock_page(page); > - if (!rc) { > - list_safe_reset_next(page, page2, lru); > + if (!try_split_thp(page, page2, from)) { > nr_thp_split++; > goto retry; > } > @@ -1490,7 +1520,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > break; > default: > /* > - * Permanent failure (-EBUSY, -ENOSYS, etc.): > + * Permanent failure (-EBUSY, etc.): > * unlike -EAGAIN case, the failed page is > * removed from migration page list and not > * retried in the next outer loop. > -- > 2.26.2 — Best Regards, Yan Zi
On Fri, Nov 6, 2020 at 12:17 PM Zi Yan <ziy@nvidia.com> wrote: > > On 3 Nov 2020, at 8:03, Yang Shi wrote: > > > In the current implementation unmap_and_move() would return -ENOMEM if > > THP migration is unsupported, then the THP will be split. If split is > > failed just exit without trying to migrate other pages. It doesn't make > > too much sense since there may be enough free memory to migrate other > > pages and there may be a lot base pages on the list. > > > > Return -ENOSYS to make consistent with hugetlb. And if THP split is > > failed just skip and try other pages on the list. > > > > Just skip the whole list and exit when free memory is really low. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > mm/migrate.c | 62 ++++++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 46 insertions(+), 16 deletions(-) > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > index 8f6a61c9274b..b3466d8c7f03 100644 > > --- a/mm/migrate.c > > +++ b/mm/migrate.c > > @@ -1172,7 +1172,7 @@ static int unmap_and_move(new_page_t get_new_page, > > struct page *newpage = NULL; > > > > if (!thp_migration_supported() && PageTransHuge(page)) > > - return -ENOMEM; > > + return -ENOSYS; > > > > if (page_count(page) == 1) { > > /* page was freed from under us. So we are done. */ > > @@ -1376,6 +1376,20 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, > > return rc; > > } > > > > +static inline int try_split_thp(struct page *page, struct page *page2, > > + struct list_head *from) > > +{ > > + int rc = 0; > > + > > + lock_page(page); > > + rc = split_huge_page_to_list(page, from); > > + unlock_page(page); > > + if (!rc) > > + list_safe_reset_next(page, page2, lru); > > This does not work as expected, right? After macro expansion, we have > page2 = list_next_entry(page, lru). Since page2 is passed as a pointer, the change > does not return back the caller. You need to use the pointer to page2 here. Yes, I should pass in **page2. Thanks for catching this. > > > + > > + return rc; > > +} > > + > > /* > > * migrate_pages - migrate the pages specified in a list, to the free pages > > * supplied as the target for the page migration > > @@ -1445,24 +1459,40 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > > reason, &ret_pages); > > > > switch(rc) { > > + /* > > + * THP migration might be unsupported or the > > + * allocation could've failed so we should > > + * retry on the same page with the THP split > > + * to base pages. > > + * > > + * Head page is retried immediately and tail > > + * pages are added to the tail of the list so > > + * we encounter them after the rest of the list > > + * is processed. > > + */ > > + case -ENOSYS: > > + /* THP migration is unsupported */ > > + if (is_thp) { > > + if (!try_split_thp(page, page2, from)) { > > + nr_thp_split++; > > + goto retry; > > + } > > + > > + nr_thp_failed++; > > + nr_failed += nr_subpages; > > + break; > > + } > > + > > + /* Hugetlb migration is unsupported */ > > + nr_failed++; > > + break; > > case -ENOMEM: > > /* > > - * THP migration might be unsupported or the > > - * allocation could've failed so we should > > - * retry on the same page with the THP split > > - * to base pages. > > - * > > - * Head page is retried immediately and tail > > - * pages are added to the tail of the list so > > - * we encounter them after the rest of the list > > - * is processed. > > + * When memory is low, don't bother to try to migrate > > + * other pages, just exit. > > The comment does not match the code below. For THPs, the code tries to split the THP > and migrate the base pages if the split is successful. The comment here just covers the "goto out" rather than the split because I thought it is covered by the comment before "case -ENOSYS", which says "unsupported or the allocation could've failed", so I didn't repeat here. > > > */ > > if (is_thp) { > > - lock_page(page); > > - rc = split_huge_page_to_list(page, from); > > - unlock_page(page); > > - if (!rc) { > > - list_safe_reset_next(page, page2, lru); > > + if (!try_split_thp(page, page2, from)) { > > nr_thp_split++; > > goto retry; > > } > > @@ -1490,7 +1520,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > > break; > > default: > > /* > > - * Permanent failure (-EBUSY, -ENOSYS, etc.): > > + * Permanent failure (-EBUSY, etc.): > > * unlike -EAGAIN case, the failed page is > > * removed from migration page list and not > > * retried in the next outer loop. > > -- > > 2.26.2 > > > — > Best Regards, > Yan Zi
On Fri, Nov 6, 2020 at 12:17 PM Zi Yan <ziy@nvidia.com> wrote: > > On 3 Nov 2020, at 8:03, Yang Shi wrote: > > > In the current implementation unmap_and_move() would return -ENOMEM if > > THP migration is unsupported, then the THP will be split. If split is > > failed just exit without trying to migrate other pages. It doesn't make > > too much sense since there may be enough free memory to migrate other > > pages and there may be a lot base pages on the list. > > > > Return -ENOSYS to make consistent with hugetlb. And if THP split is > > failed just skip and try other pages on the list. > > > > Just skip the whole list and exit when free memory is really low. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > mm/migrate.c | 62 ++++++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 46 insertions(+), 16 deletions(-) > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > index 8f6a61c9274b..b3466d8c7f03 100644 > > --- a/mm/migrate.c > > +++ b/mm/migrate.c > > @@ -1172,7 +1172,7 @@ static int unmap_and_move(new_page_t get_new_page, > > struct page *newpage = NULL; > > > > if (!thp_migration_supported() && PageTransHuge(page)) > > - return -ENOMEM; > > + return -ENOSYS; > > > > if (page_count(page) == 1) { > > /* page was freed from under us. So we are done. */ > > @@ -1376,6 +1376,20 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, > > return rc; > > } > > > > +static inline int try_split_thp(struct page *page, struct page *page2, > > + struct list_head *from) > > +{ > > + int rc = 0; > > + > > + lock_page(page); > > + rc = split_huge_page_to_list(page, from); > > + unlock_page(page); > > + if (!rc) > > + list_safe_reset_next(page, page2, lru); > > This does not work as expected, right? After macro expansion, we have > page2 = list_next_entry(page, lru). Since page2 is passed as a pointer, the change > does not return back the caller. You need to use the pointer to page2 here. > > > + > > + return rc; > > +} > > + > > /* > > * migrate_pages - migrate the pages specified in a list, to the free pages > > * supplied as the target for the page migration > > @@ -1445,24 +1459,40 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > > reason, &ret_pages); > > > > switch(rc) { > > + /* > > + * THP migration might be unsupported or the > > + * allocation could've failed so we should > > + * retry on the same page with the THP split > > + * to base pages. > > + * > > + * Head page is retried immediately and tail > > + * pages are added to the tail of the list so > > + * we encounter them after the rest of the list > > + * is processed. > > + */ > > + case -ENOSYS: > > + /* THP migration is unsupported */ > > + if (is_thp) { > > + if (!try_split_thp(page, page2, from)) { > > + nr_thp_split++; > > + goto retry; > > + } > > + > > + nr_thp_failed++; > > + nr_failed += nr_subpages; > > + break; > > + } > > + > > + /* Hugetlb migration is unsupported */ > > + nr_failed++; > > + break; > > case -ENOMEM: > > /* > > - * THP migration might be unsupported or the > > - * allocation could've failed so we should > > - * retry on the same page with the THP split > > - * to base pages. > > - * > > - * Head page is retried immediately and tail > > - * pages are added to the tail of the list so > > - * we encounter them after the rest of the list > > - * is processed. > > + * When memory is low, don't bother to try to migrate > > + * other pages, just exit. > > The comment does not match the code below. For THPs, the code tries to split the THP > and migrate the base pages if the split is successful. BTW, actually I don't think -ENOSYS is a proper return value for this case since it typically means "the syscall doesn't exist". IMHO, it should return -EINVAL. And actually the return value doesn't matter since all callers of migrate_pages() just check if the return value != 0. And, neither -ENOSYS nor -EINVAL won't be visible by userspace since migrate_pages() just returns the number of failed pages for this case. Anyway the return value is a little bit messy, it may return -ENOMEM, 0 or nr_failed. But it looks the callers just care about if ret != 0, so it may be better to let it return nr_failed for -ENOMEM case too. > > > */ > > if (is_thp) { > > - lock_page(page); > > - rc = split_huge_page_to_list(page, from); > > - unlock_page(page); > > - if (!rc) { > > - list_safe_reset_next(page, page2, lru); > > + if (!try_split_thp(page, page2, from)) { > > nr_thp_split++; > > goto retry; > > } > > @@ -1490,7 +1520,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > > break; > > default: > > /* > > - * Permanent failure (-EBUSY, -ENOSYS, etc.): > > + * Permanent failure (-EBUSY, etc.): > > * unlike -EAGAIN case, the failed page is > > * removed from migration page list and not > > * retried in the next outer loop. > > -- > > 2.26.2 > > > — > Best Regards, > Yan Zi
On Fri, Nov 6, 2020 at 2:02 PM Yang Shi <shy828301@gmail.com> wrote: > > On Fri, Nov 6, 2020 at 12:17 PM Zi Yan <ziy@nvidia.com> wrote: > > > > On 3 Nov 2020, at 8:03, Yang Shi wrote: > > > > > In the current implementation unmap_and_move() would return -ENOMEM if > > > THP migration is unsupported, then the THP will be split. If split is > > > failed just exit without trying to migrate other pages. It doesn't make > > > too much sense since there may be enough free memory to migrate other > > > pages and there may be a lot base pages on the list. > > > > > > Return -ENOSYS to make consistent with hugetlb. And if THP split is > > > failed just skip and try other pages on the list. > > > > > > Just skip the whole list and exit when free memory is really low. > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > --- > > > mm/migrate.c | 62 ++++++++++++++++++++++++++++++++++++++-------------- > > > 1 file changed, 46 insertions(+), 16 deletions(-) > > > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > > index 8f6a61c9274b..b3466d8c7f03 100644 > > > --- a/mm/migrate.c > > > +++ b/mm/migrate.c > > > @@ -1172,7 +1172,7 @@ static int unmap_and_move(new_page_t get_new_page, > > > struct page *newpage = NULL; > > > > > > if (!thp_migration_supported() && PageTransHuge(page)) > > > - return -ENOMEM; > > > + return -ENOSYS; > > > > > > if (page_count(page) == 1) { > > > /* page was freed from under us. So we are done. */ > > > @@ -1376,6 +1376,20 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, > > > return rc; > > > } > > > > > > +static inline int try_split_thp(struct page *page, struct page *page2, > > > + struct list_head *from) > > > +{ > > > + int rc = 0; > > > + > > > + lock_page(page); > > > + rc = split_huge_page_to_list(page, from); > > > + unlock_page(page); > > > + if (!rc) > > > + list_safe_reset_next(page, page2, lru); > > > > This does not work as expected, right? After macro expansion, we have > > page2 = list_next_entry(page, lru). Since page2 is passed as a pointer, the change > > does not return back the caller. You need to use the pointer to page2 here. > > > > > + > > > + return rc; > > > +} > > > + > > > /* > > > * migrate_pages - migrate the pages specified in a list, to the free pages > > > * supplied as the target for the page migration > > > @@ -1445,24 +1459,40 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > > > reason, &ret_pages); > > > > > > switch(rc) { > > > + /* > > > + * THP migration might be unsupported or the > > > + * allocation could've failed so we should > > > + * retry on the same page with the THP split > > > + * to base pages. > > > + * > > > + * Head page is retried immediately and tail > > > + * pages are added to the tail of the list so > > > + * we encounter them after the rest of the list > > > + * is processed. > > > + */ > > > + case -ENOSYS: > > > + /* THP migration is unsupported */ > > > + if (is_thp) { > > > + if (!try_split_thp(page, page2, from)) { > > > + nr_thp_split++; > > > + goto retry; > > > + } > > > + > > > + nr_thp_failed++; > > > + nr_failed += nr_subpages; > > > + break; > > > + } > > > + > > > + /* Hugetlb migration is unsupported */ > > > + nr_failed++; > > > + break; > > > case -ENOMEM: > > > /* > > > - * THP migration might be unsupported or the > > > - * allocation could've failed so we should > > > - * retry on the same page with the THP split > > > - * to base pages. > > > - * > > > - * Head page is retried immediately and tail > > > - * pages are added to the tail of the list so > > > - * we encounter them after the rest of the list > > > - * is processed. > > > + * When memory is low, don't bother to try to migrate > > > + * other pages, just exit. > > > > The comment does not match the code below. For THPs, the code tries to split the THP > > and migrate the base pages if the split is successful. > > BTW, actually I don't think -ENOSYS is a proper return value for this > case since it typically means "the syscall doesn't exist". IMHO, it > should return -EINVAL. And actually the return value doesn't matter > since all callers of migrate_pages() just check if the return value != > 0. And, neither -ENOSYS nor -EINVAL won't be visible by userspace > since migrate_pages() just returns the number of failed pages for this > case. > > Anyway the return value is a little bit messy, it may return -ENOMEM, > 0 or nr_failed. But it looks the callers just care about if ret != 0, > so it may be better to let it return nr_failed for -ENOMEM case too. By relooking all the callsites, it seems I overlooked two cases: * compaction: it checks if the return value is -ENOMEM if so it may return COMPACT_CONTENDED * migrate_pages syscall: it may return -ENOMEM to the userspace, but the man page doesn't document this case, not sure if it is legitimate > > > > > > */ > > > if (is_thp) { > > > - lock_page(page); > > > - rc = split_huge_page_to_list(page, from); > > > - unlock_page(page); > > > - if (!rc) { > > > - list_safe_reset_next(page, page2, lru); > > > + if (!try_split_thp(page, page2, from)) { > > > nr_thp_split++; > > > goto retry; > > > } > > > @@ -1490,7 +1520,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > > > break; > > > default: > > > /* > > > - * Permanent failure (-EBUSY, -ENOSYS, etc.): > > > + * Permanent failure (-EBUSY, etc.): > > > * unlike -EAGAIN case, the failed page is > > > * removed from migration page list and not > > > * retried in the next outer loop. > > > -- > > > 2.26.2 > > > > > > — > > Best Regards, > > Yan Zi
diff --git a/mm/migrate.c b/mm/migrate.c index 8f6a61c9274b..b3466d8c7f03 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1172,7 +1172,7 @@ static int unmap_and_move(new_page_t get_new_page, struct page *newpage = NULL; if (!thp_migration_supported() && PageTransHuge(page)) - return -ENOMEM; + return -ENOSYS; if (page_count(page) == 1) { /* page was freed from under us. So we are done. */ @@ -1376,6 +1376,20 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, return rc; } +static inline int try_split_thp(struct page *page, struct page *page2, + struct list_head *from) +{ + int rc = 0; + + lock_page(page); + rc = split_huge_page_to_list(page, from); + unlock_page(page); + if (!rc) + list_safe_reset_next(page, page2, lru); + + return rc; +} + /* * migrate_pages - migrate the pages specified in a list, to the free pages * supplied as the target for the page migration @@ -1445,24 +1459,40 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, reason, &ret_pages); switch(rc) { + /* + * THP migration might be unsupported or the + * allocation could've failed so we should + * retry on the same page with the THP split + * to base pages. + * + * Head page is retried immediately and tail + * pages are added to the tail of the list so + * we encounter them after the rest of the list + * is processed. + */ + case -ENOSYS: + /* THP migration is unsupported */ + if (is_thp) { + if (!try_split_thp(page, page2, from)) { + nr_thp_split++; + goto retry; + } + + nr_thp_failed++; + nr_failed += nr_subpages; + break; + } + + /* Hugetlb migration is unsupported */ + nr_failed++; + break; case -ENOMEM: /* - * THP migration might be unsupported or the - * allocation could've failed so we should - * retry on the same page with the THP split - * to base pages. - * - * Head page is retried immediately and tail - * pages are added to the tail of the list so - * we encounter them after the rest of the list - * is processed. + * When memory is low, don't bother to try to migrate + * other pages, just exit. */ if (is_thp) { - lock_page(page); - rc = split_huge_page_to_list(page, from); - unlock_page(page); - if (!rc) { - list_safe_reset_next(page, page2, lru); + if (!try_split_thp(page, page2, from)) { nr_thp_split++; goto retry; } @@ -1490,7 +1520,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, break; default: /* - * Permanent failure (-EBUSY, -ENOSYS, etc.): + * Permanent failure (-EBUSY, etc.): * unlike -EAGAIN case, the failed page is * removed from migration page list and not * retried in the next outer loop.
In the current implementation unmap_and_move() would return -ENOMEM if THP migration is unsupported, then the THP will be split. If split is failed just exit without trying to migrate other pages. It doesn't make too much sense since there may be enough free memory to migrate other pages and there may be a lot base pages on the list. Return -ENOSYS to make consistent with hugetlb. And if THP split is failed just skip and try other pages on the list. Just skip the whole list and exit when free memory is really low. Signed-off-by: Yang Shi <shy828301@gmail.com> --- mm/migrate.c | 62 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 16 deletions(-)