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

[Matthew Wilcox]

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 ...