[Patch v2 0/4] mm/page_alloc.c: cleanup on check page

[Patch v2 0/4] mm/page_alloc.c: cleanup on check page

[Wei Yang]

The patch set does some cleanup related to check page.

1. Enable passing all bad reason to __dump_page()
2. Extract common part to check page
3. Remove unnecessary bad_reason assignment

Thanks suggestions from David Rientjes.

v2:
  * merge two rename patches into extract patch
  * enable dump several reasons for __dump_page()

Wei Yang (4):
  mm: enable dump several reasons for __dump_page()
  mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison
  mm/page_alloc.c: pass all bad reasons to bad_page()
  mm/page_alloc.c: extract commom part to check page

 include/linux/mmdebug.h |  2 +-
 mm/debug.c              | 11 +++---
 mm/page_alloc.c         | 87 +++++++++++++++++++++--------------------
 3 files changed, 51 insertions(+), 49 deletions(-)

-- 
2.17.1

[Patch v2 1/4] mm: enable dump several reasons for __dump_page()

[Wei Yang]

This is a preparation to dump all reasons during check page.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 include/linux/mmdebug.h |  2 +-
 mm/debug.c              | 11 ++++++-----
 mm/page_alloc.c         |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 2ad72d2c8cc5..f0a612db8bae 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -10,7 +10,7 @@ struct vm_area_struct;
 struct mm_struct;
 
 extern void dump_page(struct page *page, const char *reason);
-extern void __dump_page(struct page *page, const char *reason);
+extern void __dump_page(struct page *page, int num, const char **reason);
 void dump_vma(const struct vm_area_struct *vma);
 void dump_mm(const struct mm_struct *mm);
 
diff --git a/mm/debug.c b/mm/debug.c
index 0461df1207cb..a8ac6f951f9f 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -42,11 +42,11 @@ const struct trace_print_flags vmaflag_names[] = {
 	{0, NULL}
 };
 
-void __dump_page(struct page *page, const char *reason)
+void __dump_page(struct page *page, int num, const char **reason)
 {
 	struct address_space *mapping;
 	bool page_poisoned = PagePoisoned(page);
-	int mapcount;
+	int mapcount, i;
 
 	/*
 	 * If struct page is poisoned don't access Page*() functions as that
@@ -97,8 +97,9 @@ void __dump_page(struct page *page, const char *reason)
 			sizeof(unsigned long), page,
 			sizeof(struct page), false);
 
-	if (reason)
-		pr_warn("page dumped because: %s\n", reason);
+	pr_warn("page dumped because:\n");
+	for (i = 0; i < num; i++)
+		pr_warn("\t%s\n", reason[i]);
 
 #ifdef CONFIG_MEMCG
 	if (!page_poisoned && page->mem_cgroup)
@@ -108,7 +109,7 @@ void __dump_page(struct page *page, const char *reason)
 
 void dump_page(struct page *page, const char *reason)
 {
-	__dump_page(page, reason);
+	__dump_page(page, 1, &reason);
 	dump_page_owner(page);
 }
 EXPORT_SYMBOL(dump_page);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d047bf7d8fd4..0cf6218aaba7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -638,7 +638,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, 1, &reason);
 	bad_flags &= page->flags;
 	if (bad_flags)
 		pr_alert("bad because of flags: %#lx(%pGp)\n",
-- 
2.17.1

Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()

[Anshuman Khandual]

On 01/20/2020 08:34 AM, Wei Yang wrote:
> This is a preparation to dump all reasons during check page.

This really makes sense rather then just picking the reason from
the last "if" statement.

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  include/linux/mmdebug.h |  2 +-
>  mm/debug.c              | 11 ++++++-----
>  mm/page_alloc.c         |  2 +-
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 2ad72d2c8cc5..f0a612db8bae 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -10,7 +10,7 @@ struct vm_area_struct;
>  struct mm_struct;
>  
>  extern void dump_page(struct page *page, const char *reason);
> -extern void __dump_page(struct page *page, const char *reason);
> +extern void __dump_page(struct page *page, int num, const char **reason);
>  void dump_vma(const struct vm_area_struct *vma);
>  void dump_mm(const struct mm_struct *mm);
>  
> diff --git a/mm/debug.c b/mm/debug.c
> index 0461df1207cb..a8ac6f951f9f 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -42,11 +42,11 @@ const struct trace_print_flags vmaflag_names[] = {
>  	{0, NULL}
>  };
>  
> -void __dump_page(struct page *page, const char *reason)
> +void __dump_page(struct page *page, int num, const char **reason)
>  {
>  	struct address_space *mapping;
>  	bool page_poisoned = PagePoisoned(page);
> -	int mapcount;
> +	int mapcount, i;
>  
>  	/*
>  	 * If struct page is poisoned don't access Page*() functions as that
> @@ -97,8 +97,9 @@ void __dump_page(struct page *page, const char *reason)
>  			sizeof(unsigned long), page,
>  			sizeof(struct page), false);
>  
> -	if (reason)
> -		pr_warn("page dumped because: %s\n", reason);
> +	pr_warn("page dumped because:\n");
> +	for (i = 0; i < num; i++)
> +		pr_warn("\t%s\n", reason[i]);

We should have a NR_BAD_PAGE_REASONS or something to cap this iteration
and also check reason[i] for non-NULL before trying to print the array.
There might be call sites like the following which will be problematic
otherwise.

split_huge_page_to_list() -> dump_page(head, NULL)

>  
>  #ifdef CONFIG_MEMCG
>  	if (!page_poisoned && page->mem_cgroup)

While here, will it be better to move the above debug print block after
mem_cgroup block instead ?

> @@ -108,7 +109,7 @@ void __dump_page(struct page *page, const char *reason)
>  
>  void dump_page(struct page *page, const char *reason)
>  {
> -	__dump_page(page, reason);
> +	__dump_page(page, 1, &reason);
>  	dump_page_owner(page);
>  }
>  EXPORT_SYMBOL(dump_page);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d047bf7d8fd4..0cf6218aaba7 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -638,7 +638,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, 1, &reason);
>  	bad_flags &= page->flags;
>  	if (bad_flags)
>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
> 

Do we still need to have bad_flags ? After consolidating all reasons making
a page bad should not we just print page->flags unconditionally each time and
let the user decipher it instead. __dump_page() will print page->flags for
each case (atleast after the new patch from Vlastimil). AFAICS, the only
place currently consuming bad_flags is bad_page() which seems redundant after
first calling __dump_page().

Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()

[Wei Yang]

On Mon, Jan 20, 2020 at 11:42:30AM +0530, Anshuman Khandual wrote:
>
>
>On 01/20/2020 08:34 AM, Wei Yang wrote:
>> This is a preparation to dump all reasons during check page.
>
>This really makes sense rather then just picking the reason from
>the last "if" statement.
>
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  include/linux/mmdebug.h |  2 +-
>>  mm/debug.c              | 11 ++++++-----
>>  mm/page_alloc.c         |  2 +-
>>  3 files changed, 8 insertions(+), 7 deletions(-)
>> 
>> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
>> index 2ad72d2c8cc5..f0a612db8bae 100644
>> --- a/include/linux/mmdebug.h
>> +++ b/include/linux/mmdebug.h
>> @@ -10,7 +10,7 @@ struct vm_area_struct;
>>  struct mm_struct;
>>  
>>  extern void dump_page(struct page *page, const char *reason);
>> -extern void __dump_page(struct page *page, const char *reason);
>> +extern void __dump_page(struct page *page, int num, const char **reason);
>>  void dump_vma(const struct vm_area_struct *vma);
>>  void dump_mm(const struct mm_struct *mm);
>>  
>> diff --git a/mm/debug.c b/mm/debug.c
>> index 0461df1207cb..a8ac6f951f9f 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -42,11 +42,11 @@ const struct trace_print_flags vmaflag_names[] = {
>>  	{0, NULL}
>>  };
>>  
>> -void __dump_page(struct page *page, const char *reason)
>> +void __dump_page(struct page *page, int num, const char **reason)
>>  {
>>  	struct address_space *mapping;
>>  	bool page_poisoned = PagePoisoned(page);
>> -	int mapcount;
>> +	int mapcount, i;
>>  
>>  	/*
>>  	 * If struct page is poisoned don't access Page*() functions as that
>> @@ -97,8 +97,9 @@ void __dump_page(struct page *page, const char *reason)
>>  			sizeof(unsigned long), page,
>>  			sizeof(struct page), false);
>>  
>> -	if (reason)
>> -		pr_warn("page dumped because: %s\n", reason);
>> +	pr_warn("page dumped because:\n");
>> +	for (i = 0; i < num; i++)
>> +		pr_warn("\t%s\n", reason[i]);
>
>We should have a NR_BAD_PAGE_REASONS or something to cap this iteration
>and also check reason[i] for non-NULL before trying to print the array.
>There might be call sites like the following which will be problematic
>otherwise.
>
>split_huge_page_to_list() -> dump_page(head, NULL)
>

You are right, I missed this case.

>>  
>>  #ifdef CONFIG_MEMCG
>>  	if (!page_poisoned && page->mem_cgroup)
>
>While here, will it be better to move the above debug print block after
>mem_cgroup block instead ?
>

Not sure, let's see whether others have some idea.

>> @@ -108,7 +109,7 @@ void __dump_page(struct page *page, const char *reason)
>>  
>>  void dump_page(struct page *page, const char *reason)
>>  {
>> -	__dump_page(page, reason);
>> +	__dump_page(page, 1, &reason);
>>  	dump_page_owner(page);
>>  }
>>  EXPORT_SYMBOL(dump_page);
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index d047bf7d8fd4..0cf6218aaba7 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -638,7 +638,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, 1, &reason);
>>  	bad_flags &= page->flags;
>>  	if (bad_flags)
>>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
>> 
>
>Do we still need to have bad_flags ? After consolidating all reasons making
>a page bad should not we just print page->flags unconditionally each time and
>let the user decipher it instead. __dump_page() will print page->flags for
>each case (atleast after the new patch from Vlastimil). AFAICS, the only
>place currently consuming bad_flags is bad_page() which seems redundant after
>first calling __dump_page().

Hmm... I don't catch this. The work in __dump_page() seems a little different
from this one. Not sure we could remove it.

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()

[Anshuman Khandual]

On 01/20/2020 02:25 PM, Wei Yang wrote:
> On Mon, Jan 20, 2020 at 11:42:30AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 01/20/2020 08:34 AM, Wei Yang wrote:
>>> This is a preparation to dump all reasons during check page.
>>
>> This really makes sense rather then just picking the reason from
>> the last "if" statement.
>>
>>>
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>>  include/linux/mmdebug.h |  2 +-
>>>  mm/debug.c              | 11 ++++++-----
>>>  mm/page_alloc.c         |  2 +-
>>>  3 files changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
>>> index 2ad72d2c8cc5..f0a612db8bae 100644
>>> --- a/include/linux/mmdebug.h
>>> +++ b/include/linux/mmdebug.h
>>> @@ -10,7 +10,7 @@ struct vm_area_struct;
>>>  struct mm_struct;
>>>  
>>>  extern void dump_page(struct page *page, const char *reason);
>>> -extern void __dump_page(struct page *page, const char *reason);
>>> +extern void __dump_page(struct page *page, int num, const char **reason);
>>>  void dump_vma(const struct vm_area_struct *vma);
>>>  void dump_mm(const struct mm_struct *mm);
>>>  
>>> diff --git a/mm/debug.c b/mm/debug.c
>>> index 0461df1207cb..a8ac6f951f9f 100644
>>> --- a/mm/debug.c
>>> +++ b/mm/debug.c
>>> @@ -42,11 +42,11 @@ const struct trace_print_flags vmaflag_names[] = {
>>>  	{0, NULL}
>>>  };
>>>  
>>> -void __dump_page(struct page *page, const char *reason)
>>> +void __dump_page(struct page *page, int num, const char **reason)
>>>  {
>>>  	struct address_space *mapping;
>>>  	bool page_poisoned = PagePoisoned(page);
>>> -	int mapcount;
>>> +	int mapcount, i;
>>>  
>>>  	/*
>>>  	 * If struct page is poisoned don't access Page*() functions as that
>>> @@ -97,8 +97,9 @@ void __dump_page(struct page *page, const char *reason)
>>>  			sizeof(unsigned long), page,
>>>  			sizeof(struct page), false);
>>>  
>>> -	if (reason)
>>> -		pr_warn("page dumped because: %s\n", reason);
>>> +	pr_warn("page dumped because:\n");
>>> +	for (i = 0; i < num; i++)
>>> +		pr_warn("\t%s\n", reason[i]);
>>
>> We should have a NR_BAD_PAGE_REASONS or something to cap this iteration
>> and also check reason[i] for non-NULL before trying to print the array.
>> There might be call sites like the following which will be problematic
>> otherwise.
>>
>> split_huge_page_to_list() -> dump_page(head, NULL)
>>
> 
> You are right, I missed this case.
> 
>>>  
>>>  #ifdef CONFIG_MEMCG
>>>  	if (!page_poisoned && page->mem_cgroup)
>>
>> While here, will it be better to move the above debug print block after
>> mem_cgroup block instead ?
>>
> 
> Not sure, let's see whether others have some idea.
> 
>>> @@ -108,7 +109,7 @@ void __dump_page(struct page *page, const char *reason)
>>>  
>>>  void dump_page(struct page *page, const char *reason)
>>>  {
>>> -	__dump_page(page, reason);
>>> +	__dump_page(page, 1, &reason);
>>>  	dump_page_owner(page);
>>>  }
>>>  EXPORT_SYMBOL(dump_page);
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index d047bf7d8fd4..0cf6218aaba7 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -638,7 +638,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, 1, &reason);
>>>  	bad_flags &= page->flags;
>>>  	if (bad_flags)
>>>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
>>>
>>
>> Do we still need to have bad_flags ? After consolidating all reasons making
>> a page bad should not we just print page->flags unconditionally each time and
>> let the user decipher it instead. __dump_page() will print page->flags for
>> each case (atleast after the new patch from Vlastimil). AFAICS, the only
>> place currently consuming bad_flags is bad_page() which seems redundant after
>> first calling __dump_page().
> 
> Hmm... I don't catch this. The work in __dump_page() seems a little different
> from this one. Not sure we could remove it.

Lets look at 'bad_flags' as it exists today without this series.

It gets evaluated in free_pages_check_bad() and check_new_page_bad() before
being passed into bad_page(). All other call sites for bad_page() just pass
0 for 'bad_flags'. Now in bad_page(), we have

        __dump_page(page, reason);
        bad_flags &= page->flags;
        if (bad_flags)
                pr_alert("bad because of flags: %#lx(%pGp)\n",
                                                bad_flags, &bad_flags);

Here, bad_flags &= page->flags will always be positive when 'reason'
is either

"PAGE_FLAGS_CHECK_AT_FREE flag(s) set"

or

"PAGE_FLAGS_CHECK_AT_PREP flag set"

The point here is we dont need to print bad_flags here as __dump_page()
already prints page->flags universally along with the "bad_reason"
after the following change.

[1] https://patchwork.kernel.org/patch/11332035/

Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()

[Wei Yang]

On Tue, Jan 21, 2020 at 10:50:41AM +0530, Anshuman Khandual wrote:
>
>
>On 01/20/2020 02:25 PM, Wei Yang wrote:
>> On Mon, Jan 20, 2020 at 11:42:30AM +0530, Anshuman Khandual wrote:
>>>
>>>
>>> On 01/20/2020 08:34 AM, Wei Yang wrote:
>>>> This is a preparation to dump all reasons during check page.
>>>
>>> This really makes sense rather then just picking the reason from
>>> the last "if" statement.
>>>
>>>>
>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>> ---
>>>>  include/linux/mmdebug.h |  2 +-
>>>>  mm/debug.c              | 11 ++++++-----
>>>>  mm/page_alloc.c         |  2 +-
>>>>  3 files changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
>>>> index 2ad72d2c8cc5..f0a612db8bae 100644
>>>> --- a/include/linux/mmdebug.h
>>>> +++ b/include/linux/mmdebug.h
>>>> @@ -10,7 +10,7 @@ struct vm_area_struct;
>>>>  struct mm_struct;
>>>>  
>>>>  extern void dump_page(struct page *page, const char *reason);
>>>> -extern void __dump_page(struct page *page, const char *reason);
>>>> +extern void __dump_page(struct page *page, int num, const char **reason);
>>>>  void dump_vma(const struct vm_area_struct *vma);
>>>>  void dump_mm(const struct mm_struct *mm);
>>>>  
>>>> diff --git a/mm/debug.c b/mm/debug.c
>>>> index 0461df1207cb..a8ac6f951f9f 100644
>>>> --- a/mm/debug.c
>>>> +++ b/mm/debug.c
>>>> @@ -42,11 +42,11 @@ const struct trace_print_flags vmaflag_names[] = {
>>>>  	{0, NULL}
>>>>  };
>>>>  
>>>> -void __dump_page(struct page *page, const char *reason)
>>>> +void __dump_page(struct page *page, int num, const char **reason)
>>>>  {
>>>>  	struct address_space *mapping;
>>>>  	bool page_poisoned = PagePoisoned(page);
>>>> -	int mapcount;
>>>> +	int mapcount, i;
>>>>  
>>>>  	/*
>>>>  	 * If struct page is poisoned don't access Page*() functions as that
>>>> @@ -97,8 +97,9 @@ void __dump_page(struct page *page, const char *reason)
>>>>  			sizeof(unsigned long), page,
>>>>  			sizeof(struct page), false);
>>>>  
>>>> -	if (reason)
>>>> -		pr_warn("page dumped because: %s\n", reason);
>>>> +	pr_warn("page dumped because:\n");
>>>> +	for (i = 0; i < num; i++)
>>>> +		pr_warn("\t%s\n", reason[i]);
>>>
>>> We should have a NR_BAD_PAGE_REASONS or something to cap this iteration
>>> and also check reason[i] for non-NULL before trying to print the array.
>>> There might be call sites like the following which will be problematic
>>> otherwise.
>>>
>>> split_huge_page_to_list() -> dump_page(head, NULL)
>>>
>> 
>> You are right, I missed this case.
>> 
>>>>  
>>>>  #ifdef CONFIG_MEMCG
>>>>  	if (!page_poisoned && page->mem_cgroup)
>>>
>>> While here, will it be better to move the above debug print block after
>>> mem_cgroup block instead ?
>>>
>> 
>> Not sure, let's see whether others have some idea.
>> 
>>>> @@ -108,7 +109,7 @@ void __dump_page(struct page *page, const char *reason)
>>>>  
>>>>  void dump_page(struct page *page, const char *reason)
>>>>  {
>>>> -	__dump_page(page, reason);
>>>> +	__dump_page(page, 1, &reason);
>>>>  	dump_page_owner(page);
>>>>  }
>>>>  EXPORT_SYMBOL(dump_page);
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index d047bf7d8fd4..0cf6218aaba7 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -638,7 +638,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, 1, &reason);
>>>>  	bad_flags &= page->flags;
>>>>  	if (bad_flags)
>>>>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
>>>>
>>>
>>> Do we still need to have bad_flags ? After consolidating all reasons making
>>> a page bad should not we just print page->flags unconditionally each time and
>>> let the user decipher it instead. __dump_page() will print page->flags for
>>> each case (atleast after the new patch from Vlastimil). AFAICS, the only
>>> place currently consuming bad_flags is bad_page() which seems redundant after
>>> first calling __dump_page().
>> 
>> Hmm... I don't catch this. The work in __dump_page() seems a little different
>> from this one. Not sure we could remove it.
>
>Lets look at 'bad_flags' as it exists today without this series.
>
>It gets evaluated in free_pages_check_bad() and check_new_page_bad() before
>being passed into bad_page(). All other call sites for bad_page() just pass
>0 for 'bad_flags'. Now in bad_page(), we have
>
>        __dump_page(page, reason);
>        bad_flags &= page->flags;
>        if (bad_flags)
>                pr_alert("bad because of flags: %#lx(%pGp)\n",
>                                                bad_flags, &bad_flags);
>
>Here, bad_flags &= page->flags will always be positive when 'reason'
>is either
>
>"PAGE_FLAGS_CHECK_AT_FREE flag(s) set"
>
>or
>
>"PAGE_FLAGS_CHECK_AT_PREP flag set"
>
>The point here is we dont need to print bad_flags here as __dump_page()
>already prints page->flags universally along with the "bad_reason"
>after the following change.
>

Thanks, I see your point. It is not necessary to print flags twice.

>[1] https://patchwork.kernel.org/patch/11332035/

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()

[Wei Yang]

On Tue, Jan 21, 2020 at 10:50:41AM +0530, Anshuman Khandual wrote:
>
>
>On 01/20/2020 02:25 PM, Wei Yang wrote:
>> On Mon, Jan 20, 2020 at 11:42:30AM +0530, Anshuman Khandual wrote:
>>>
>>>
>>> On 01/20/2020 08:34 AM, Wei Yang wrote:
>>>> This is a preparation to dump all reasons during check page.
>>>
>>> This really makes sense rather then just picking the reason from
>>> the last "if" statement.
>>>
>>>>
>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>> ---
>>>>  include/linux/mmdebug.h |  2 +-
>>>>  mm/debug.c              | 11 ++++++-----
>>>>  mm/page_alloc.c         |  2 +-
>>>>  3 files changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
>>>> index 2ad72d2c8cc5..f0a612db8bae 100644
>>>> --- a/include/linux/mmdebug.h
>>>> +++ b/include/linux/mmdebug.h
>>>> @@ -10,7 +10,7 @@ struct vm_area_struct;
>>>>  struct mm_struct;
>>>>  
>>>>  extern void dump_page(struct page *page, const char *reason);
>>>> -extern void __dump_page(struct page *page, const char *reason);
>>>> +extern void __dump_page(struct page *page, int num, const char **reason);
>>>>  void dump_vma(const struct vm_area_struct *vma);
>>>>  void dump_mm(const struct mm_struct *mm);
>>>>  
>>>> diff --git a/mm/debug.c b/mm/debug.c
>>>> index 0461df1207cb..a8ac6f951f9f 100644
>>>> --- a/mm/debug.c
>>>> +++ b/mm/debug.c
>>>> @@ -42,11 +42,11 @@ const struct trace_print_flags vmaflag_names[] = {
>>>>  	{0, NULL}
>>>>  };
>>>>  
>>>> -void __dump_page(struct page *page, const char *reason)
>>>> +void __dump_page(struct page *page, int num, const char **reason)
>>>>  {
>>>>  	struct address_space *mapping;
>>>>  	bool page_poisoned = PagePoisoned(page);
>>>> -	int mapcount;
>>>> +	int mapcount, i;
>>>>  
>>>>  	/*
>>>>  	 * If struct page is poisoned don't access Page*() functions as that
>>>> @@ -97,8 +97,9 @@ void __dump_page(struct page *page, const char *reason)
>>>>  			sizeof(unsigned long), page,
>>>>  			sizeof(struct page), false);
>>>>  
>>>> -	if (reason)
>>>> -		pr_warn("page dumped because: %s\n", reason);
>>>> +	pr_warn("page dumped because:\n");
>>>> +	for (i = 0; i < num; i++)
>>>> +		pr_warn("\t%s\n", reason[i]);
>>>
>>> We should have a NR_BAD_PAGE_REASONS or something to cap this iteration
>>> and also check reason[i] for non-NULL before trying to print the array.
>>> There might be call sites like the following which will be problematic
>>> otherwise.
>>>
>>> split_huge_page_to_list() -> dump_page(head, NULL)
>>>
>> 
>> You are right, I missed this case.
>> 
>>>>  
>>>>  #ifdef CONFIG_MEMCG
>>>>  	if (!page_poisoned && page->mem_cgroup)
>>>
>>> While here, will it be better to move the above debug print block after
>>> mem_cgroup block instead ?
>>>
>> 
>> Not sure, let's see whether others have some idea.
>> 
>>>> @@ -108,7 +109,7 @@ void __dump_page(struct page *page, const char *reason)
>>>>  
>>>>  void dump_page(struct page *page, const char *reason)
>>>>  {
>>>> -	__dump_page(page, reason);
>>>> +	__dump_page(page, 1, &reason);
>>>>  	dump_page_owner(page);
>>>>  }
>>>>  EXPORT_SYMBOL(dump_page);
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index d047bf7d8fd4..0cf6218aaba7 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -638,7 +638,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, 1, &reason);
>>>>  	bad_flags &= page->flags;
>>>>  	if (bad_flags)
>>>>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
>>>>
>>>
>>> Do we still need to have bad_flags ? After consolidating all reasons making
>>> a page bad should not we just print page->flags unconditionally each time and
>>> let the user decipher it instead. __dump_page() will print page->flags for
>>> each case (atleast after the new patch from Vlastimil). AFAICS, the only
>>> place currently consuming bad_flags is bad_page() which seems redundant after
>>> first calling __dump_page().
>> 
>> Hmm... I don't catch this. The work in __dump_page() seems a little different
>> from this one. Not sure we could remove it.
>
>Lets look at 'bad_flags' as it exists today without this series.
>
>It gets evaluated in free_pages_check_bad() and check_new_page_bad() before
>being passed into bad_page(). All other call sites for bad_page() just pass
>0 for 'bad_flags'. Now in bad_page(), we have
>
>        __dump_page(page, reason);
>        bad_flags &= page->flags;
>        if (bad_flags)
>                pr_alert("bad because of flags: %#lx(%pGp)\n",
>                                                bad_flags, &bad_flags);
>
>Here, bad_flags &= page->flags will always be positive when 'reason'
>is either
>
>"PAGE_FLAGS_CHECK_AT_FREE flag(s) set"
>
>or
>
>"PAGE_FLAGS_CHECK_AT_PREP flag set"
>
>The point here is we dont need to print bad_flags here as __dump_page()
>already prints page->flags universally along with the "bad_reason"
>after the following change.
>
>[1] https://patchwork.kernel.org/patch/11332035/

Hi, Anshuman

I am preparing a patch to remove the bad_flags. While since the above change
is not merged upstream yet, how can I wording the change log to point this
change?

Or I should wait till this one is merged?

-- 
Wei Yang
Help you, Help me

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

[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 <richardw.yang@linux.intel.com>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/page_alloc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0cf6218aaba7..a43b9d2482f2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2051,8 +2051,6 @@ static void check_new_page_bad(struct page *page)
 	if (unlikely(page_ref_count(page) != 0))
 		bad_reason = "nonzero _refcount";
 	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;
-- 
2.17.1

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

[Anshuman Khandual]

On 01/20/2020 08:34 AM, Wei Yang wrote:
> 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 <richardw.yang@linux.intel.com>
> Acked-by: David Rientjes <rientjes@google.com>
> ---
>  mm/page_alloc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0cf6218aaba7..a43b9d2482f2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2051,8 +2051,6 @@ static void check_new_page_bad(struct page *page)
>  	if (unlikely(page_ref_count(page) != 0))
>  		bad_reason = "nonzero _refcount";
>  	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;

This bail out condition should be the first in the function
check_new_page_bad() before evaluating bad_[reason|flags]
as they will never be used.

> 

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

[Wei Yang]

On Mon, Jan 20, 2020 at 11:58:04AM +0530, Anshuman Khandual wrote:
>
>
>On 01/20/2020 08:34 AM, Wei Yang wrote:
>> 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 <richardw.yang@linux.intel.com>
>> Acked-by: David Rientjes <rientjes@google.com>
>> ---
>>  mm/page_alloc.c | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 0cf6218aaba7..a43b9d2482f2 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -2051,8 +2051,6 @@ static void check_new_page_bad(struct page *page)
>>  	if (unlikely(page_ref_count(page) != 0))
>>  		bad_reason = "nonzero _refcount";
>>  	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;
>
>This bail out condition should be the first in the function
>check_new_page_bad() before evaluating bad_[reason|flags]
>as they will never be used.
>

This is reasonable.

>> 

-- 
Wei Yang
Help you, Help me

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

[Michal Hocko]

On Mon 20-01-20 11:04:13, Wei Yang wrote:
> 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 <richardw.yang@linux.intel.com>
> Acked-by: David Rientjes <rientjes@google.com>

This is a left over from loong time ago. AFAICS bad_reason and flags hav
never been used.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_alloc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0cf6218aaba7..a43b9d2482f2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2051,8 +2051,6 @@ static void check_new_page_bad(struct page *page)
>  	if (unlikely(page_ref_count(page) != 0))
>  		bad_reason = "nonzero _refcount";
>  	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;
> -- 
> 2.17.1

-- 
Michal Hocko
SUSE Labs

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

[David Hildenbrand]

On 20.01.20 04:04, Wei Yang wrote:
> 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 <richardw.yang@linux.intel.com>
> Acked-by: David Rientjes <rientjes@google.com>
> ---
>  mm/page_alloc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0cf6218aaba7..a43b9d2482f2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2051,8 +2051,6 @@ static void check_new_page_bad(struct page *page)
>  	if (unlikely(page_ref_count(page) != 0))
>  		bad_reason = "nonzero _refcount";
>  	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;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[Wei Yang]

Now we can pass all bad reasons to __dump_page().

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 mm/page_alloc.c | 52 ++++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a43b9d2482f2..a7b793c739fc 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -609,7 +609,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 nr, const char **reason,
 		unsigned long bad_flags)
 {
 	static unsigned long resume;
@@ -638,7 +638,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, 1, &reason);
+	__dump_page(page, nr, reason);
 	bad_flags &= page->flags;
 	if (bad_flags)
 		pr_alert("bad because of flags: %#lx(%pGp)\n",
@@ -1027,27 +1027,25 @@ static inline bool page_expected_state(struct page *page,
 
 static void free_pages_check_bad(struct page *page)
 {
-	const char *bad_reason;
-	unsigned long bad_flags;
-
-	bad_reason = NULL;
-	bad_flags = 0;
+	const char *bad_reason[5];
+	unsigned long bad_flags = 0;
+	int nr = 0;
 
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason = "nonzero mapcount";
+		bad_reason[nr++] = "nonzero mapcount";
 	if (unlikely(page->mapping != NULL))
-		bad_reason = "non-NULL mapping";
+		bad_reason[nr++] = "non-NULL mapping";
 	if (unlikely(page_ref_count(page) != 0))
-		bad_reason = "nonzero _refcount";
+		bad_reason[nr++] = "nonzero _refcount";
 	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
-		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
+		bad_reason[nr++] = "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";
+		bad_reason[nr++] = "page still charged to cgroup";
 #endif
-	bad_page(page, bad_reason, bad_flags);
+	bad_page(page, nr, bad_reason, bad_flags);
 }
 
 static inline int free_pages_check(struct page *page)
@@ -1062,6 +1060,7 @@ static inline int free_pages_check(struct page *page)
 
 static int free_tail_pages_check(struct page *head_page, struct page *page)
 {
+	const char *reason;
 	int ret = 1;
 
 	/*
@@ -1078,7 +1077,8 @@ 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);
+			reason = "nonzero compound_mapcount";
+			bad_page(page, 1, &reason, 0);
 			goto out;
 		}
 		break;
@@ -1090,17 +1090,20 @@ 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);
+			reason = "corrupted mapping in tail page";
+			bad_page(page, 1, &reason, 0);
 			goto out;
 		}
 		break;
 	}
 	if (unlikely(!PageTail(page))) {
-		bad_page(page, "PageTail not set", 0);
+		reason = "PageTail not set";
+		bad_page(page, 1, &reason, 0);
 		goto out;
 	}
 	if (unlikely(compound_head(page) != head_page)) {
-		bad_page(page, "compound_head not consistent", 0);
+		reason = "compound_head not consistent";
+		bad_page(page, 1, &reason, 0);
 		goto out;
 	}
 	ret = 0;
@@ -2041,29 +2044,30 @@ static inline void expand(struct zone *zone, struct page *page,
 
 static void check_new_page_bad(struct page *page)
 {
-	const char *bad_reason = NULL;
+	const char *bad_reason[5];
 	unsigned long bad_flags = 0;
+	int nr = 0;
 
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason = "nonzero mapcount";
+		bad_reason[nr++] = "nonzero mapcount";
 	if (unlikely(page->mapping != NULL))
-		bad_reason = "non-NULL mapping";
+		bad_reason[nr++] = "non-NULL mapping";
 	if (unlikely(page_ref_count(page) != 0))
-		bad_reason = "nonzero _refcount";
+		bad_reason[nr++] = "nonzero _refcount";
 	if (unlikely(page->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[nr++] = "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";
+		bad_reason[nr++] = "page still charged to cgroup";
 #endif
-	bad_page(page, bad_reason, bad_flags);
+	bad_page(page, 1, bad_reason, bad_flags);
 }
 
 /*
-- 
2.17.1

Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[Anshuman Khandual]

On 01/20/2020 08:34 AM, Wei Yang wrote:
> Now we can pass all bad reasons to __dump_page().
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  mm/page_alloc.c | 52 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a43b9d2482f2..a7b793c739fc 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -609,7 +609,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 nr, const char **reason,
>  		unsigned long bad_flags)
>  {
>  	static unsigned long resume;
> @@ -638,7 +638,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, 1, &reason);
> +	__dump_page(page, nr, reason);
>  	bad_flags &= page->flags;
>  	if (bad_flags)
>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
> @@ -1027,27 +1027,25 @@ static inline bool page_expected_state(struct page *page,
>  
>  static void free_pages_check_bad(struct page *page)
>  {
> -	const char *bad_reason;
> -	unsigned long bad_flags;
> -
> -	bad_reason = NULL;
> -	bad_flags = 0;
> +	const char *bad_reason[5];

s/5/NR_BAD_PAGE_REASONS


> +	unsigned long bad_flags = 0;
> +	int nr = 0;
>  
>  	if (unlikely(atomic_read(&page->_mapcount) != -1))
> -		bad_reason = "nonzero mapcount";
> +		bad_reason[nr++] = "nonzero mapcount";
>  	if (unlikely(page->mapping != NULL))
> -		bad_reason = "non-NULL mapping";
> +		bad_reason[nr++] = "non-NULL mapping";
>  	if (unlikely(page_ref_count(page) != 0))
> -		bad_reason = "nonzero _refcount";
> +		bad_reason[nr++] = "nonzero _refcount";
>  	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
> -		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
> +		bad_reason[nr++] = "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";
> +		bad_reason[nr++] = "page still charged to cgroup";
>  #endif
> -	bad_page(page, bad_reason, bad_flags);
> +	bad_page(page, nr, bad_reason, bad_flags);
>  }
>  
>  static inline int free_pages_check(struct page *page)
> @@ -1062,6 +1060,7 @@ static inline int free_pages_check(struct page *page)
>  
>  static int free_tail_pages_check(struct page *head_page, struct page *page)
>  {
> +	const char *reason;
>  	int ret = 1;
>  
>  	/*
> @@ -1078,7 +1077,8 @@ 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);
> +			reason = "nonzero compound_mapcount";
> +			bad_page(page, 1, &reason, 0);
>  			goto out;
>  		}
>  		break;
> @@ -1090,17 +1090,20 @@ 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);
> +			reason = "corrupted mapping in tail page";
> +			bad_page(page, 1, &reason, 0);
>  			goto out;
>  		}
>  		break;
>  	}
>  	if (unlikely(!PageTail(page))) {
> -		bad_page(page, "PageTail not set", 0);
> +		reason = "PageTail not set";
> +		bad_page(page, 1, &reason, 0);
>  		goto out;
>  	}
>  	if (unlikely(compound_head(page) != head_page)) {
> -		bad_page(page, "compound_head not consistent", 0);
> +		reason = "compound_head not consistent";
> +		bad_page(page, 1, &reason, 0);
>  		goto out;
>  	}
>  	ret = 0;
> @@ -2041,29 +2044,30 @@ static inline void expand(struct zone *zone, struct page *page,
>  
>  static void check_new_page_bad(struct page *page)
>  {
> -	const char *bad_reason = NULL;
> +	const char *bad_reason[5];

s/5/NR_BAD_PAGE_REASONS 

>  	unsigned long bad_flags = 0;
> +	int nr = 0;
>  
>  	if (unlikely(atomic_read(&page->_mapcount) != -1))
> -		bad_reason = "nonzero mapcount";
> +		bad_reason[nr++] = "nonzero mapcount";
>  	if (unlikely(page->mapping != NULL))
> -		bad_reason = "non-NULL mapping";
> +		bad_reason[nr++] = "non-NULL mapping";
>  	if (unlikely(page_ref_count(page) != 0))
> -		bad_reason = "nonzero _refcount";
> +		bad_reason[nr++] = "nonzero _refcount";
>  	if (unlikely(page->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[nr++] = "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";
> +		bad_reason[nr++] = "page still charged to cgroup";
>  #endif
> -	bad_page(page, bad_reason, bad_flags);
> +	bad_page(page, 1, bad_reason, bad_flags);
This should be 'nr' here instead ?

>  }
>  
>  /*
> 

Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[Wei Yang]

On Mon, Jan 20, 2020 at 12:03:20PM +0530, Anshuman Khandual wrote:
>
>
>On 01/20/2020 08:34 AM, Wei Yang wrote:
>> Now we can pass all bad reasons to __dump_page().
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  mm/page_alloc.c | 52 ++++++++++++++++++++++++++-----------------------
>>  1 file changed, 28 insertions(+), 24 deletions(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a43b9d2482f2..a7b793c739fc 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -609,7 +609,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 nr, const char **reason,
>>  		unsigned long bad_flags)
>>  {
>>  	static unsigned long resume;
>> @@ -638,7 +638,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, 1, &reason);
>> +	__dump_page(page, nr, reason);
>>  	bad_flags &= page->flags;
>>  	if (bad_flags)
>>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
>> @@ -1027,27 +1027,25 @@ static inline bool page_expected_state(struct page *page,
>>  
>>  static void free_pages_check_bad(struct page *page)
>>  {
>> -	const char *bad_reason;
>> -	unsigned long bad_flags;
>> -
>> -	bad_reason = NULL;
>> -	bad_flags = 0;
>> +	const char *bad_reason[5];
>
>s/5/NR_BAD_PAGE_REASONS
>
>
>> +	unsigned long bad_flags = 0;
>> +	int nr = 0;
>>  
>>  	if (unlikely(atomic_read(&page->_mapcount) != -1))
>> -		bad_reason = "nonzero mapcount";
>> +		bad_reason[nr++] = "nonzero mapcount";
>>  	if (unlikely(page->mapping != NULL))
>> -		bad_reason = "non-NULL mapping";
>> +		bad_reason[nr++] = "non-NULL mapping";
>>  	if (unlikely(page_ref_count(page) != 0))
>> -		bad_reason = "nonzero _refcount";
>> +		bad_reason[nr++] = "nonzero _refcount";
>>  	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
>> -		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
>> +		bad_reason[nr++] = "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";
>> +		bad_reason[nr++] = "page still charged to cgroup";
>>  #endif
>> -	bad_page(page, bad_reason, bad_flags);
>> +	bad_page(page, nr, bad_reason, bad_flags);
>>  }
>>  
>>  static inline int free_pages_check(struct page *page)
>> @@ -1062,6 +1060,7 @@ static inline int free_pages_check(struct page *page)
>>  
>>  static int free_tail_pages_check(struct page *head_page, struct page *page)
>>  {
>> +	const char *reason;
>>  	int ret = 1;
>>  
>>  	/*
>> @@ -1078,7 +1077,8 @@ 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);
>> +			reason = "nonzero compound_mapcount";
>> +			bad_page(page, 1, &reason, 0);
>>  			goto out;
>>  		}
>>  		break;
>> @@ -1090,17 +1090,20 @@ 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);
>> +			reason = "corrupted mapping in tail page";
>> +			bad_page(page, 1, &reason, 0);
>>  			goto out;
>>  		}
>>  		break;
>>  	}
>>  	if (unlikely(!PageTail(page))) {
>> -		bad_page(page, "PageTail not set", 0);
>> +		reason = "PageTail not set";
>> +		bad_page(page, 1, &reason, 0);
>>  		goto out;
>>  	}
>>  	if (unlikely(compound_head(page) != head_page)) {
>> -		bad_page(page, "compound_head not consistent", 0);
>> +		reason = "compound_head not consistent";
>> +		bad_page(page, 1, &reason, 0);
>>  		goto out;
>>  	}
>>  	ret = 0;
>> @@ -2041,29 +2044,30 @@ static inline void expand(struct zone *zone, struct page *page,
>>  
>>  static void check_new_page_bad(struct page *page)
>>  {
>> -	const char *bad_reason = NULL;
>> +	const char *bad_reason[5];
>
>s/5/NR_BAD_PAGE_REASONS 
>
>>  	unsigned long bad_flags = 0;
>> +	int nr = 0;
>>  
>>  	if (unlikely(atomic_read(&page->_mapcount) != -1))
>> -		bad_reason = "nonzero mapcount";
>> +		bad_reason[nr++] = "nonzero mapcount";
>>  	if (unlikely(page->mapping != NULL))
>> -		bad_reason = "non-NULL mapping";
>> +		bad_reason[nr++] = "non-NULL mapping";
>>  	if (unlikely(page_ref_count(page) != 0))
>> -		bad_reason = "nonzero _refcount";
>> +		bad_reason[nr++] = "nonzero _refcount";
>>  	if (unlikely(page->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[nr++] = "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";
>> +		bad_reason[nr++] = "page still charged to cgroup";
>>  #endif
>> -	bad_page(page, bad_reason, bad_flags);
>> +	bad_page(page, 1, bad_reason, bad_flags);
>This should be 'nr' here instead ?
>

Thanks, I missed this one.

>>  }
>>  
>>  /*
>> 

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[Michal Hocko]

On Mon 20-01-20 11:04:14, Wei Yang wrote:
> Now we can pass all bad reasons to __dump_page().

And we do we want to do that? The dump of the page will tell us the
whole story so a single and the most important reason sounds like a
better implementation. The code is also more subtle because each caller
of the function has to be aware of how many reasons there might be.
Not to mention that you need a room for 5 pointers on the stack and this
and page allocator might be called from deeper call chains.

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  mm/page_alloc.c | 52 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a43b9d2482f2..a7b793c739fc 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -609,7 +609,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 nr, const char **reason,
>  		unsigned long bad_flags)
>  {
>  	static unsigned long resume;
> @@ -638,7 +638,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, 1, &reason);
> +	__dump_page(page, nr, reason);
>  	bad_flags &= page->flags;
>  	if (bad_flags)
>  		pr_alert("bad because of flags: %#lx(%pGp)\n",
> @@ -1027,27 +1027,25 @@ static inline bool page_expected_state(struct page *page,
>  
>  static void free_pages_check_bad(struct page *page)
>  {
> -	const char *bad_reason;
> -	unsigned long bad_flags;
> -
> -	bad_reason = NULL;
> -	bad_flags = 0;
> +	const char *bad_reason[5];
> +	unsigned long bad_flags = 0;
> +	int nr = 0;
>  
>  	if (unlikely(atomic_read(&page->_mapcount) != -1))
> -		bad_reason = "nonzero mapcount";
> +		bad_reason[nr++] = "nonzero mapcount";
>  	if (unlikely(page->mapping != NULL))
> -		bad_reason = "non-NULL mapping";
> +		bad_reason[nr++] = "non-NULL mapping";
>  	if (unlikely(page_ref_count(page) != 0))
> -		bad_reason = "nonzero _refcount";
> +		bad_reason[nr++] = "nonzero _refcount";
>  	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
> -		bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
> +		bad_reason[nr++] = "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";
> +		bad_reason[nr++] = "page still charged to cgroup";
>  #endif
> -	bad_page(page, bad_reason, bad_flags);
> +	bad_page(page, nr, bad_reason, bad_flags);
>  }
>  
>  static inline int free_pages_check(struct page *page)
> @@ -1062,6 +1060,7 @@ static inline int free_pages_check(struct page *page)
>  
>  static int free_tail_pages_check(struct page *head_page, struct page *page)
>  {
> +	const char *reason;
>  	int ret = 1;
>  
>  	/*
> @@ -1078,7 +1077,8 @@ 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);
> +			reason = "nonzero compound_mapcount";
> +			bad_page(page, 1, &reason, 0);
>  			goto out;
>  		}
>  		break;
> @@ -1090,17 +1090,20 @@ 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);
> +			reason = "corrupted mapping in tail page";
> +			bad_page(page, 1, &reason, 0);
>  			goto out;
>  		}
>  		break;
>  	}
>  	if (unlikely(!PageTail(page))) {
> -		bad_page(page, "PageTail not set", 0);
> +		reason = "PageTail not set";
> +		bad_page(page, 1, &reason, 0);
>  		goto out;
>  	}
>  	if (unlikely(compound_head(page) != head_page)) {
> -		bad_page(page, "compound_head not consistent", 0);
> +		reason = "compound_head not consistent";
> +		bad_page(page, 1, &reason, 0);
>  		goto out;
>  	}
>  	ret = 0;
> @@ -2041,29 +2044,30 @@ static inline void expand(struct zone *zone, struct page *page,
>  
>  static void check_new_page_bad(struct page *page)
>  {
> -	const char *bad_reason = NULL;
> +	const char *bad_reason[5];
>  	unsigned long bad_flags = 0;
> +	int nr = 0;
>  
>  	if (unlikely(atomic_read(&page->_mapcount) != -1))
> -		bad_reason = "nonzero mapcount";
> +		bad_reason[nr++] = "nonzero mapcount";
>  	if (unlikely(page->mapping != NULL))
> -		bad_reason = "non-NULL mapping";
> +		bad_reason[nr++] = "non-NULL mapping";
>  	if (unlikely(page_ref_count(page) != 0))
> -		bad_reason = "nonzero _refcount";
> +		bad_reason[nr++] = "nonzero _refcount";
>  	if (unlikely(page->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[nr++] = "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";
> +		bad_reason[nr++] = "page still charged to cgroup";
>  #endif
> -	bad_page(page, bad_reason, bad_flags);
> +	bad_page(page, 1, bad_reason, bad_flags);
>  }
>  
>  /*
> -- 
> 2.17.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[David Hildenbrand]

On 20.01.20 11:22, Michal Hocko wrote:
> On Mon 20-01-20 11:04:14, Wei Yang wrote:
>> Now we can pass all bad reasons to __dump_page().
> 
> And we do we want to do that? The dump of the page will tell us the
> whole story so a single and the most important reason sounds like a
> better implementation. The code is also more subtle because each caller
> of the function has to be aware of how many reasons there might be.
> Not to mention that you need a room for 5 pointers on the stack and this
> and page allocator might be called from deeper call chains.
> 

+1, I don't think we want/need this


-- 
Thanks,

David / dhildenb

Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[Wei Yang]

On Mon, Jan 20, 2020 at 01:19:10PM +0100, David Hildenbrand wrote:
>On 20.01.20 11:22, Michal Hocko wrote:
>> On Mon 20-01-20 11:04:14, Wei Yang wrote:
>>> Now we can pass all bad reasons to __dump_page().
>> 
>> And we do we want to do that? The dump of the page will tell us the
>> whole story so a single and the most important reason sounds like a
>> better implementation. The code is also more subtle because each caller
>> of the function has to be aware of how many reasons there might be.
>> Not to mention that you need a room for 5 pointers on the stack and this
>> and page allocator might be called from deeper call chains.
>> 
>
>+1, I don't think we want/need this
>

Well, I am fine with both.

Sounds we have 2 vs 2 voting :-)

>
>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[Anshuman Khandual]

On 01/20/2020 03:52 PM, Michal Hocko wrote:
> On Mon 20-01-20 11:04:14, Wei Yang wrote:
>> Now we can pass all bad reasons to __dump_page().
> And we do we want to do that? The dump of the page will tell us the
> whole story so a single and the most important reason sounds like a
> better implementation. The code is also more subtle because each caller
> of the function has to be aware of how many reasons there might be.
> Not to mention that you need a room for 5 pointers on the stack and this
> and page allocator might be called from deeper call chains.
> 

Two paths which lead to __dump_page(), dump_page() and bad_page().
Callers of dump_page() can give a single reason what they consider the
most important which leads to page dumping. This makes sense but gets
trickier in bad_page() path. At present, free_pages_check_bad() and
check_new_page_bad() has a sequence of 'if' statements which decides
"most important" reason for __dump_page() without much rationale and
similar in case of free_tail_pages_check() as well. As all information
about the page for corresponding reasons are printed with __dump_page()
anyways, do free_pages_check_bad() or check_new_page_bad() really need
to provide any particular single reason ?

Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()

[Michal Hocko]

On Tue 21-01-20 11:38:29, Anshuman Khandual wrote:
> 
> 
> On 01/20/2020 03:52 PM, Michal Hocko wrote:
> > On Mon 20-01-20 11:04:14, Wei Yang wrote:
> >> Now we can pass all bad reasons to __dump_page().
> > And we do we want to do that? The dump of the page will tell us the
> > whole story so a single and the most important reason sounds like a
> > better implementation. The code is also more subtle because each caller
> > of the function has to be aware of how many reasons there might be.
> > Not to mention that you need a room for 5 pointers on the stack and this
> > and page allocator might be called from deeper call chains.
> > 
> 
> Two paths which lead to __dump_page(), dump_page() and bad_page().
> Callers of dump_page() can give a single reason what they consider the
> most important which leads to page dumping. This makes sense but gets
> trickier in bad_page() path. At present, free_pages_check_bad() and
> check_new_page_bad() has a sequence of 'if' statements which decides
> "most important" reason for __dump_page() without much rationale and
> similar in case of free_tail_pages_check() as well. As all information
> about the page for corresponding reasons are printed with __dump_page()
> anyways, do free_pages_check_bad() or check_new_page_bad() really need
> to provide any particular single reason ?

Do you see any particular problem with the existing logic? I find a
single reason sufficient and a good lead for what to check most of the
time.
-- 
Michal Hocko
SUSE Labs

[Patch v2 4/4] mm/page_alloc.c: extract commom part to check page

[Wei Yang]

During free and new page, we did some check on the status of page
struct. There is some common part, just extract them.

Besides this, this patch also rename two functions to keep the name
convention, since free_pages_check_bad/free_pages_check are counterparts
of check_new_page_bad/check_new_page.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 mm/page_alloc.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a7b793c739fc..7f23cc836f90 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1025,36 +1025,44 @@ static inline bool page_expected_state(struct page *page,
 	return true;
 }
 
-static void free_pages_check_bad(struct page *page)
+static inline int __check_page(struct page *page, int nr,
+				const char **bad_reason)
 {
-	const char *bad_reason[5];
-	unsigned long bad_flags = 0;
-	int nr = 0;
-
 	if (unlikely(atomic_read(&page->_mapcount) != -1))
 		bad_reason[nr++] = "nonzero mapcount";
 	if (unlikely(page->mapping != NULL))
 		bad_reason[nr++] = "non-NULL mapping";
 	if (unlikely(page_ref_count(page) != 0))
 		bad_reason[nr++] = "nonzero _refcount";
-	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
-		bad_reason[nr++] = "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[nr++] = "page still charged to cgroup";
 #endif
+
+	return nr;
+}
+
+static void check_free_page_bad(struct page *page)
+{
+	const char *bad_reason[5];
+	unsigned long bad_flags = 0;
+	int nr = 0;
+
+	nr = __check_page(page, nr, bad_reason);
+	if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) {
+		bad_reason[nr++] = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set";
+		bad_flags = PAGE_FLAGS_CHECK_AT_FREE;
+	}
 	bad_page(page, nr, 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;
 
 	/* Something has gone sideways, find it */
-	free_pages_check_bad(page);
+	check_free_page_bad(page);
 	return 1;
 }
 
@@ -1145,7 +1153,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;
 			}
@@ -1157,7 +1165,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;
 
@@ -1204,7 +1212,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;
 }
@@ -1225,7 +1233,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 */
 
@@ -2048,12 +2056,7 @@ static void check_new_page_bad(struct page *page)
 	unsigned long bad_flags = 0;
 	int nr = 0;
 
-	if (unlikely(atomic_read(&page->_mapcount) != -1))
-		bad_reason[nr++] = "nonzero mapcount";
-	if (unlikely(page->mapping != NULL))
-		bad_reason[nr++] = "non-NULL mapping";
-	if (unlikely(page_ref_count(page) != 0))
-		bad_reason[nr++] = "nonzero _refcount";
+	nr = __check_page(page, nr, bad_reason);
 	if (unlikely(page->flags & __PG_HWPOISON)) {
 		/* Don't complain about hwpoisoned pages */
 		page_mapcount_reset(page); /* remove PageBuddy */
@@ -2063,10 +2066,6 @@ static void check_new_page_bad(struct page *page)
 		bad_reason[nr++] = "PAGE_FLAGS_CHECK_AT_PREP flag set";
 		bad_flags = PAGE_FLAGS_CHECK_AT_PREP;
 	}
-#ifdef CONFIG_MEMCG
-	if (unlikely(page->mem_cgroup))
-		bad_reason[nr++] = "page still charged to cgroup";
-#endif
 	bad_page(page, 1, bad_reason, bad_flags);
 }
 
-- 
2.17.1

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

[Anshuman Khandual]

On 01/20/2020 08:34 AM, Wei Yang wrote:
> During free and new page, we did some check on the status of page
> struct. There is some common part, just extract them.

Makes sense.

> 
> Besides this, this patch also rename two functions to keep the name
> convention, since free_pages_check_bad/free_pages_check are counterparts
> of check_new_page_bad/check_new_page.

This probably should be in a different patch.

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  mm/page_alloc.c | 49 ++++++++++++++++++++++++-------------------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a7b793c739fc..7f23cc836f90 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1025,36 +1025,44 @@ static inline bool page_expected_state(struct page *page,
>  	return true;
>  }
>  
> -static void free_pages_check_bad(struct page *page)
> +static inline int __check_page(struct page *page, int nr,
> +				const char **bad_reason)

free and new page checks are in and out of the buddy allocator, hence
this common factored function should have a more relevant name.

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

[Wei Yang]

On Mon, Jan 20, 2020 at 12:13:38PM +0530, Anshuman Khandual wrote:
>
>
>On 01/20/2020 08:34 AM, Wei Yang wrote:
>> During free and new page, we did some check on the status of page
>> struct. There is some common part, just extract them.
>
>Makes sense.
>
>> 
>> Besides this, this patch also rename two functions to keep the name
>> convention, since free_pages_check_bad/free_pages_check are counterparts
>> of check_new_page_bad/check_new_page.
>
>This probably should be in a different patch.
>

In v1, they are in two separate patches. While David Suggest to merge it.

I am not sure whether my understanding is correct.

>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  mm/page_alloc.c | 49 ++++++++++++++++++++++++-------------------------
>>  1 file changed, 24 insertions(+), 25 deletions(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a7b793c739fc..7f23cc836f90 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1025,36 +1025,44 @@ static inline bool page_expected_state(struct page *page,
>>  	return true;
>>  }
>>  
>> -static void free_pages_check_bad(struct page *page)
>> +static inline int __check_page(struct page *page, int nr,
>> +				const char **bad_reason)
>
>free and new page checks are in and out of the buddy allocator, hence
>this common factored function should have a more relevant name.

Hmm... naming is really difficult. Do you have any suggestion?

-- 
Wei Yang
Help you, Help me

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

[Anshuman Khandual]

On 01/20/2020 06:06 PM, Wei Yang wrote:
> On Mon, Jan 20, 2020 at 12:13:38PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 01/20/2020 08:34 AM, Wei Yang wrote:
>>> During free and new page, we did some check on the status of page
>>> struct. There is some common part, just extract them.
>>
>> Makes sense.
>>
>>>
>>> Besides this, this patch also rename two functions to keep the name
>>> convention, since free_pages_check_bad/free_pages_check are counterparts
>>> of check_new_page_bad/check_new_page.
>>
>> This probably should be in a different patch.
>>
> 
> In v1, they are in two separate patches. While David Suggest to merge it.
> 
> I am not sure whether my understanding is correct.

Keeping code refactoring and renaming separate is always better
but its okay, will leave it upto you.

> 
>>>
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>>  mm/page_alloc.c | 49 ++++++++++++++++++++++++-------------------------
>>>  1 file changed, 24 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index a7b793c739fc..7f23cc836f90 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -1025,36 +1025,44 @@ static inline bool page_expected_state(struct page *page,
>>>  	return true;
>>>  }
>>>  
>>> -static void free_pages_check_bad(struct page *page)
>>> +static inline int __check_page(struct page *page, int nr,
>>> +				const char **bad_reason)
>>
>> free and new page checks are in and out of the buddy allocator, hence
>> this common factored function should have a more relevant name.
> 
> Hmm... naming is really difficult. Do you have any suggestion?
> 

Probably something like bad_page_fetch_reasons() and also this helper
can be expanded to include an additional argument 'bool free' which
can test either PAGE_FLAGS_CHECK_AT_FREE or PAGE_FLAGS_CHECK_AT_PREP.
This way bad_page_fetch_reasons() is where all possible reasons are
evaluated including page->flags based and then final 'int nr' can be
returned once and for all.

bad_flags does not seem to be needed. Wondering what it adds more
in bad_page() when page->flags gets printed universally through
__dump_page(). In case it is still required, it can be derived
from 'bad_reasons' evaluated in bad_page_fetch_reasons().

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

[Wei Yang]

On Tue, Jan 21, 2020 at 10:19:38AM +0530, Anshuman Khandual wrote:
>
>
>On 01/20/2020 06:06 PM, Wei Yang wrote:
>> On Mon, Jan 20, 2020 at 12:13:38PM +0530, Anshuman Khandual wrote:
>>>
>>>
>>> On 01/20/2020 08:34 AM, Wei Yang wrote:
>>>> During free and new page, we did some check on the status of page
>>>> struct. There is some common part, just extract them.
>>>
>>> Makes sense.
>>>
>>>>
>>>> Besides this, this patch also rename two functions to keep the name
>>>> convention, since free_pages_check_bad/free_pages_check are counterparts
>>>> of check_new_page_bad/check_new_page.
>>>
>>> This probably should be in a different patch.
>>>
>> 
>> In v1, they are in two separate patches. While David Suggest to merge it.
>> 
>> I am not sure whether my understanding is correct.
>
>Keeping code refactoring and renaming separate is always better
>but its okay, will leave it upto you.
>

Agree with you :-)

Maybe I misunderstand David. Will split it in next version.

-- 
Wei Yang
Help you, Help me