Re: [PATCH] mm: migrate: handle freed page at the first place

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 11/11/19 3:18 PM, Andrew Morton wrote:
On Tue, 12 Nov 2019 06:09:25 +0800 Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx> wrote:

When doing migration if the freed page is met, we just return without
migrating it since it is pointless to migrate a freed page.  But, the
current code did two things before handling freed page:

1. Return -ENOMEM if the page is THP and THP migration is not supported.
2. Allocate target page unconditionally.

Both makes not too much sense.  If we handle freed page at the first place
we don't have to worry about allocating/freeing target page and split
THP at all.

For example (worst case) if we are trying to migrate a freed THP without
THP migration supported, the migrate_pages() would just split the THP then
retry to migrate base pages one by one by pointless allocating and freeing
pages, this is just waste of time.

I didn't run into any actual problem with the current code (or I may
just not notice it yet), it was found by visual inspection.


--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
  	int rc = MIGRATEPAGE_SUCCESS;
  	struct page *newpage;
- if (!thp_migration_supported() && PageTransHuge(page))
-		return -ENOMEM;
-
-	newpage = get_new_page(page, private);
-	if (!newpage)
-		return -ENOMEM;
-
  	if (page_count(page) == 1) {
Is it possible to have (!thp_migration_supported() &&
PageTransHuge(page) && page_count(page) == 1)?  If so, isn't this new
behviour?

IMHO it should be possible on some architectures, i.e. aarch64, with anonymous THP. I just saw PowerPC and x86_64 have CONFIG_ARCH_ENABLE_THP_MIGRATION selected. I'm not quite sure if I miss something.

It should be not new behavior since migrate_pages() should just split the THP then retry with base pages one by one. Even though it returns -EBUSY due to THP split failure in the current code, the behavior sounds problematic. We should not return errno for a freed page, right?


  		/* page was freed from under us. So we are done. */
  		ClearPageActive(page);
@@ -1187,13 +1180,16 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
  				__ClearPageIsolated(page);
  			unlock_page(page);
  		}
-		if (put_new_page)
-			put_new_page(newpage, private);
-		else
-			put_page(newpage);
  		goto out;
  	}
+ if (!thp_migration_supported() && PageTransHuge(page))
+		return -ENOMEM;
+
+	newpage = get_new_page(page, private);
+	if (!newpage)
+		return -ENOMEM;
+
  	rc = __unmap_and_move(page, newpage, force, mode);
  	if (rc == MIGRATEPAGE_SUCCESS)
  		set_page_owner_migrate_reason(newpage, reason);





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux