by Michal Hocko

[permalink] [raw]

Subject: Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> > On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> > [...]
> > > > RIght, I confused the two. What is the proper layer to fix that then?
> > > > rmap_walk_file?
> > >
> > > Maybe something like this? Totally untested.
> >
> > This looks way too complex. Why cannot we simply split THP page cache
> > during migration?
>
> This way we unify the codepath for archictures that don't support THP
> migration and shmem THP.

But why? There shouldn't be really nothing to prevent THP (anon or
shemem) to be migratable. If we cannot migrate it at once we can always
split it. So why should we add another thp specific handling all over
the place?
--
Michal Hocko
SUSE Labs

2018-04-05 18:00:06

[permalink] [raw]

Subject: Re: [PATCH] mm: shmem: enable thp migration (Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page)

On Sun, Apr 22, 2018 at 08:03:49PM -0700, Matthew Wilcox wrote:
> On Fri, Apr 06, 2018 at 03:07:11AM +0000, Naoya Horiguchi wrote:
> > Subject: [PATCH] mm: enable thp migration for shmem thp
>
> This patch is buggy, but not in a significant way:
>
> > @@ -524,13 +524,26 @@ int migrate_page_move_mapping(struct address_space *mapping,
> > }
> >
> > radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
>
> ^^^ this line should have been deleted
>
> > + if (PageTransHuge(page)) {
> > + int i;
> > + int index = page_index(page);
> > +
> > + for (i = 0; i < HPAGE_PMD_NR; i++) {
> ^^^ or this iteration should start at 1
> > + pslot = radix_tree_lookup_slot(&mapping->i_pages,
> > + index + i);
> > + radix_tree_replace_slot(&mapping->i_pages, pslot,
> > + newpage + i);
> > + }
> > + } else {
> > + radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
> ^^^ and if the second option, then we don't need this line
> > + }
>
> So either this:
>
> - radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
> + if (PageTransHuge(page)) {
> + int i;
> + int index = page_index(page);
> +
> + for (i = 0; i < HPAGE_PMD_NR; i++) {
> + pslot = radix_tree_lookup_slot(&mapping->i_pages,
> + index + i);
> + radix_tree_replace_slot(&mapping->i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
> + }
>
> Or this:
>
> radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
> + if (PageTransHuge(page)) {
> + int i;
> + int index = page_index(page);
> +
> + for (i = 1; i < HPAGE_PMD_NR; i++) {
> + pslot = radix_tree_lookup_slot(&mapping->i_pages,
> + index + i);
> + radix_tree_replace_slot(&mapping->i_pages, pslot,
> + newpage + i);
> + }
> + }
>
> The second one is shorter and involves fewer lookups ...

Hi Matthew,

Thank you for poinitng out, I like the second one.
The original patch is now in upsteam, so I wrote a patch on it.

Thanks,
Naoya

--------
From: Naoya Horiguchi <[email protected]>
Date: Sun, 22 Apr 2018 20:03:49 -0700
Subject: [PATCH] mm: migrate: fix double call of radix_tree_replace_slot()

radix_tree_replace_slot() is called twice for head page, it's
obviously a bug. Let's fix it.

Fixes: e71769ae5260 ("mm: enable thp migration for shmem thp")
Reported-by: Matthew Wilcox <[email protected]>
Signed-off-by: Naoya Horiguchi <[email protected]>
---
mm/migrate.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 568433023831..8c0af0f7cab1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -528,14 +528,12 @@ int migrate_page_move_mapping(struct address_space *mapping,
int i;
int index = page_index(page);

- for (i = 0; i < HPAGE_PMD_NR; i++) {
+ for (i = 1; i < HPAGE_PMD_NR; i++) {
pslot = radix_tree_lookup_slot(&mapping->i_pages,
index + i);
radix_tree_replace_slot(&mapping->i_pages, pslot,
newpage + i);
}
- } else {
- radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
}

/*
--
2.7.4