lkml.org 
[lkml]   [2018]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH] mm: shmem: enable thp migration (Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page)
Date
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@ah.jp.nec.com>
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@infradead.org>
Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
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
\
 
 \ /
  Last update: 2018-04-23 09:27    [W:0.140 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site