This patch series converts several functions to use folio in migrate.c.
Kefeng Wang (4):
mm: migrate: use a folio in add_page_for_migration()
mm: migrate: convert numamigrate_isolate_page() to
numamigrate_isolate_folio()
mm: migrate: make migrate_misplaced_page() to take a folio
mm: migrate: use __folio_test_movable()
mm/migrate.c | 101 +++++++++++++++++++++++++--------------------------
1 file changed, 50 insertions(+), 51 deletions(-)
--
2.41.0
Use __folio_test_movable(), no need to convert from folio to page.
Signed-off-by: Kefeng Wang <[email protected]>
---
mm/migrate.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 4be61f944cac..8590e6f68cf3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -157,8 +157,8 @@ void putback_movable_pages(struct list_head *l)
list_del(&folio->lru);
/*
* We isolated non-lru movable folio so here we can use
- * __PageMovable because LRU folio's mapping cannot have
- * PAGE_MAPPING_MOVABLE.
+ * __folio_test_movable because LRU folio's mapping cannot
+ * have PAGE_MAPPING_MOVABLE.
*/
if (unlikely(__folio_test_movable(folio))) {
VM_BUG_ON_FOLIO(!folio_test_isolated(folio), folio);
@@ -943,7 +943,7 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
enum migrate_mode mode)
{
int rc = -EAGAIN;
- bool is_lru = !__PageMovable(&src->page);
+ bool is_lru = !__folio_test_movable(src);
VM_BUG_ON_FOLIO(!folio_test_locked(src), src);
VM_BUG_ON_FOLIO(!folio_test_locked(dst), dst);
@@ -990,7 +990,7 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
* src is freed; but stats require that PageAnon be left as PageAnon.
*/
if (rc == MIGRATEPAGE_SUCCESS) {
- if (__PageMovable(&src->page)) {
+ if (__folio_test_movable(src)) {
VM_BUG_ON_FOLIO(!folio_test_isolated(src), src);
/*
@@ -1082,7 +1082,7 @@ static void migrate_folio_done(struct folio *src,
/*
* Compaction can migrate also non-LRU pages which are
* not accounted to NR_ISOLATED_*. They can be recognized
- * as __PageMovable
+ * as __folio_test_movable
*/
if (likely(!__folio_test_movable(src)))
mod_node_page_state(folio_pgdat(src), NR_ISOLATED_ANON +
@@ -1103,7 +1103,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
int rc = -EAGAIN;
int page_was_mapped = 0;
struct anon_vma *anon_vma = NULL;
- bool is_lru = !__PageMovable(&src->page);
+ bool is_lru = !__folio_test_movable(src);
bool locked = false;
bool dst_locked = false;
@@ -1261,7 +1261,7 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
int rc;
int page_was_mapped = 0;
struct anon_vma *anon_vma = NULL;
- bool is_lru = !__PageMovable(&src->page);
+ bool is_lru = !__folio_test_movable(src);
struct list_head *prev;
__migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
--
2.41.0
Make migrate_misplaced_page() to take a folio and use folio API
to save compound_head() calls.
Signed-off-by: Kefeng Wang <[email protected]>
---
mm/migrate.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 39e0d35bccb3..4be61f944cac 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2525,17 +2525,18 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
int node)
{
pg_data_t *pgdat = NODE_DATA(node);
+ struct folio *folio = page_folio(page);
int isolated;
int nr_remaining;
unsigned int nr_succeeded;
LIST_HEAD(migratepages);
- int nr_pages = thp_nr_pages(page);
+ int nr_pages = folio_nr_pages(folio);
/*
* Don't migrate file pages that are mapped in multiple processes
* with execute permissions as they are probably shared libraries.
*/
- if (page_mapcount(page) != 1 && page_is_file_lru(page) &&
+ if (folio_estimated_sharers(folio) != 1 && folio_is_file_lru(folio) &&
(vma->vm_flags & VM_EXEC))
goto out;
@@ -2543,29 +2544,29 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
* Also do not migrate dirty pages as not all filesystems can move
* dirty pages in MIGRATE_ASYNC mode which is a waste of cycles.
*/
- if (page_is_file_lru(page) && PageDirty(page))
+ if (folio_is_file_lru(folio) && folio_test_dirty(folio))
goto out;
- isolated = numamigrate_isolate_folio(pgdat, page_folio(page));
+ isolated = numamigrate_isolate_folio(pgdat, folio);
if (!isolated)
goto out;
- list_add(&page->lru, &migratepages);
+ list_add(&folio->lru, &migratepages);
nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_folio,
NULL, node, MIGRATE_ASYNC,
MR_NUMA_MISPLACED, &nr_succeeded);
if (nr_remaining) {
if (!list_empty(&migratepages)) {
- list_del(&page->lru);
- mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON +
- page_is_file_lru(page), -nr_pages);
- putback_lru_page(page);
+ list_del(&folio->lru);
+ node_stat_mod_folio(folio, NR_ISOLATED_ANON +
+ folio_is_file_lru(folio), -nr_pages);
+ folio_putback_lru(folio);
}
isolated = 0;
}
if (nr_succeeded) {
count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
- if (!node_is_toptier(page_to_nid(page)) && node_is_toptier(node))
+ if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
nr_succeeded);
}
@@ -2573,7 +2574,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
return isolated;
out:
- put_page(page);
+ folio_put(folio);
return 0;
}
#endif /* CONFIG_NUMA_BALANCING */
--
2.41.0
Rename numamigrate_isolate_page() to numamigrate_isolate_folio(), then
make it takes a folio and use folio API to save compound_head() calls.
Signed-off-by: Kefeng Wang <[email protected]>
---
mm/migrate.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index b0c318bc531c..39e0d35bccb3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2476,15 +2476,15 @@ static struct folio *alloc_misplaced_dst_folio(struct folio *src,
return __folio_alloc_node(gfp, order, nid);
}
-static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
+static int numamigrate_isolate_folio(pg_data_t *pgdat, struct folio *folio)
{
- int nr_pages = thp_nr_pages(page);
- int order = compound_order(page);
+ int nr_pages = folio_nr_pages(folio);
+ int order = folio_order(folio);
- VM_BUG_ON_PAGE(order && !PageTransHuge(page), page);
+ VM_BUG_ON_FOLIO(order && !folio_test_pmd_mappable(folio), folio);
/* Do not migrate THP mapped by multiple processes */
- if (PageTransHuge(page) && total_mapcount(page) > 1)
+ if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) > 1)
return 0;
/* Avoid migrating to a node that is nearly full */
@@ -2501,18 +2501,18 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
return 0;
}
- if (!isolate_lru_page(page))
+ if (!folio_isolate_lru(folio))
return 0;
- mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_is_file_lru(page),
+ node_stat_mod_folio(folio, NR_ISOLATED_ANON + folio_is_file_lru(folio),
nr_pages);
/*
- * Isolating the page has taken another reference, so the
- * caller's reference can be safely dropped without the page
+ * Isolating the folio has taken another reference, so the
+ * caller's reference can be safely dropped without the folio
* disappearing underneath us during migration.
*/
- put_page(page);
+ folio_put(folio);
return 1;
}
@@ -2546,7 +2546,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
if (page_is_file_lru(page) && PageDirty(page))
goto out;
- isolated = numamigrate_isolate_page(pgdat, page);
+ isolated = numamigrate_isolate_folio(pgdat, page_folio(page));
if (!isolated)
goto out;
--
2.41.0
On 02.08.23 11:53, Kefeng Wang wrote:
> This patch series converts several functions to use folio in migrate.c.
Please clearly spell out in the patch descriptions when you are changing
mapcount logic and which effects that might have (both, positive and
negative) for PTE-mapped THP.
--
Cheers,
David / dhildenb
On Wed, Aug 02, 2023 at 05:53:46PM +0800, Kefeng Wang wrote:
> Use __folio_test_movable(), no need to convert from folio to page.
>
> Signed-off-by: Kefeng Wang <[email protected]>
In my defence, I did many of these function conversions before
__folio_test_movable() existed ;-)
Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
On 2023/8/2 19:47, David Hildenbrand wrote:
> On 02.08.23 11:53, Kefeng Wang wrote:
>> This patch series converts several functions to use folio in migrate.c.
>
> Please clearly spell out in the patch descriptions when you are changing
> mapcount logic and which effects that might have (both, positive and
> negative) for PTE-mapped THP.
Oh, I see your comments on another mail[1], the folio_estimated_sharers()
is not good to evaluate the sharing by multiple processes, will read more
history of the mail's discussion, maybe just ignore the first three patches.
Thank.
[1] [PATCH 0/2] don't use mapcount() to check large folio sharing
>
On 02.08.23 11:53, Kefeng Wang wrote:
> Use __folio_test_movable(), no need to convert from folio to page.
>
> Signed-off-by: Kefeng Wang <[email protected]>
> ---
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
On Wed, Aug 02, 2023 at 05:53:44PM +0800, Kefeng Wang wrote:
> -static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
> +static int numamigrate_isolate_folio(pg_data_t *pgdat, struct folio *folio)
> {
> - int nr_pages = thp_nr_pages(page);
> - int order = compound_order(page);
> + int nr_pages = folio_nr_pages(folio);
> + int order = folio_order(folio);
>
> - VM_BUG_ON_PAGE(order && !PageTransHuge(page), page);
> + VM_BUG_ON_FOLIO(order && !folio_test_pmd_mappable(folio), folio);
I don't know why we have this assertion. I would be inclined to delete
it as part of generalising the migration code to handle arbitrary sizes
of folio, rather than assert that we only support PMD size folios.
> /* Do not migrate THP mapped by multiple processes */
> - if (PageTransHuge(page) && total_mapcount(page) > 1)
> + if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) > 1)
> return 0;
I don't know if this is the right logic. We've willing to move folios
mapped by multiple processes, as long as they're smaller than PMD size,
but once they get to PMD size they're magical and can't be moved?
On 2023/8/2 20:30, Matthew Wilcox wrote:
> On Wed, Aug 02, 2023 at 05:53:44PM +0800, Kefeng Wang wrote:
>> -static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>> +static int numamigrate_isolate_folio(pg_data_t *pgdat, struct folio *folio)
>> {
>> - int nr_pages = thp_nr_pages(page);
>> - int order = compound_order(page);
>> + int nr_pages = folio_nr_pages(folio);
>> + int order = folio_order(folio);
>>
>> - VM_BUG_ON_PAGE(order && !PageTransHuge(page), page);
>> + VM_BUG_ON_FOLIO(order && !folio_test_pmd_mappable(folio), folio);
>
> I don't know why we have this assertion. I would be inclined to delete
> it as part of generalising the migration code to handle arbitrary sizes
> of folio, rather than assert that we only support PMD size folios.
Ok, will drop it.
>
>> /* Do not migrate THP mapped by multiple processes */
>> - if (PageTransHuge(page) && total_mapcount(page) > 1)
>> + if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) > 1)
>> return 0;
>
> I don't know if this is the right logic. We've willing to move folios
> mapped by multiple processes, as long as they're smaller than PMD size,
> but once they get to PMD size they're magical and can't be moved?
It seems that the logical is introduced by commit 04fa5d6a6547 ("mm:
migrate: check page_count of THP before migrating") and refactor by
340ef3902cf2 ("mm: numa: cleanup flow of transhuge page migration"),
"Hugh Dickins pointed out that migrate_misplaced_transhuge_page() does
not check page_count before migrating like base page migration and
khugepage. He could not see why this was safe and he is right."
For now, there is no migrate_misplaced_transhuge_page() and base/thp
page migrate's path is unified, there is a check(for old/new kernel) in
migrate_misplaced_page(),
"Don't migrate file pages that are mapped in multiple processes
with execute permissions as they are probably shared libraries."
We could drop the above check in numamigrate_isolate_page(), but
according to 04fa5d6a6547, maybe disable migrate page shared by
multi-process during numa balance for both base/thp page.
>
>
On 02.08.23 14:38, Kefeng Wang wrote:
>
>
> On 2023/8/2 19:47, David Hildenbrand wrote:
>> On 02.08.23 11:53, Kefeng Wang wrote:
>>> This patch series converts several functions to use folio in migrate.c.
>>
>> Please clearly spell out in the patch descriptions when you are changing
>> mapcount logic and which effects that might have (both, positive and
>> negative) for PTE-mapped THP.
>
> Oh, I see your comments on another mail[1], the folio_estimated_sharers()
> is not good to evaluate the sharing by multiple processes, will read more
> history of the mail's discussion, maybe just ignore the first three patches.
It might be good enough for some cases right now. My point is that we
better just clearly spell out the possible effects of such a change.
(so far you didn't even mention them, and that's sub-optimal when buried
in a "folio" conversion that better shouldn't change the functionality
without spelling it out)
--
Cheers,
David / dhildenb
On Thu, 3 Aug 2023, Kefeng Wang wrote:
> On 2023/8/2 20:30, Matthew Wilcox wrote:
> > On Wed, Aug 02, 2023 at 05:53:44PM +0800, Kefeng Wang wrote:
> >> /* Do not migrate THP mapped by multiple processes */
> >> - if (PageTransHuge(page) && total_mapcount(page) > 1)
> >> + if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) >
> >> 1)
> >> return 0;
> >
> > I don't know if this is the right logic. We've willing to move folios
> > mapped by multiple processes, as long as they're smaller than PMD size,
> > but once they get to PMD size they're magical and can't be moved?
>
> It seems that the logical is introduced by commit 04fa5d6a6547 ("mm:
> migrate: check page_count of THP before migrating") and refactor by
> 340ef3902cf2 ("mm: numa: cleanup flow of transhuge page migration"),
>
>
> "Hugh Dickins pointed out that migrate_misplaced_transhuge_page() does
> not check page_count before migrating like base page migration and
> khugepage. He could not see why this was safe and he is right."
>
> For now, there is no migrate_misplaced_transhuge_page() and base/thp
> page migrate's path is unified, there is a check(for old/new kernel) in
> migrate_misplaced_page(),
Right, Mel's comment on safety above comes from a time when
migrate_misplaced_transhuge_page() went its own way, and so did not
reach the careful reference count checking found in (the now named)
folio_migrate_mapping(). If migrate_misplaced_page() is now using
the standard migrate_pages() for small pages and THPs, then there
should no longer be safety concerns.
>
> "Don't migrate file pages that are mapped in multiple processes
> with execute permissions as they are probably shared libraries."
>
> We could drop the above check in numamigrate_isolate_page(), but
> according to 04fa5d6a6547, maybe disable migrate page shared by
> multi-process during numa balance for both base/thp page.
I'm out of touch with the NUMA balancing preferences at present,
but think that you're probably right that it should aim to treat
folio mapped into multiple processes the same way for small and large
and THP (except, of course, that large and THP, with multiple PTEs in
the same process, can present more challenges to how to go about that).
Hugh