Skip to contentSkip to conversation

mm/page_alloc.c: cleanup on check page

PATCH Superseded Wei Yang 2 participants 10 messages Jan 19, 2020 – Jan 20, 2020

Subsystems (2)
Memory management Page allocator

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Wei Yang

author
#0 lore raw

cover mm/page_alloc.c: cleanup on check page

The patch set does some cleanup related to check page:

* extract the common part to do check page
* rename two functions for consistent name convention
* remove two unnecessary variable assignments

No functional change.

Wei Yang (4):
mm/page_alloc.c: extract commom part to check page mm/page_alloc.c: rename free_pages_check_bad() to
check_free_page_bad()
mm/page_alloc.c: rename free_pages_check() to check_free_page() mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison

 mm/page_alloc.c | 50 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/4 mm/page_alloc.c: extract commom part to check page

During free and new page, we did some check on the status of page struct.

There is some common part, just extract them.

Signed-off-by Wei Yang <[email protected]>
---
 mm/page_alloc.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)
mm/page_alloc.c +16 -20
index d047bf7d8fd4..8cd06729169f 100644
@@ -1025,13 +1025,9 @@ static inline bool page_expected_state(struct page *page,
 	return true;
 }
 
-static void free_pages_check_bad(struct page *page)
+static inline const char *__check_page(struct page *page)
 {
-	const char *bad_reason;
-	unsigned long bad_flags;
-
-	bad_reason = NULL;
-	bad_flags = 0;
+	const char *bad_reason = NULL;
 
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
 		bad_reason = "nonzero mapcount";

@@ -1039,14 +1035,23 @@ static void free_pages_check_bad(struct page *page)
 		bad_reason = "non-NULL mapping";
 	if (unlikely(page_ref_count(page) != 0))
 		bad_reason = "nonzero _refcount";
-	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
-		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
-		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
-	}
 #ifdef CONFIG_MEMCG
 	if (unlikely(page->mem_cgroup))
 		bad_reason = "page still charged to cgroup";
 #endif
+	return bad_reason;
+}
+
+static void free_pages_check_bad(struct page *page)
+{
+	const char *bad_reason = NULL;
+	unsigned long bad_flags = 0;
+
+	bad_reason = __check_page(page);
+	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
+		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
+		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
+	}
 	bad_page(page, bad_reason, bad_flags);
 }
 

@@ -2044,12 +2049,7 @@ static void check_new_page_bad(struct page *page)
 	const char *bad_reason = NULL;
 	unsigned long bad_flags = 0;
 
-	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason = "nonzero mapcount";
-	if (unlikely(page->mapping != NULL))
-		bad_reason = "non-NULL mapping";
-	if (unlikely(page_ref_count(page) != 0))
-		bad_reason = "nonzero _refcount";
+	bad_reason = __check_page(page);
 	if (unlikely(page->flags & __PG_HWPOISON)) {
 		bad_reason = "HWPoisoned (hardware-corrupted)";
 		bad_flags = __PG_HWPOISON;

@@ -2061,10 +2061,6 @@ static void check_new_page_bad(struct page *page)
 		bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set";
 		bad_flags = PAGE_FLAGS_CHECK_AT_PREP;
 	}
-#ifdef CONFIG_MEMCG
-	if (unlikely(page->mem_cgroup))
-		bad_reason = "page still charged to cgroup";
-#endif
 	bad_page(page, bad_reason, bad_flags);
 }
Signature · 1 line
2.17.1
#2 lore raw

Re: [PATCH 1/4] mm/page_alloc.c: extract commom part to check page

Quoted 63 lines · Wei Yang
mm/page_alloc.c +16 -16
index d047bf7d8fd4..8cd06729169f 100644
@@ -1025,13 +1025,9 @@ static inline bool page_expected_state(struct page *page,
 	return true;
 }
 
-static void free_pages_check_bad(struct page *page)
+static inline const char *__check_page(struct page *page)
 {
-	const char *bad_reason;
-	unsigned long bad_flags;
-
-	bad_reason = NULL;
-	bad_flags = 0;
+	const char *bad_reason = NULL;
 
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
 		bad_reason = "nonzero mapcount";

@@ -1039,14 +1035,23 @@ static void free_pages_check_bad(struct page *page)
 		bad_reason = "non-NULL mapping";
 	if (unlikely(page_ref_count(page) != 0))
 		bad_reason = "nonzero _refcount";
-	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
-		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
-		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
-	}
 #ifdef CONFIG_MEMCG
 	if (unlikely(page->mem_cgroup))
 		bad_reason = "page still charged to cgroup";
 #endif
+	return bad_reason;
+}
+
+static void free_pages_check_bad(struct page *page)
+{
+	const char *bad_reason = NULL;
+	unsigned long bad_flags = 0;
+
+	bad_reason = __check_page(page);
+	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
+		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
+		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
+	}
 	bad_page(page, bad_reason, bad_flags);
 }
 

@@ -2044,12 +2049,7 @@ static void check_new_page_bad(struct page *page)
 	const char *bad_reason = NULL;
 	unsigned long bad_flags = 0;
 
-	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason = "nonzero mapcount";
-	if (unlikely(page->mapping != NULL))
-		bad_reason = "non-NULL mapping";
-	if (unlikely(page_ref_count(page) != 0))
-		bad_reason = "nonzero _refcount";
+	bad_reason = __check_page(page);
 	if (unlikely(page->flags & __PG_HWPOISON)) {
 		bad_reason = "HWPoisoned (hardware-corrupted)";
 		bad_flags = __PG_HWPOISON;

mm/page_alloc.c static void check_new_page_bad(struct page *page) · line 2066

Quoted 10 lines
mm/page_alloc.c +0 -4
@@ -2061,10 +2061,6 @@ static void check_new_page_bad(struct page *page)
 		bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set";
 		bad_flags = PAGE_FLAGS_CHECK_AT_PREP;
 	}
-#ifdef CONFIG_MEMCG
-	if (unlikely(page->mem_cgroup))
-		bad_reason = "page still charged to cgroup";
-#endif
 	bad_page(page, bad_reason, bad_flags);
 }

I think this is compounding a previous problem in these functions: these are all "if" clauses, not "else if" clauses so they are presumably ordered based on least significant to most significant (we only see the last bad_reason that we find). For the page->mem_cgroup check, this leaves bad_flags set but it doesn't match bad_reason.

Could you instead fix the problem with these functions so that we actually list *all* the problems with the page rather than only the last conditional that is true?

author
#3 lore raw

Re: [PATCH 1/4] mm/page_alloc.c: extract commom part to check page

Quoted 73 lines · David Rientjes
Quoted 71 lines · Wei Yang
mm/page_alloc.c +16 -20
index d047bf7d8fd4..8cd06729169f 100644
@@ -1025,13 +1025,9 @@ static inline bool page_expected_state(struct page *page,
 	return true;
 }
 
-static void free_pages_check_bad(struct page *page)
+static inline const char *__check_page(struct page *page)
 {
-	const char *bad_reason;
-	unsigned long bad_flags;
-
-	bad_reason = NULL;
-	bad_flags = 0;
+	const char *bad_reason = NULL;
 
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
 		bad_reason = "nonzero mapcount";

@@ -1039,14 +1035,23 @@ static void free_pages_check_bad(struct page *page)
 		bad_reason = "non-NULL mapping";
 	if (unlikely(page_ref_count(page) != 0))
 		bad_reason = "nonzero _refcount";
-	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
-		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
-		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
-	}
 #ifdef CONFIG_MEMCG
 	if (unlikely(page->mem_cgroup))
 		bad_reason = "page still charged to cgroup";
 #endif
+	return bad_reason;
+}
+
+static void free_pages_check_bad(struct page *page)
+{
+	const char *bad_reason = NULL;
+	unsigned long bad_flags = 0;
+
+	bad_reason = __check_page(page);
+	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
+		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
+		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
+	}
 	bad_page(page, bad_reason, bad_flags);
 }
 

@@ -2044,12 +2049,7 @@ static void check_new_page_bad(struct page *page)
 	const char *bad_reason = NULL;
 	unsigned long bad_flags = 0;
 
-	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason = "nonzero mapcount";
-	if (unlikely(page->mapping != NULL))
-		bad_reason = "non-NULL mapping";
-	if (unlikely(page_ref_count(page) != 0))
-		bad_reason = "nonzero _refcount";
+	bad_reason = __check_page(page);
 	if (unlikely(page->flags & __PG_HWPOISON)) {
 		bad_reason = "HWPoisoned (hardware-corrupted)";
 		bad_flags = __PG_HWPOISON;

@@ -2061,10 +2061,6 @@ static void check_new_page_bad(struct page *page)
 		bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set";
 		bad_flags = PAGE_FLAGS_CHECK_AT_PREP;
 	}
-#ifdef CONFIG_MEMCG
-	if (unlikely(page->mem_cgroup))
-		bad_reason = "page still charged to cgroup";
-#endif
 	bad_page(page, bad_reason, bad_flags);
Quoted 9 lines

mm/page_alloc.c static void check_new_page_bad(struct page *page) · line 2066

Quoted 2 lines
mm/page_alloc.c +0 -0
 }

I think this is compounding a previous problem in these functions: these are all "if" clauses, not "else if" clauses so they are presumably ordered based on least significant to most significant (we only see the last bad_reason that we find). For the page->mem_cgroup check, this leaves bad_flags set but it doesn't match bad_reason.

I have thought about this. And curious about the order of those reasons.

Quoted 3 lines

Could you instead fix the problem with these functions so that we actually list *all* the problems with the page rather than only the last conditional that is true?

Sure, thanks for the suggestion.

Signature · 2 lines
Wei Yang
Help you, Help me
author
#4 lore raw

PATCH 2/4 mm/page_alloc.c: rename free_pages_check_bad() to check_free_page_bad()

In reply to Wei Yang

free_pages_check_bad() is the counterpart of check_new_page_bad(), while their naming convention is a little different.

Use verb at first and singular form.

Signed-off-by Wei Yang <[email protected]>
---
 mm/page_alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mm/page_alloc.c +2 -2
index 8cd06729169f..e2324df1b261 100644
@@ -1042,7 +1042,7 @@ static inline const char *__check_page(struct page *page)
 	return bad_reason;
 }
 
-static void free_pages_check_bad(struct page *page)
+static void check_free_page_bad(struct page *page)
 {
 	const char *bad_reason = NULL;
 	unsigned long bad_flags = 0;

@@ -1061,7 +1061,7 @@ static inline int free_pages_check(struct page *page)
 		return 0;
 
 	/* Something has gone sideways, find it */
-	free_pages_check_bad(page);
+	check_free_page_bad(page);
 	return 1;
 }
Signature · 1 line
2.17.1
#5 lore raw

Re: [PATCH 2/4] mm/page_alloc.c: rename free_pages_check_bad() to check_free_page_bad()

Quoted 5 lines · Wei Yang

free_pages_check_bad() is the counterpart of check_new_page_bad(), while their naming convention is a little different.

Use verb at first and singular form.

I think if you agree with the suggestion in patch 1/4 to fix the issue with bad page reporting that it would likely be better to fold patches 2 and 3 into that change.

author
#6 lore raw

Re: [PATCH 2/4] mm/page_alloc.c: rename free_pages_check_bad() to check_free_page_bad()

Quoted 11 lines · David Rientjes
Quoted 5 lines · Wei Yang

free_pages_check_bad() is the counterpart of check_new_page_bad(), while their naming convention is a little different.

Use verb at first and singular form.

I think if you agree with the suggestion in patch 1/4 to fix the issue with bad page reporting that it would likely be better to fold patches 2 and 3 into that change.

I am ok with this, while would it be confusing for review?

Signature · 2 lines
Wei Yang
Help you, Help me
author
#7 lore raw

PATCH 3/4 mm/page_alloc.c: rename free_pages_check() to check_free_page()

In reply to Wei Yang

free_pages_check() is the counterpart of check_new_page(), while their naming convention is a little different.

Use verb at first and singular form.

Signed-off-by Wei Yang <[email protected]>
---
 mm/page_alloc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
mm/page_alloc.c +5 -5
index e2324df1b261..87658a16af07 100644
@@ -1055,7 +1055,7 @@ static void check_free_page_bad(struct page *page)
 	bad_page(page, bad_reason, bad_flags);
 }
 
-static inline int free_pages_check(struct page *page)
+static inline int check_free_page(struct page *page)
 {
 	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE)))
 		return 0;

@@ -1147,7 +1147,7 @@ static __always_inline bool free_pages_prepare(struct page *page,
 		for (i = 1; i < (1 << order); i++) {
 			if (compound)
 				bad += free_tail_pages_check(page, page + i);
-			if (unlikely(free_pages_check(page + i))) {
+			if (unlikely(check_free_page(page + i))) {
 				bad++;
 				continue;
 			}

@@ -1159,7 +1159,7 @@ static __always_inline bool free_pages_prepare(struct page *page,
 	if (memcg_kmem_enabled() && PageKmemcg(page))
 		__memcg_kmem_uncharge(page, order);
 	if (check_free)
-		bad += free_pages_check(page);
+		bad += check_free_page(page);
 	if (bad)
 		return false;
 

@@ -1206,7 +1206,7 @@ static bool free_pcp_prepare(struct page *page)
 static bool bulkfree_pcp_prepare(struct page *page)
 {
 	if (debug_pagealloc_enabled_static())
-		return free_pages_check(page);
+		return check_free_page(page);
 	else
 		return false;
 }

@@ -1227,7 +1227,7 @@ static bool free_pcp_prepare(struct page *page)
 
 static bool bulkfree_pcp_prepare(struct page *page)
 {
-	return free_pages_check(page);
+	return check_free_page(page);
 }
 #endif /* CONFIG_DEBUG_VM */
Signature · 1 line
2.17.1
author
#8 lore raw

PATCH 4/4 mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison

In reply to Wei Yang

Since function returns directly, bad_[reason|flags] is not used any where.

This is a following cleanup for commit e570f56cccd21 ("mm:
check_new_page_bad() directly returns in __PG_HWPOISON case")

Signed-off-by Wei Yang <[email protected]>
---
 mm/page_alloc.c | 2 --
 1 file changed, 2 deletions(-)
mm/page_alloc.c +0 -2
index 87658a16af07..2b5e2e156dbf 100644
@@ -2051,8 +2051,6 @@ static void check_new_page_bad(struct page *page)
 
 	bad_reason = __check_page(page);
 	if (unlikely(page->flags & __PG_HWPOISON)) {
-		bad_reason = "HWPoisoned (hardware-corrupted)";
-		bad_flags = __PG_HWPOISON;
 		/* Don't complain about hwpoisoned pages */
 		page_mapcount_reset(page); /* remove PageBuddy */
 		return;
Signature · 1 line
2.17.1
Acked-by on v1 4/4
#9 lore raw

Re: [PATCH 4/4] mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison

Quoted 7 lines · Wei Yang

Since function returns directly, bad_[reason|flags] is not used any where.

This is a following cleanup for commit e570f56cccd21 ("mm:
check_new_page_bad() directly returns in __PG_HWPOISON case")

Signed-off-by Wei Yang <[email protected]>
Acked-by David Rientjes <[email protected]>