Re: [PATCH 2/5] mm: migrate: simplify the logic for handling permanent failure
On Fri, Nov 6, 2020 at 12:03 PM Zi Yan wrote:
>
> On 3 Nov 2020, at 8:03, Yang Shi wrote:
>
> > When unmap_and_move{_huge_page}() returns !-EAGAIN and !MIGRATEPAGE_SUCCESS,
> > the page would be put back to LRU or proper list if it is non-LRU movable
> > page. But, the callers always call putback_movable_pages() to put the
> > failed pages back later on, so it seems not very efficient to put every
> > single page back immediately, and the code looks convoluted.
> >
> > Put the failed page on a separate list, then splice the list to migrate
> > list when all pages are tried. It is the caller's responsibility to
> > call putback_movable_pages() to handle failures. This also makes the
> > code simpler and more readable.
> >
> > After the change the rules are:
> > * Success: non hugetlb page will be freed, hugetlb page will be put
> >back
> > * -EAGAIN: stay on the from list
> > * -ENOMEM: stay on the from list
> > * Other errno: put on ret_pages list then splice to from list
>
> Can you put this before the switch case in the migrate_pages? That will
> be very helpful to understand the code.
Sure, I agree the switch case deserves some comments.
> >
> > The from list would be empty iff all pages are migrated successfully, it
>
> s/iff/if unless you really mean if and only if. :)
Yes, I mean if and only if.
>
>
> Everything else looks good to me. Thanks for making the code cleaner.
> With the changes above, you can add Reviewed-by: Zi Yan .
Thanks.
>
> > was not so before. This has no impact to current existing callsites.
> >
> > Signed-off-by: Yang Shi
> > ---
> > mm/migrate.c | 58 ++--
> > 1 file changed, 29 insertions(+), 29 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 8a2e7e19e27b..c33c92495ead 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1169,7 +1169,8 @@ static int unmap_and_move(new_page_t get_new_page,
> > free_page_t put_new_page,
> > unsigned long private, struct page *page,
> > int force, enum migrate_mode mode,
> > -enum migrate_reason reason)
> > +enum migrate_reason reason,
> > +struct list_head *ret)
> > {
> > int rc = MIGRATEPAGE_SUCCESS;
> > struct page *newpage = NULL;
> > @@ -1206,7 +1207,14 @@ static int unmap_and_move(new_page_t get_new_page,
> >* migrated will have kept its references and be restored.
> >*/
> > list_del(&page->lru);
> > + }
> >
> > + /*
> > + * If migration is successful, releases reference grabbed during
> > + * isolation. Otherwise, restore the page to right list unless
> > + * we want to retry.
> > + */
> > + if (rc == MIGRATEPAGE_SUCCESS) {
> > /*
> >* Compaction can migrate also non-LRU pages which are
> >* not accounted to NR_ISOLATED_*. They can be recognized
> > @@ -1215,35 +1223,16 @@ static int unmap_and_move(new_page_t get_new_page,
> > if (likely(!__PageMovable(page)))
> > mod_node_page_state(page_pgdat(page),
> > NR_ISOLATED_ANON +
> > page_is_file_lru(page),
> > -thp_nr_pages(page));
> > - }
> >
> > - /*
> > - * If migration is successful, releases reference grabbed during
> > - * isolation. Otherwise, restore the page to right list unless
> > - * we want to retry.
> > - */
> > - if (rc == MIGRATEPAGE_SUCCESS) {
> > if (reason != MR_MEMORY_FAILURE)
> > /*
> >* We release the page in page_handle_poison.
> >*/
> > put_page(page);
> > } else {
> > - if (rc != -EAGAIN) {
> > - if (likely(!__PageMovable(page))) {
> > - putback_lru_page(page);
> > - goto put_new;
> > - }
> > + if (rc != -EAGAIN)
> > + list_add_tail(&page->lru, ret);
> >
> > - lock_page(page);
> > - if (PageMovable(page))
> > - putback_movable_page(page);
> > - else
> > - __ClearPageIsolated(page);
> > - unlock_page(page);
> > - put_page(page);
> > - }
> > -put_new:
> > if (put_new_page)
> > put_new_page(newpage, private);
> > else
> > @@ -1274,7 +1263,8 @@ static int unmap_and_move(new_page_t get_new_page,
> > static int unmap_and_move_huge_page(new_page_t get_new_page,
> > free_page_t put_new_page, unsigned long
> > private,
> >
Re: [PATCH 2/5] mm: migrate: simplify the logic for handling permanent failure
On 3 Nov 2020, at 8:03, Yang Shi wrote:
> When unmap_and_move{_huge_page}() returns !-EAGAIN and !MIGRATEPAGE_SUCCESS,
> the page would be put back to LRU or proper list if it is non-LRU movable
> page. But, the callers always call putback_movable_pages() to put the
> failed pages back later on, so it seems not very efficient to put every
> single page back immediately, and the code looks convoluted.
>
> Put the failed page on a separate list, then splice the list to migrate
> list when all pages are tried. It is the caller's responsibility to
> call putback_movable_pages() to handle failures. This also makes the
> code simpler and more readable.
>
> After the change the rules are:
> * Success: non hugetlb page will be freed, hugetlb page will be put
>back
> * -EAGAIN: stay on the from list
> * -ENOMEM: stay on the from list
> * Other errno: put on ret_pages list then splice to from list
Can you put this before the switch case in the migrate_pages? That will
be very helpful to understand the code.
>
> The from list would be empty iff all pages are migrated successfully, it
s/iff/if unless you really mean if and only if. :)
Everything else looks good to me. Thanks for making the code cleaner.
With the changes above, you can add Reviewed-by: Zi Yan .
> was not so before. This has no impact to current existing callsites.
>
> Signed-off-by: Yang Shi
> ---
> mm/migrate.c | 58 ++--
> 1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8a2e7e19e27b..c33c92495ead 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1169,7 +1169,8 @@ static int unmap_and_move(new_page_t get_new_page,
> free_page_t put_new_page,
> unsigned long private, struct page *page,
> int force, enum migrate_mode mode,
> -enum migrate_reason reason)
> +enum migrate_reason reason,
> +struct list_head *ret)
> {
> int rc = MIGRATEPAGE_SUCCESS;
> struct page *newpage = NULL;
> @@ -1206,7 +1207,14 @@ static int unmap_and_move(new_page_t get_new_page,
>* migrated will have kept its references and be restored.
>*/
> list_del(&page->lru);
> + }
>
> + /*
> + * If migration is successful, releases reference grabbed during
> + * isolation. Otherwise, restore the page to right list unless
> + * we want to retry.
> + */
> + if (rc == MIGRATEPAGE_SUCCESS) {
> /*
>* Compaction can migrate also non-LRU pages which are
>* not accounted to NR_ISOLATED_*. They can be recognized
> @@ -1215,35 +1223,16 @@ static int unmap_and_move(new_page_t get_new_page,
> if (likely(!__PageMovable(page)))
> mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON +
> page_is_file_lru(page),
> -thp_nr_pages(page));
> - }
>
> - /*
> - * If migration is successful, releases reference grabbed during
> - * isolation. Otherwise, restore the page to right list unless
> - * we want to retry.
> - */
> - if (rc == MIGRATEPAGE_SUCCESS) {
> if (reason != MR_MEMORY_FAILURE)
> /*
>* We release the page in page_handle_poison.
>*/
> put_page(page);
> } else {
> - if (rc != -EAGAIN) {
> - if (likely(!__PageMovable(page))) {
> - putback_lru_page(page);
> - goto put_new;
> - }
> + if (rc != -EAGAIN)
> + list_add_tail(&page->lru, ret);
>
> - lock_page(page);
> - if (PageMovable(page))
> - putback_movable_page(page);
> - else
> - __ClearPageIsolated(page);
> - unlock_page(page);
> - put_page(page);
> - }
> -put_new:
> if (put_new_page)
> put_new_page(newpage, private);
> else
> @@ -1274,7 +1263,8 @@ static int unmap_and_move(new_page_t get_new_page,
> static int unmap_and_move_huge_page(new_page_t get_new_page,
> free_page_t put_new_page, unsigned long private,
> struct page *hpage, int force,
> - enum migrate_mode mode, int reason)
> + enum migrate_mode mode, int reason,
> + struct list_head *ret)
> {
> int rc = -EAGAIN;
> int page_was_mapped = 0;
> @@ -1290,7 +1280,7 @@ static int unmap_and_move_huge_page(new_
