[RFC] mm/page_alloc: Enumerate bad page reasons

[RFC] mm/page_alloc: Enumerate bad page reasons

[Anshuman Khandual]

Enumerate all existing bad page reasons which can be used in bad_page() for
reporting via __dump_page(). Unfortunately __dump_page() cannot be changed.
__dump_page() is called from dump_page() that accepts a raw string and is
also an exported symbol that is currently being used from various generic
memory functions and other drivers. This reduces code duplication while
reporting bad pages.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Boot tested on arm64 and x86 platforms (next-20200327) but has been built
tested on many other platforms.

 include/linux/memory.h | 36 ++++++++++++++++++++++++++++++++++++
 mm/page_alloc.c        | 40 ++++++++++++++++++++--------------------
 2 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/include/linux/memory.h b/include/linux/memory.h
index 7914b0dbd4bb..b2fb90fda537 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -82,6 +82,42 @@ struct memory_notify {
 struct notifier_block;
 struct mem_section;
 
+/*
+ * Bad page reasons
+ */
+enum page_bad_reason {
+	PAGE_BAD_NONZERO_MAPCOUNT,
+	PAGE_BAD_NONZERO_REFCOUNT,
+	PAGE_BAD_NONNULL_MAPPING,
+	PAGE_BAD_FREE_FLAGS,
+	PAGE_BAD_PREP_FLAGS,
+	PAGE_BAD_CGROUP_CHARGED,
+	PAGE_BAD_HWPOISON,
+	PAGE_BAD_TAIL_CORRUPTED,
+	PAGE_BAD_TAIL_NOT_SET,
+	PAGE_BAD_COMPOUND_HEAD,
+	PAGE_BAD_COMPOUND_MAPCOUNT,
+};
+
+static const char *const page_bad_names[] = {
+	[PAGE_BAD_NONZERO_MAPCOUNT]  = "non-zero mapcout",
+	[PAGE_BAD_NONZERO_REFCOUNT]  = "non-zero _refcount",
+	[PAGE_BAD_NONNULL_MAPPING]   = "non-NULL mapping",
+	[PAGE_BAD_FREE_FLAGS]        = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set",
+	[PAGE_BAD_PREP_FLAGS]        = "PAGE_FLAGS_CHECK_AT_PREP flag(s) set",
+	[PAGE_BAD_CGROUP_CHARGED]    = "page still charged to cgroup",
+	[PAGE_BAD_HWPOISON]          = "HWPoisoned (hardware-corrupted)",
+	[PAGE_BAD_TAIL_CORRUPTED]    = "corrupted mapping in tail page",
+	[PAGE_BAD_TAIL_NOT_SET]      = "PageTail not set",
+	[PAGE_BAD_COMPOUND_HEAD]     = "compound_head not consistent",
+	[PAGE_BAD_COMPOUND_MAPCOUNT] = "non-zero compound_mapcount",
+};
+
+static inline const char *get_page_bad(int reason)
+{
+	return page_bad_names[reason];
+}
+
 /*
  * Priorities for the hotplug memory callback routines (stored in decreasing
  * order in the callback chain)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ca1453204e66..0f05da0fdc9a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -68,6 +68,7 @@
 #include <linux/lockdep.h>
 #include <linux/nmi.h>
 #include <linux/psi.h>
+#include <linux/memory.h>
 
 #include <asm/sections.h>
 #include <asm/tlbflush.h>
@@ -609,7 +610,7 @@ static inline int __maybe_unused bad_range(struct zone *zone, struct page *page)
 }
 #endif
 
-static void bad_page(struct page *page, const char *reason,
+static void bad_page(struct page *page, int reason,
 		unsigned long bad_flags)
 {
 	static unsigned long resume;
@@ -638,7 +639,7 @@ static void bad_page(struct page *page, const char *reason,
 
 	pr_alert("BUG: Bad page state in process %s  pfn:%05lx\n",
 		current->comm, page_to_pfn(page));
-	__dump_page(page, reason);
+	__dump_page(page, get_page_bad(reason));
 	bad_flags &= page->flags;
 	if (bad_flags)
 		pr_alert("bad because of flags: %#lx(%pGp)\n",
@@ -1079,25 +1080,24 @@ static inline bool page_expected_state(struct page *page,
 
 static void free_pages_check_bad(struct page *page)
 {
-	const char *bad_reason;
+	int bad_reason;
 	unsigned long bad_flags;
 
-	bad_reason = NULL;
 	bad_flags = 0;
 
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason = "nonzero mapcount";
+		bad_reason = PAGE_BAD_NONZERO_MAPCOUNT;
 	if (unlikely(page->mapping != NULL))
-		bad_reason = "non-NULL mapping";
+		bad_reason = PAGE_BAD_NONNULL_MAPPING;
 	if (unlikely(page_ref_count(page) != 0))
-		bad_reason = "nonzero _refcount";
+		bad_reason = PAGE_BAD_NONZERO_REFCOUNT;
 	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
-		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
+		bad_reason = PAGE_BAD_FREE_FLAGS;
 		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
 	}
 #ifdef CONFIG_MEMCG
 	if (unlikely(page->mem_cgroup))
-		bad_reason = "page still charged to cgroup";
+		bad_reason = PAGE_BAD_CGROUP_CHARGED;
 #endif
 	bad_page(page, bad_reason, bad_flags);
 }
@@ -1130,7 +1130,7 @@ static int free_tail_pages_check(struct page *head_page, struct page *page)
 	case 1:
 		/* the first tail page: ->mapping may be compound_mapcount() */
 		if (unlikely(compound_mapcount(page))) {
-			bad_page(page, "nonzero compound_mapcount", 0);
+			bad_page(page, PAGE_BAD_COMPOUND_MAPCOUNT, 0);
 			goto out;
 		}
 		break;
@@ -1142,17 +1142,17 @@ static int free_tail_pages_check(struct page *head_page, struct page *page)
 		break;
 	default:
 		if (page->mapping != TAIL_MAPPING) {
-			bad_page(page, "corrupted mapping in tail page", 0);
+			bad_page(page, PAGE_BAD_TAIL_CORRUPTED, 0);
 			goto out;
 		}
 		break;
 	}
 	if (unlikely(!PageTail(page))) {
-		bad_page(page, "PageTail not set", 0);
+		bad_page(page, PAGE_BAD_TAIL_NOT_SET, 0);
 		goto out;
 	}
 	if (unlikely(compound_head(page) != head_page)) {
-		bad_page(page, "compound_head not consistent", 0);
+		bad_page(page, PAGE_BAD_COMPOUND_HEAD, 0);
 		goto out;
 	}
 	ret = 0;
@@ -2110,29 +2110,29 @@ static inline void expand(struct zone *zone, struct page *page,
 
 static void check_new_page_bad(struct page *page)
 {
-	const char *bad_reason = NULL;
+	int bad_reason;
 	unsigned long bad_flags = 0;
 
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason = "nonzero mapcount";
+		bad_reason = PAGE_BAD_NONZERO_MAPCOUNT;
 	if (unlikely(page->mapping != NULL))
-		bad_reason = "non-NULL mapping";
+		bad_reason = PAGE_BAD_NONNULL_MAPPING;
 	if (unlikely(page_ref_count(page) != 0))
-		bad_reason = "nonzero _refcount";
+		bad_reason = PAGE_BAD_NONZERO_REFCOUNT;
 	if (unlikely(page->flags & __PG_HWPOISON)) {
-		bad_reason = "HWPoisoned (hardware-corrupted)";
+		bad_reason = PAGE_BAD_HWPOISON;
 		bad_flags = __PG_HWPOISON;
 		/* Don't complain about hwpoisoned pages */
 		page_mapcount_reset(page); /* remove PageBuddy */
 		return;
 	}
 	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_PREP)) {
-		bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set";
+		bad_reason = PAGE_BAD_PREP_FLAGS;
 		bad_flags = PAGE_FLAGS_CHECK_AT_PREP;
 	}
 #ifdef CONFIG_MEMCG
 	if (unlikely(page->mem_cgroup))
-		bad_reason = "page still charged to cgroup";
+		bad_reason = PAGE_BAD_CGROUP_CHARGED;
 #endif
 	bad_page(page, bad_reason, bad_flags);
 }
-- 
2.20.1

Re: [RFC] mm/page_alloc: Enumerate bad page reasons

[Michal Hocko]

On Mon 30-03-20 12:21:37, Anshuman Khandual wrote:
> Enumerate all existing bad page reasons which can be used in bad_page() for
> reporting via __dump_page(). Unfortunately __dump_page() cannot be changed.
> __dump_page() is called from dump_page() that accepts a raw string and is
> also an exported symbol that is currently being used from various generic
> memory functions and other drivers. This reduces code duplication while
> reporting bad pages.

I dunno. It sounds like over engineering something that is an internal
stuff. Besides that I consider string reasons kinda obvious and I am
pretty sure I would have to check them for each numeric alias when want
to read the code. Yeah, yeah, nothing really hard but still...

So I am not really sure this is all worth the code churn. Besides
that I stongly suspect you wanted ...

> -static void bad_page(struct page *page, const char *reason,
> +static void bad_page(struct page *page, int reason,
>  		unsigned long bad_flags)

... enum page_bad_reason reason here, right? What is the point of declaring
an enum when you are not using it?
-- 
Michal Hocko
SUSE Labs

Re: [RFC] mm/page_alloc: Enumerate bad page reasons

[David Hildenbrand]

On 30.03.20 08:51, Anshuman Khandual wrote:
> Enumerate all existing bad page reasons which can be used in bad_page() for
> reporting via __dump_page(). Unfortunately __dump_page() cannot be changed.
> __dump_page() is called from dump_page() that accepts a raw string and is
> also an exported symbol that is currently being used from various generic
> memory functions and other drivers. This reduces code duplication while
> reporting bad pages.

Yeah sounds nice, but "56 insertions(+), 20 deletions(-)" does not sound
so nice ... and the "code duplication" is actually "repeating strings".

... I don't think we want/need this.

-- 
Thanks,

David / dhildenb

Re: [RFC] mm/page_alloc: Enumerate bad page reasons

[Anshuman Khandual]

On 03/30/2020 02:13 PM, Michal Hocko wrote:
> On Mon 30-03-20 12:21:37, Anshuman Khandual wrote:
>> Enumerate all existing bad page reasons which can be used in bad_page() for
>> reporting via __dump_page(). Unfortunately __dump_page() cannot be changed.
>> __dump_page() is called from dump_page() that accepts a raw string and is
>> also an exported symbol that is currently being used from various generic
>> memory functions and other drivers. This reduces code duplication while
>> reporting bad pages.
> 
> I dunno. It sounds like over engineering something that is an internal
> stuff. Besides that I consider string reasons kinda obvious and I am
> pretty sure I would have to check them for each numeric alias when want
> to read the code. Yeah, yeah, nothing really hard but still...

Right these are very much self explanatory. Would moving these aliases into
mm/page_alloc.c itself, make it any better for quicker access ?

> 
> So I am not really sure this is all worth the code churn. Besides

I understand but is not just repeating the same strings in similar functions
bit suboptimal as well.

> that I stongly suspect you wanted ...
> 
>> -static void bad_page(struct page *page, const char *reason,
>> +static void bad_page(struct page *page, int reason,
>>  		unsigned long bad_flags)
> 
> ... enum page_bad_reason reason here, right? What is the point of declaring
> an enum when you are not using it?

Sure, will replace here and other local reason variables which are 'int'.

Re: [RFC] mm/page_alloc: Enumerate bad page reasons

[Anshuman Khandual]

On 03/30/2020 02:15 PM, David Hildenbrand wrote:
> On 30.03.20 08:51, Anshuman Khandual wrote:
>> Enumerate all existing bad page reasons which can be used in bad_page() for
>> reporting via __dump_page(). Unfortunately __dump_page() cannot be changed.
>> __dump_page() is called from dump_page() that accepts a raw string and is
>> also an exported symbol that is currently being used from various generic
>> memory functions and other drivers. This reduces code duplication while
>> reporting bad pages.
> 
> Yeah sounds nice, but "56 insertions(+), 20 deletions(-)" does not sound
> so nice ... and the "code duplication" is actually "repeating strings".

But repeating strings in very similar functions dealing with bad page state
is not bit suboptimal ?

> 
> ... I don't think we want/need this.
> 

Re: [RFC] mm/page_alloc: Enumerate bad page reasons

[Michal Hocko]

On Mon 30-03-20 17:55:14, Anshuman Khandual wrote:
> 
> 
> On 03/30/2020 02:13 PM, Michal Hocko wrote:
> > On Mon 30-03-20 12:21:37, Anshuman Khandual wrote:
> >> Enumerate all existing bad page reasons which can be used in bad_page() for
> >> reporting via __dump_page(). Unfortunately __dump_page() cannot be changed.
> >> __dump_page() is called from dump_page() that accepts a raw string and is
> >> also an exported symbol that is currently being used from various generic
> >> memory functions and other drivers. This reduces code duplication while
> >> reporting bad pages.
> > 
> > I dunno. It sounds like over engineering something that is an internal
> > stuff. Besides that I consider string reasons kinda obvious and I am
> > pretty sure I would have to check them for each numeric alias when want
> > to read the code. Yeah, yeah, nothing really hard but still...
> 
> Right these are very much self explanatory. Would moving these aliases into
> mm/page_alloc.c itself, make it any better for quicker access ?

Not really. Cscopes doesn't really care where it is. It is the fact that
the constant makes to have a look is what makes this not an improvement
from my POV.
 
> > So I am not really sure this is all worth the code churn. Besides
> 
> I understand but is not just repeating the same strings in similar functions
> bit suboptimal as well.

I do not really see why that would be suboptimal.
-- 
Michal Hocko
SUSE Labs

Re: [RFC] mm/page_alloc: Enumerate bad page reasons

[David Hildenbrand]

On 30.03.20 14:28, Anshuman Khandual wrote:
> 
> 
> On 03/30/2020 02:15 PM, David Hildenbrand wrote:
>> On 30.03.20 08:51, Anshuman Khandual wrote:
>>> Enumerate all existing bad page reasons which can be used in bad_page() for
>>> reporting via __dump_page(). Unfortunately __dump_page() cannot be changed.
>>> __dump_page() is called from dump_page() that accepts a raw string and is
>>> also an exported symbol that is currently being used from various generic
>>> memory functions and other drivers. This reduces code duplication while
>>> reporting bad pages.
>>
>> Yeah sounds nice, but "56 insertions(+), 20 deletions(-)" does not sound
>> so nice ... and the "code duplication" is actually "repeating strings".
> 
> But repeating strings in very similar functions dealing with bad page state
> is not bit suboptimal ?

Who cares? This is debug output.

-- 
Thanks,

David / dhildenb