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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> ---
>>> 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().
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