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 <n-horiguchi@xxxxxxxxxxxxx>
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 <willy@xxxxxxxxxxxxx>
Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
---
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
