Hi,
This patch set did some cleanup and improvements for the page migration,
please help to review. Thanks a lot.
Note: the patch set is against 20210804 linux-next.
Baolin Wang (5):
mm: migrate: Move the page count validation to the proper place
mm: migrate: Introduce a local variable to get the number of pages
mm: migrate: Fix the incorrect function name in comments
mm: migrate: Change to use bool type for 'page_was_mapped'
mm: migrate: Remove redundant goto labels
mm/migrate.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
--
1.8.3.1
Use thp_nr_pages() instead of compound_nr() to get the number of pages
for THP page, meanwhile introducing a local variable 'nr_pages' to
avoid getting the number of pages repeatedly.
Signed-off-by: Baolin Wang <[email protected]>
---
mm/migrate.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 5559571..eeba4c6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2106,6 +2106,7 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
{
int page_lru;
+ int nr_pages = thp_nr_pages(page);
VM_BUG_ON_PAGE(compound_order(page) && !PageTransHuge(page), page);
@@ -2114,7 +2115,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
return 0;
/* Avoid migrating to a node that is nearly full */
- if (!migrate_balanced_pgdat(pgdat, compound_nr(page)))
+ if (!migrate_balanced_pgdat(pgdat, nr_pages))
return 0;
if (isolate_lru_page(page))
@@ -2122,7 +2123,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
page_lru = page_is_file_lru(page);
mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_lru,
- thp_nr_pages(page));
+ nr_pages);
/*
* Isolating the page has taken another reference, so the
--
1.8.3.1
Remove redundant goto labels to simplify the code.
Signed-off-by: Baolin Wang <[email protected]>
---
mm/migrate.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 0ab364f..ed74fda 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
*/
VM_BUG_ON_PAGE(!PageIsolated(page), page);
if (!PageMovable(page)) {
- rc = MIGRATEPAGE_SUCCESS;
__ClearPageIsolated(page);
- goto out;
+ return MIGRATEPAGE_SUCCESS;
}
rc = mapping->a_ops->migratepage(mapping, newpage,
@@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
flush_dcache_page(newpage);
}
-out:
+
return rc;
}
@@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
HPAGE_PMD_ORDER);
if (!newpage)
- goto out;
+ return NULL;
prep_transhuge_page(newpage);
-out:
return newpage;
}
--
1.8.3.1
Change to use bool type for 'page_was_mapped' variable making it
more readable.
Signed-off-by: Baolin Wang <[email protected]>
---
mm/migrate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 6f048a8..0ab364f 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -957,7 +957,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
int force, enum migrate_mode mode)
{
int rc = -EAGAIN;
- int page_was_mapped = 0;
+ bool page_was_mapped = false;
struct anon_vma *anon_vma = NULL;
bool is_lru = !__PageMovable(page);
@@ -1060,7 +1060,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
VM_BUG_ON_PAGE(PageAnon(page) && !PageKsm(page) && !anon_vma,
page);
try_to_migrate(page, 0);
- page_was_mapped = 1;
+ page_was_mapped = true;
}
if (!page_mapped(page))
--
1.8.3.1
since commit a98a2f0c8ce1 ("mm/rmap: split migration into its own function"),
the migration ptes establishment has been split into a separate
try_to_migrate() function, thus update the related comments.
Signed-off-by: Baolin Wang <[email protected]>
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index eeba4c6..6f048a8 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1005,7 +1005,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
}
/*
- * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
+ * By try_to_migrate(), page->mapcount goes down to 0 here. In this case,
* we cannot notice that anon_vma is freed while we migrates a page.
* This get_anon_vma() delays freeing anon_vma pointer until the end
* of migration. File cache pages are no problem because of page_lock()
--
1.8.3.1
We've got the expected count for anonymous page or file page by
expected_page_refs() at the beginning of migrate_page_move_mapping(),
thus we should move the page count validation a little forward to
reduce duplicated code.
Signed-off-by: Baolin Wang <[email protected]>
---
mm/migrate.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 239b238..5559571 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -386,11 +386,10 @@ int folio_migrate_mapping(struct address_space *mapping,
int expected_count = expected_page_refs(mapping, &folio->page) + extra_count;
long nr = folio_nr_pages(folio);
- if (!mapping) {
- /* Anonymous page without mapping */
- if (folio_ref_count(folio) != expected_count)
- return -EAGAIN;
+ if (folio_ref_count(folio) != expected_count)
+ return -EAGAIN;
+ if (!mapping) {
/* No turning back from here */
newfolio->index = folio->index;
newfolio->mapping = folio->mapping;
@@ -404,8 +403,7 @@ int folio_migrate_mapping(struct address_space *mapping,
newzone = folio_zone(newfolio);
xas_lock_irq(&xas);
- if (folio_ref_count(folio) != expected_count ||
- xas_load(&xas) != folio) {
+ if (xas_load(&xas) != folio) {
xas_unlock_irq(&xas);
return -EAGAIN;
}
--
1.8.3.1
On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<[email protected]> wrote:
>
> Remove redundant goto labels to simplify the code.
TBH I don't see too much benefit. The "goto" makes the functions have
a single exit point.
>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> mm/migrate.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 0ab364f..ed74fda 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> */
> VM_BUG_ON_PAGE(!PageIsolated(page), page);
> if (!PageMovable(page)) {
> - rc = MIGRATEPAGE_SUCCESS;
> __ClearPageIsolated(page);
> - goto out;
> + return MIGRATEPAGE_SUCCESS;
> }
>
> rc = mapping->a_ops->migratepage(mapping, newpage,
> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> flush_dcache_page(newpage);
>
> }
> -out:
> +
> return rc;
> }
>
> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
> newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
> HPAGE_PMD_ORDER);
> if (!newpage)
> - goto out;
> + return NULL;
>
> prep_transhuge_page(newpage);
>
> -out:
> return newpage;
> }
>
> --
> 1.8.3.1
>
>
On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<[email protected]> wrote:
>
> since commit a98a2f0c8ce1 ("mm/rmap: split migration into its own function"),
> the migration ptes establishment has been split into a separate
> try_to_migrate() function, thus update the related comments.
>
> Signed-off-by: Baolin Wang <[email protected]>
Reviewed-by: Yang Shi <[email protected]>
> ---
> mm/migrate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index eeba4c6..6f048a8 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1005,7 +1005,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> }
>
> /*
> - * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
> + * By try_to_migrate(), page->mapcount goes down to 0 here. In this case,
> * we cannot notice that anon_vma is freed while we migrates a page.
> * This get_anon_vma() delays freeing anon_vma pointer until the end
> * of migration. File cache pages are no problem because of page_lock()
> --
> 1.8.3.1
>
>
On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<[email protected]> wrote:
>
> Change to use bool type for 'page_was_mapped' variable making it
> more readable.
Seems better to me. Reviewed-by: Yang Shi <[email protected]>
>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> mm/migrate.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 6f048a8..0ab364f 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -957,7 +957,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> int force, enum migrate_mode mode)
> {
> int rc = -EAGAIN;
> - int page_was_mapped = 0;
> + bool page_was_mapped = false;
> struct anon_vma *anon_vma = NULL;
> bool is_lru = !__PageMovable(page);
>
> @@ -1060,7 +1060,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> VM_BUG_ON_PAGE(PageAnon(page) && !PageKsm(page) && !anon_vma,
> page);
> try_to_migrate(page, 0);
> - page_was_mapped = 1;
> + page_was_mapped = true;
> }
>
> if (!page_mapped(page))
> --
> 1.8.3.1
>
>
On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<[email protected]> wrote:
>
> Use thp_nr_pages() instead of compound_nr() to get the number of pages
> for THP page, meanwhile introducing a local variable 'nr_pages' to
> avoid getting the number of pages repeatedly.
>
> Signed-off-by: Baolin Wang <[email protected]>
Reviewed-by: Yang Shi <[email protected]>
> ---
> mm/migrate.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5559571..eeba4c6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2106,6 +2106,7 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
> static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
> {
> int page_lru;
> + int nr_pages = thp_nr_pages(page);
>
> VM_BUG_ON_PAGE(compound_order(page) && !PageTransHuge(page), page);
>
> @@ -2114,7 +2115,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
> return 0;
>
> /* Avoid migrating to a node that is nearly full */
> - if (!migrate_balanced_pgdat(pgdat, compound_nr(page)))
> + if (!migrate_balanced_pgdat(pgdat, nr_pages))
> return 0;
>
> if (isolate_lru_page(page))
> @@ -2122,7 +2123,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>
> page_lru = page_is_file_lru(page);
> mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_lru,
> - thp_nr_pages(page));
> + nr_pages);
>
> /*
> * Isolating the page has taken another reference, so the
> --
> 1.8.3.1
>
>
Hi Yang,
> On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
> <[email protected]> wrote:
>>
>> Remove redundant goto labels to simplify the code.
>
> TBH I don't see too much benefit. The "goto" makes the functions have
> a single exit point.
Yes, I agree that the 'goto' statement can make things easier when a
function exits from multiple locations and some common work such as
cleanup has to be done, as well as introducing complexity to reading the
code. So per the coding style documentation, "If there is no cleanup
needed then just return directly", which can make code more readable I
think :)
But I have no strong opinion on this, I can drop this patch if you still
think this is unnecessary. Thanks for your review and comments.
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> mm/migrate.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 0ab364f..ed74fda 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>> */
>> VM_BUG_ON_PAGE(!PageIsolated(page), page);
>> if (!PageMovable(page)) {
>> - rc = MIGRATEPAGE_SUCCESS;
>> __ClearPageIsolated(page);
>> - goto out;
>> + return MIGRATEPAGE_SUCCESS;
>> }
>>
>> rc = mapping->a_ops->migratepage(mapping, newpage,
>> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>> flush_dcache_page(newpage);
>>
>> }
>> -out:
>> +
>> return rc;
>> }
>>
>> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
>> newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
>> HPAGE_PMD_ORDER);
>> if (!newpage)
>> - goto out;
>> + return NULL;
>>
>> prep_transhuge_page(newpage);
>>
>> -out:
>> return newpage;
>> }
>>
>> --
>> 1.8.3.1
>>
>>
On Thu, Aug 5, 2021 at 8:19 PM Baolin Wang
<[email protected]> wrote:
>
> Hi Yang,
>
> > On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
> > <[email protected]> wrote:
> >>
> >> Remove redundant goto labels to simplify the code.
> >
> > TBH I don't see too much benefit. The "goto" makes the functions have
> > a single exit point.
>
> Yes, I agree that the 'goto' statement can make things easier when a
> function exits from multiple locations and some common work such as
> cleanup has to be done, as well as introducing complexity to reading the
> code. So per the coding style documentation, "If there is no cleanup
> needed then just return directly", which can make code more readable I
> think :)
>
> But I have no strong opinion on this, I can drop this patch if you still
> think this is unnecessary. Thanks for your review and comments.
Thanks, IMHO I'd like to drop it for now.
>
> >> Signed-off-by: Baolin Wang <[email protected]>
> >> ---
> >> mm/migrate.c | 8 +++-----
> >> 1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index 0ab364f..ed74fda 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> >> */
> >> VM_BUG_ON_PAGE(!PageIsolated(page), page);
> >> if (!PageMovable(page)) {
> >> - rc = MIGRATEPAGE_SUCCESS;
> >> __ClearPageIsolated(page);
> >> - goto out;
> >> + return MIGRATEPAGE_SUCCESS;
> >> }
> >>
> >> rc = mapping->a_ops->migratepage(mapping, newpage,
> >> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> >> flush_dcache_page(newpage);
> >>
> >> }
> >> -out:
> >> +
> >> return rc;
> >> }
> >>
> >> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
> >> newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
> >> HPAGE_PMD_ORDER);
> >> if (!newpage)
> >> - goto out;
> >> + return NULL;
> >>
> >> prep_transhuge_page(newpage);
> >>
> >> -out:
> >> return newpage;
> >> }
> >>
> >> --
> >> 1.8.3.1
> >>
> >>
> On Thu, Aug 5, 2021 at 8:19 PM Baolin Wang
> <[email protected]> wrote:
>>
>> Hi Yang,
>>
>>> On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
>>> <[email protected]> wrote:
>>>>
>>>> Remove redundant goto labels to simplify the code.
>>>
>>> TBH I don't see too much benefit. The "goto" makes the functions have
>>> a single exit point.
>>
>> Yes, I agree that the 'goto' statement can make things easier when a
>> function exits from multiple locations and some common work such as
>> cleanup has to be done, as well as introducing complexity to reading the
>> code. So per the coding style documentation, "If there is no cleanup
>> needed then just return directly", which can make code more readable I
>> think :)
>>
>> But I have no strong opinion on this, I can drop this patch if you still
>> think this is unnecessary. Thanks for your review and comments.
>
> Thanks, IMHO I'd like to drop it for now.
OK, will do. Thanks.
Thanks for catching that.
Reviewed-by: Alistair Popple <[email protected]>
On Friday, 6 August 2021 1:05:58 AM AEST Baolin Wang wrote:
> since commit a98a2f0c8ce1 ("mm/rmap: split migration into its own function"),
> the migration ptes establishment has been split into a separate
> try_to_migrate() function, thus update the related comments.
>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> mm/migrate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index eeba4c6..6f048a8 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1005,7 +1005,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> }
>
> /*
> - * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
> + * By try_to_migrate(), page->mapcount goes down to 0 here. In this case,
> * we cannot notice that anon_vma is freed while we migrates a page.
> * This get_anon_vma() delays freeing anon_vma pointer until the end
> * of migration. File cache pages are no problem because of page_lock()
>