[1/7] mm,hwpoison: Refactor get_any_page
diff mbox series

Message ID 20201119105716.5962-2-osalvador@suse.de
State In Next
Commit 0ce70d1caa601eb4dee98c56b8158e3c297f5c63
Headers show
Series
  • HWPoison: Refactor get page interface
Related show

Commit Message

Oscar Salvador Nov. 19, 2020, 10:57 a.m. UTC
When we want to grab a refcount via get_any_page, we call
__get_any_page that calls get_hwpoison_page to get the
actual refcount.
get_any_page is only there because we have a sort of retry
mechanism in case the page we met is unknown to us or
if we raced with an allocation.

Also __get_any_page prints some messages about the page type
in case the page was a free page or the page type was unknown,
but if anything, we only need to print a message in case the
pagetype was unknown, as that is reporting an error down the chain.

Let us merge get_any_page and __get_any_page, and let the message
be printed in soft_offline_page.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 mm/memory-failure.c | 91 ++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 54 deletions(-)

Comments

HORIGUCHI NAOYA(堀口 直也) Nov. 20, 2020, 1:33 a.m. UTC | #1
On Thu, Nov 19, 2020 at 11:57:10AM +0100, Oscar Salvador wrote:
> When we want to grab a refcount via get_any_page, we call
> __get_any_page that calls get_hwpoison_page to get the
> actual refcount.
> get_any_page is only there because we have a sort of retry
> mechanism in case the page we met is unknown to us or
> if we raced with an allocation.
> 
> Also __get_any_page prints some messages about the page type
> in case the page was a free page or the page type was unknown,
> but if anything, we only need to print a message in case the
> pagetype was unknown, as that is reporting an error down the chain.
> 
> Let us merge get_any_page and __get_any_page, and let the message
> be printed in soft_offline_page.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>

Looks good to me, thank you.

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Vlastimil Babka Nov. 25, 2020, 4:54 p.m. UTC | #2
On 11/19/20 11:57 AM, Oscar Salvador wrote:
> When we want to grab a refcount via get_any_page, we call
> __get_any_page that calls get_hwpoison_page to get the
> actual refcount.
> get_any_page is only there because we have a sort of retry
> mechanism in case the page we met is unknown to us or
> if we raced with an allocation.
> 
> Also __get_any_page prints some messages about the page type
> in case the page was a free page or the page type was unknown,
> but if anything, we only need to print a message in case the
> pagetype was unknown, as that is reporting an error down the chain.
> 
> Let us merge get_any_page and __get_any_page, and let the message
> be printed in soft_offline_page.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>

Acked-by: Vlastimil Babka <Vbabka@suse.cz>

Patch
diff mbox series

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 869ece2a1de2..0d2323ba4b8e 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1707,70 +1707,49 @@  EXPORT_SYMBOL(unpoison_memory);
 
 /*
  * Safely get reference count of an arbitrary page.
- * Returns 0 for a free page, -EIO for a zero refcount page
- * that is not free, and 1 for any other page type.
- * For 1 the page is returned with increased page count, otherwise not.
+ * Returns 0 for a free page, 1 for an in-use page, -EIO for a page-type we
+ * cannot handle and -EBUSY if we raced with an allocation.
+ * We only incremented refcount in case the page was already in-use and it is
+ * a known type we can handle.
  */
-static int __get_any_page(struct page *p, unsigned long pfn, int flags)
+static int get_any_page(struct page *p, unsigned long pfn, int flags)
 {
-	int ret;
+	int ret = 0, pass = 0;
 
 	if (flags & MF_COUNT_INCREASED)
 		return 1;
 
-	/*
-	 * When the target page is a free hugepage, just remove it
-	 * from free hugepage list.
-	 */
+try_again:
 	if (!get_hwpoison_page(p)) {
-		if (PageHuge(p)) {
-			pr_info("%s: %#lx free huge page\n", __func__, pfn);
-			ret = 0;
-		} else if (is_free_buddy_page(p)) {
-			pr_info("%s: %#lx free buddy page\n", __func__, pfn);
-			ret = 0;
-		} else if (page_count(p)) {
-			/* raced with allocation */
+		if (page_count(p)) {
+			/* We raced with an allocation, retry. */
+			if (pass++ < 3)
+				goto try_again;
 			ret = -EBUSY;
-		} else {
-			pr_info("%s: %#lx: unknown zero refcount page type %lx\n",
-				__func__, pfn, p->flags);
+		} else if (!PageHuge(p) && !is_free_buddy_page(p)) {
+			/* We raced with put_page, retry. */
+			if (pass++ < 3)
+				goto try_again;
 			ret = -EIO;
 		}
 	} else {
-		/* Not a free page */
-		ret = 1;
-	}
-	return ret;
-}
-
-static int get_any_page(struct page *page, unsigned long pfn, int flags)
-{
-	int ret = __get_any_page(page, pfn, flags);
-
-	if (ret == -EBUSY)
-		ret = __get_any_page(page, pfn, flags);
-
-	if (ret == 1 && !PageHuge(page) &&
-	    !PageLRU(page) && !__PageMovable(page)) {
-		/*
-		 * Try to free it.
-		 */
-		put_page(page);
-		shake_page(page, 1);
-
-		/*
-		 * Did it turn free?
-		 */
-		ret = __get_any_page(page, pfn, 0);
-		if (ret == 1 && !PageLRU(page)) {
-			/* Drop page reference which is from __get_any_page() */
-			put_page(page);
-			pr_info("soft_offline: %#lx: unknown non LRU page type %lx (%pGp)\n",
-				pfn, page->flags, &page->flags);
-			return -EIO;
+		if (PageHuge(p) || PageLRU(p) || __PageMovable(p)) {
+			ret = 1;
+		} else {
+			/*
+			 * A page we cannot handle. Check whether we can turn
+			 * it into something we can handle.
+			 */
+			if (pass++ < 3) {
+				put_page(p);
+				shake_page(p, 1);
+				goto try_again;
+			}
+			put_page(p);
+			ret = -EIO;
 		}
 	}
+
 	return ret;
 }
 
@@ -1939,7 +1918,7 @@  int soft_offline_page(unsigned long pfn, int flags)
 		return -EIO;
 
 	if (PageHWPoison(page)) {
-		pr_info("soft offline: %#lx page already poisoned\n", pfn);
+		pr_info("%s: %#lx page already poisoned\n", __func__, pfn);
 		if (flags & MF_COUNT_INCREASED)
 			put_page(page);
 		return 0;
@@ -1950,13 +1929,17 @@  int soft_offline_page(unsigned long pfn, int flags)
 	ret = get_any_page(page, pfn, flags);
 	put_online_mems();
 
-	if (ret > 0)
+	if (ret > 0) {
 		ret = soft_offline_in_use_page(page);
-	else if (ret == 0)
+	} else if (ret == 0) {
 		if (soft_offline_free_page(page) && try_again) {
 			try_again = false;
 			goto retry;
 		}
+	} else if (ret == -EIO) {
+		pr_info("%s: %#lx: unknown page type: %lx (%pGP)\n",
+			 __func__, pfn, page->flags, &page->flags);
+	}
 
 	return ret;
 }