mm/page_alloc.c: cleanup on check page
Subsystems (3)
v2 cover mm/page_alloc.c: cleanup on check page
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(-)
Signature · 1 line
2.17.1
PATCH v2 1/4 mm: enable dump several reasons for __dump_page()
This is a preparation to dump all reasons during check page.
--- include/linux/mmdebug.h | 2 +- mm/debug.c | 11 ++++++----- mm/page_alloc.c | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-)
include/linux/mmdebug.h +1 -1
@@ -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);
mm/debug.c +6 -5
@@ -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);
mm/page_alloc.c +1 -1
@@ -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",
Signature · 1 line
2.17.1
Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()
Quoted 1 line · Wei Yang
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.
Quoted 40 lines
Signed-off-by Wei Yang <[email protected]>--- include/linux/mmdebug.h | 2 +- mm/debug.c | 11 ++++++----- mm/page_alloc.c | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-)include/linux/mmdebug.h +1
-1@@ -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);mm/debug.c +2
-2@@ -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
mm/debug.c void __dump_page(struct page *page, const char *reason) · line 102
Quoted 8 lines
mm/debug.c +3
-2@@ -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)
mm/debug.c void __dump_page(struct page *page, const char *reason) · line 105
Quoted 3 lines
mm/debug.c +0
-0#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 ?
Quoted 15 lines
mm/debug.c +1
-1@@ -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);mm/page_alloc.c +0
-0
mm/page_alloc.c static void bad_page(struct page *page, const char *reason, · line 644
Quoted 8 lines
mm/page_alloc.c +1
-1@@ -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()
Quoted 62 lines · Anshuman Khandual
Quoted 1 line · Wei Yang
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.
Quoted 40 lines
Signed-off-by Wei Yang <[email protected]>--- include/linux/mmdebug.h | 2 +- mm/debug.c | 11 ++++++----- mm/page_alloc.c | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-)include/linux/mmdebug.h +1
-1@@ -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);mm/debug.c +2
-2@@ -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 mm/debug.c void __dump_page(struct page *page, const char *reason) · line 102
Quoted 8 lines
mm/debug.c +3
-2@@ -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_REASONSor 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.
Quoted 2 lines
split_huge_page_to_list()->dump_page(head, NULL)
You are right, I missed this case.
Quoted 7 lines
mm/debug.c void __dump_page(struct page *page, const char *reason) · line 105
Quoted 3 lines
mm/debug.c +0
-0#ifdef CONFIG_MEMCG if (!page_poisoned && page->mem_cgroup)While here, will it be better to move the above debug print block after
mem_cgroupblock instead ?
Not sure, let's see whether others have some idea.
Quoted 15 lines
Quoted 15 lines
mm/debug.c +1
-1@@ -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);mm/page_alloc.c +0
-0@@ -638,7 +638,7 @@ static void bad_page(struct page *page, const char *reason,
Quoted 15 lines
mm/page_alloc.c static void bad_page(struct page *page, const char *reason, · line 644
Quoted 8 lines
mm/page_alloc.c +1
-1pr_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 printpage->flagsunconditionally each time and let the user decipher it instead.__dump_page()will printpage->flagsfor each case (atleast after the new patch from Vlastimil). AFAICS, the only place currently consumingbad_flagsisbad_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.
Signature · 2 lines
Wei Yang Help you, Help me
Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()
Quoted 109 lines · Wei Yang
Quoted 62 lines · Anshuman Khandual
Quoted 1 line · Wei Yang
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.
Quoted 40 lines
Signed-off-by Wei Yang <[email protected]>--- include/linux/mmdebug.h | 2 +- mm/debug.c | 11 ++++++----- mm/page_alloc.c | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-)include/linux/mmdebug.h +1
-1@@ -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);mm/debug.c +2
-2@@ -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 mm/debug.c void __dump_page(struct page *page, const char *reason) · line 102
Quoted 8 lines
mm/debug.c +3
-2@@ -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_REASONSor 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.Quoted 2 lines
split_huge_page_to_list()->dump_page(head, NULL)You are right, I missed this case.
Quoted 7 lines
mm/debug.c void __dump_page(struct page *page, const char *reason) · line 105
Quoted 3 lines
mm/debug.c +0
-0#ifdef CONFIG_MEMCG if (!page_poisoned && page->mem_cgroup)While here, will it be better to move the above debug print block after
mem_cgroupblock instead ?Not sure, let's see whether others have some idea.
Quoted 15 lines
Quoted 15 lines
mm/debug.c +1
-1@@ -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);mm/page_alloc.c +0
-0@@ -638,7 +638,7 @@ static void bad_page(struct page *page, const char *reason,Quoted 15 lines
mm/page_alloc.c static void bad_page(struct page *page, const char *reason, · line 644
Quoted 8 lines
mm/page_alloc.c +1
-1pr_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 printpage->flagsunconditionally each time and let the user decipher it instead.__dump_page()will printpage->flagsfor each case (atleast after the new patch from Vlastimil). AFAICS, the only place currently consumingbad_flagsisbad_page()which seems redundant after first calling__dump_page().
Quoted 2 lines
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.
Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()
Quoted 132 lines · Anshuman Khandual
Quoted 109 lines · Wei Yang
Quoted 62 lines · Anshuman Khandual
Quoted 1 line · Wei Yang
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.
Quoted 40 lines
Signed-off-by Wei Yang <[email protected]>--- include/linux/mmdebug.h | 2 +- mm/debug.c | 11 ++++++----- mm/page_alloc.c | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-)include/linux/mmdebug.h +1
-1@@ -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);mm/debug.c +2
-2@@ -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 mm/debug.c void __dump_page(struct page *page, const char *reason) · line 102
Quoted 8 lines
mm/debug.c +3
-2@@ -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_REASONSor 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.Quoted 2 lines
split_huge_page_to_list()->dump_page(head, NULL)You are right, I missed this case.
Quoted 7 lines
mm/debug.c void __dump_page(struct page *page, const char *reason) · line 105
Quoted 3 lines
mm/debug.c +0
-0#ifdef CONFIG_MEMCG if (!page_poisoned && page->mem_cgroup)While here, will it be better to move the above debug print block after
mem_cgroupblock instead ?Not sure, let's see whether others have some idea.
Quoted 15 lines
Quoted 15 lines
mm/debug.c +1
-1@@ -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);mm/page_alloc.c +0
-0@@ -638,7 +638,7 @@ static void bad_page(struct page *page, const char *reason,Quoted 15 lines
mm/page_alloc.c static void bad_page(struct page *page, const char *reason, · line 644
Quoted 8 lines
mm/page_alloc.c +1
-1pr_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 printpage->flagsunconditionally each time and let the user decipher it instead.__dump_page()will printpage->flagsfor each case (atleast after the new patch from Vlastimil). AFAICS, the only place currently consumingbad_flagsisbad_page()which seems redundant after first calling__dump_page().Quoted 2 lines
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()andcheck_new_page_bad()before being passed intobad_page(). All other call sites forbad_page()just pass 0 for 'bad_flags'. Now inbad_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->flagswill always be positive when 'reason' is either"
PAGE_FLAGS_CHECK_AT_FREEflag(s) set"
Quoted 8 lines
or
"
PAGE_FLAGS_CHECK_AT_PREPflag set"The point here is we dont need to print
bad_flagshere as__dump_page()already printspage->flagsuniversally along with the "bad_reason" after the following change.
Thanks, I see your point. It is not necessary to print flags twice.
Quoted 1 line
Signature · 2 lines
Wei Yang Help you, Help me
Re: [Patch v2 1/4] mm: enable dump several reasons for __dump_page()
In reply to Anshuman KhandualQuoted 134 lines · Anshuman Khandual
Quoted 109 lines · Wei Yang
Quoted 62 lines · Anshuman Khandual
Quoted 1 line · Wei Yang
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.
Quoted 40 lines
Signed-off-by Wei Yang <[email protected]>--- include/linux/mmdebug.h | 2 +- mm/debug.c | 11 ++++++----- mm/page_alloc.c | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-)include/linux/mmdebug.h +1
-1@@ -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);mm/debug.c +2
-2@@ -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 mm/debug.c void __dump_page(struct page *page, const char *reason) · line 102
Quoted 8 lines
mm/debug.c +3
-2@@ -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_REASONSor 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.Quoted 2 lines
split_huge_page_to_list()->dump_page(head, NULL)You are right, I missed this case.
Quoted 7 lines
mm/debug.c void __dump_page(struct page *page, const char *reason) · line 105
Quoted 3 lines
mm/debug.c +0
-0#ifdef CONFIG_MEMCG if (!page_poisoned && page->mem_cgroup)While here, will it be better to move the above debug print block after
mem_cgroupblock instead ?Not sure, let's see whether others have some idea.
Quoted 15 lines
Quoted 15 lines
mm/debug.c +1
-1@@ -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);mm/page_alloc.c +0
-0@@ -638,7 +638,7 @@ static void bad_page(struct page *page, const char *reason,Quoted 15 lines
mm/page_alloc.c static void bad_page(struct page *page, const char *reason, · line 644
Quoted 8 lines
mm/page_alloc.c +1
-1pr_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 printpage->flagsunconditionally each time and let the user decipher it instead.__dump_page()will printpage->flagsfor each case (atleast after the new patch from Vlastimil). AFAICS, the only place currently consumingbad_flagsisbad_page()which seems redundant after first calling__dump_page().Quoted 2 lines
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()andcheck_new_page_bad()before being passed intobad_page(). All other call sites forbad_page()just pass 0 for 'bad_flags'. Now inbad_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->flagswill always be positive when 'reason' is either"
PAGE_FLAGS_CHECK_AT_FREEflag(s) set"or
Quoted 7 lines
"
PAGE_FLAGS_CHECK_AT_PREPflag set"The point here is we dont need to print
bad_flagshere as__dump_page()already printspage->flagsuniversally along with the "bad_reason" after the following change.
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?
Signature · 2 lines
Wei Yang Help you, Help me
PATCH v2 2/4 mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison
In reply to Wei YangSince 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")
--- mm/page_alloc.c | 2 -- 1 file changed, 2 deletions(-)
mm/page_alloc.c +0 -2
@@ -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 ;
Signature · 1 line
2.17.1
Re: [Patch v2 2/4] mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison
Quoted 17 lines · Wei Yang
Since function returns directly, bad_[reason|flags] is not used any where.
This is a following cleanup for commit
e570f56cccd21("mm:check_new_page_bad()directly returns in__PG_HWPOISONcase")Signed-off-by Wei Yang <[email protected]>Acked-by David Rientjes <[email protected]>--- mm/page_alloc.c | 2 -- 1 file changed, 2 deletions(-)mm/page_alloc.c +0
-0
mm/page_alloc.c static void check_new_page_bad(struct page *page) · line 2056
Quoted 8 lines
mm/page_alloc.c +0
-2@@ -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.
Quoted 1 line
Re: [Patch v2 2/4] mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison
Quoted 24 lines · Anshuman Khandual
Quoted 21 lines · Wei Yang
Since function returns directly, bad_[reason|flags] is not used any where.
This is a following cleanup for commit
e570f56cccd21("mm:check_new_page_bad()directly returns in__PG_HWPOISONcase")Signed-off-by Wei Yang <[email protected]>Acked-by David Rientjes <[email protected]>--- mm/page_alloc.c | 2 -- 1 file changed, 2 deletions(-)mm/page_alloc.c +0
-1@@ -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)" ;
Quoted 9 lines
mm/page_alloc.c static void check_new_page_bad(struct page *page) · line 2056
Quoted 4 lines
mm/page_alloc.c +0
-1- 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.
Quoted 1 line
Quoted 1 line
Signature · 2 lines
Wei Yang Help you, Help me
Re: [Patch v2 2/4] mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison
In reply to Wei YangQuoted 8 lines · Wei Yang
Since function returns directly, bad_[reason|flags] is not used any where.
This is a following cleanup for commit
e570f56cccd21("mm:check_new_page_bad()directly returns in__PG_HWPOISONcase")Signed-off-by Wei Yang <[email protected]>Acked-by David Rientjes <[email protected]>
This is a left over from loong time ago. AFAICS bad_reason and flags hav never been used.
Quoted 19 lines
--- mm/page_alloc.c | 2 -- 1 file changed, 2 deletions(-)mm/page_alloc.c +0
-2@@ -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 ;Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [Patch v2 2/4] mm/page_alloc.c: bad_[reason|flags] is not necessary when PageHWPoison
In reply to Wei YangQuoted 18 lines · Wei Yang
Since function returns directly, bad_[reason|flags] is not used any where.
This is a following cleanup for commit
e570f56cccd21("mm:check_new_page_bad()directly returns in__PG_HWPOISONcase")Signed-off-by Wei Yang <[email protected]>Acked-by David Rientjes <[email protected]>--- mm/page_alloc.c | 2 -- 1 file changed, 2 deletions(-)mm/page_alloc.c +0
-0
mm/page_alloc.c static void check_new_page_bad(struct page *page) · line 2056
Quoted 8 lines
mm/page_alloc.c +0
-2@@ -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 ;
Signature · 3 lines
Thanks, David / dhildenb
PATCH v2 3/4 mm/page_alloc.c: pass all bad reasons to bad_page()
In reply to Wei YangNow we can pass all bad reasons to __dump_page().
--- mm/page_alloc.c | 52 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 24 deletions(-)
mm/page_alloc.c +28 -24
@@ -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); }/*
Signature · 1 line
2.17.1
Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()
Quoted 30 lines · Wei Yang
Now we can pass all bad reasons to
__dump_page().Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 52 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 24 deletions(-)mm/page_alloc.c +2
-2@@ -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",
mm/page_alloc.c static inline bool page_expected_state(struct page *page, · line 1030
Quoted 9 lines
mm/page_alloc.c +1
-5@@ -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
Quoted 70 lines
mm/page_alloc.c +17
-10+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 ;
mm/page_alloc.c static inline void expand(struct zone *zone, struct page *page, · line 2047
Quoted 5 lines
mm/page_alloc.c +1
-1@@ -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
Quoted 19 lines
mm/page_alloc.c +4
-4unsigned 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" ;
mm/page_alloc.c static inline void expand(struct zone *zone, struct page *page, · line 2070
Quoted 10 lines
mm/page_alloc.c +3
-2+ 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 ?
Quoted 4 lines
mm/page_alloc.c +0
-0}/*
Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()
Quoted 146 lines · Anshuman Khandual
Quoted 30 lines · Wei Yang
Now we can pass all bad reasons to
__dump_page().Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 52 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 24 deletions(-)mm/page_alloc.c +2
-2@@ -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",mm/page_alloc.c static inline bool page_expected_state(struct page *page, · line 1030
Quoted 9 lines
mm/page_alloc.c +1
-5@@ -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
Quoted 70 lines
mm/page_alloc.c +17
-10+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 ;mm/page_alloc.c static inline void expand(struct zone *zone, struct page *page, · line 2047
Quoted 5 lines
mm/page_alloc.c +1
-1@@ -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
Quoted 22 lines
mm/page_alloc.c +5
-4unsigned 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 ; }
Quoted 9 lines
mm/page_alloc.c static inline void expand(struct zone *zone, struct page *page, · line 2070
Quoted 7 lines
mm/page_alloc.c +2
-2#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.
Quoted 4 lines
Quoted 4 lines
mm/page_alloc.c +0
-0}/*
Signature · 2 lines
Wei Yang Help you, Help me
Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()
In reply to Wei YangQuoted 1 line · Wei Yang
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.
Quoted 147 lines
Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 52 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 24 deletions(-)mm/page_alloc.c +28
-24@@ -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); }/* Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()
Quoted 10 lines · Michal Hocko
Quoted 1 line · Wei Yang
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
Signature · 3 lines
Thanks, David / dhildenb
Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()
Quoted 12 lines · David Hildenbrand
Quoted 10 lines · Michal Hocko
Quoted 1 line · Wei Yang
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.
Quoted 2 lines
+1, I don't think we want/need this
Well, I am fine with both.
Sounds we have 2 vs 2 voting :-)
Quoted 5 lines
Signature · 4 lines
-- Thanks, David / dhildenb
Signature · 2 lines
Wei Yang Help you, Help me
Re: [Patch v2 3/4] mm/page_alloc.c: pass all bad reasons to bad_page()
In reply to Michal HockoQuoted 9 lines · Michal Hocko
Quoted 1 line · Wei Yang
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()
Quoted 13 lines · Anshuman Khandual
Quoted 9 lines · Michal Hocko
Quoted 1 line · Wei Yang
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.
Quoted 10 lines
Two paths which lead to
__dump_page(),dump_page()andbad_page(). Callers ofdump_page()can give a single reason what they consider the most important which leads to page dumping. This makes sense but gets trickier inbad_page()path. At present,free_pages_check_bad()andcheck_new_page_bad()has a sequence of 'if' statements which decides "most important" reason for__dump_page()without much rationale and similar in case offree_tail_pages_check()as well. As all information about the page for corresponding reasons are printed with__dump_page()anyways, dofree_pages_check_bad()orcheck_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.
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH v2 4/4 mm/page_alloc.c: extract commom part to check page
In reply to Wei YangDuring 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.
--- mm/page_alloc.c | 49 ++++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 25 deletions(-)
mm/page_alloc.c +24 -25
@@ -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); }
Signature · 1 line
2.17.1
Re: [Patch v2 4/4] mm/page_alloc.c: extract commom part to check page
Quoted 2 lines · 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.
Makes sense.
Quoted 4 lines
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.
Quoted 11 lines
Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 49 ++++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 25 deletions(-)mm/page_alloc.c +0
-0
mm/page_alloc.c static inline bool page_expected_state(struct page *page, · line 1029
Quoted 6 lines
mm/page_alloc.c +2
-1@@ -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
Quoted 6 lines · Anshuman Khandual
Quoted 2 lines · 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.
Quoted 9 lines
Makes sense.
Quoted 4 lines
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.
Quoted 14 lines
Quoted 14 lines
Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 49 ++++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 25 deletions(-)mm/page_alloc.c +0
-0@@ -1025,36 +1025,44 @@ static inline bool page_expected_state(struct page *page,return true; }
Quoted 6 lines
mm/page_alloc.c static inline bool page_expected_state(struct page *page, · line 1029
Quoted 3 lines
mm/page_alloc.c +2
-1-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?
Signature · 2 lines
Wei Yang Help you, Help me
Re: [Patch v2 4/4] mm/page_alloc.c: extract commom part to check page
Quoted 14 lines · Wei Yang
Quoted 13 lines · Anshuman Khandual
Quoted 2 lines · 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.
Makes sense.
Quoted 4 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Signed-off-by Wei Yang <[email protected]>--- mm/page_alloc.c | 49 ++++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 25 deletions(-)mm/page_alloc.c +0
-0@@ -1025,36 +1025,44 @@ static inline bool page_expected_state(struct page *page,return true; }
Quoted 9 lines
Quoted 6 lines
mm/page_alloc.c static inline bool page_expected_state(struct page *page, · line 1029
Quoted 3 lines
mm/page_alloc.c +2
-1-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
Quoted 20 lines · Anshuman Khandual
Quoted 17 lines · Wei Yang
Quoted 15 lines · Anshuman Khandual
Quoted 2 lines · 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.
Makes sense.
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Signature · 2 lines
Wei Yang Help you, Help me