| Message ID | 20200119131408.23247-2-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page_alloc.c: cleanup on check page | expand |
On Sun, 19 Jan 2020, Wei Yang wrote: > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index d047bf7d8fd4..8cd06729169f 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1025,13 +1025,9 @@ static inline bool page_expected_state(struct page *page, > return true; > } > > -static void free_pages_check_bad(struct page *page) > +static inline const char *__check_page(struct page *page) > { > - const char *bad_reason; > - unsigned long bad_flags; > - > - bad_reason = NULL; > - bad_flags = 0; > + const char *bad_reason = NULL; > > if (unlikely(atomic_read(&page->_mapcount) != -1)) > bad_reason = "nonzero mapcount"; > @@ -1039,14 +1035,23 @@ static void free_pages_check_bad(struct page *page) > bad_reason = "non-NULL mapping"; > if (unlikely(page_ref_count(page) != 0)) > bad_reason = "nonzero _refcount"; > - if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { > - bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; > - bad_flags = PAGE_FLAGS_CHECK_AT_FREE; > - } > #ifdef CONFIG_MEMCG > if (unlikely(page->mem_cgroup)) > bad_reason = "page still charged to cgroup"; > #endif > + return bad_reason; > +} > + > +static void free_pages_check_bad(struct page *page) > +{ > + const char *bad_reason = NULL; > + unsigned long bad_flags = 0; > + > + bad_reason = __check_page(page); > + if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { > + bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; > + bad_flags = PAGE_FLAGS_CHECK_AT_FREE; > + } > bad_page(page, bad_reason, bad_flags); > } > > @@ -2044,12 +2049,7 @@ static void check_new_page_bad(struct page *page) > const char *bad_reason = NULL; > unsigned long bad_flags = 0; > > - if (unlikely(atomic_read(&page->_mapcount) != -1)) > - bad_reason = "nonzero mapcount"; > - if (unlikely(page->mapping != NULL)) > - bad_reason = "non-NULL mapping"; > - if (unlikely(page_ref_count(page) != 0)) > - bad_reason = "nonzero _refcount"; > + bad_reason = __check_page(page); > if (unlikely(page->flags & __PG_HWPOISON)) { > bad_reason = "HWPoisoned (hardware-corrupted)"; > bad_flags = __PG_HWPOISON; > @@ -2061,10 +2061,6 @@ static void check_new_page_bad(struct page *page) > bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set"; > bad_flags = PAGE_FLAGS_CHECK_AT_PREP; > } > -#ifdef CONFIG_MEMCG > - if (unlikely(page->mem_cgroup)) > - bad_reason = "page still charged to cgroup"; > -#endif > bad_page(page, bad_reason, bad_flags); > } > I think this is compounding a previous problem in these functions: these are all "if" clauses, not "else if" clauses so they are presumably ordered based on least significant to most significant (we only see the last bad_reason that we find). For the page->mem_cgroup check, this leaves bad_flags set but it doesn't match bad_reason. Could you instead fix the problem with these functions so that we actually list *all* the problems with the page rather than only the last conditional that is true?
On Sun, Jan 19, 2020 at 02:06:18PM -0800, David Rientjes wrote: >On Sun, 19 Jan 2020, Wei Yang wrote: > >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index d047bf7d8fd4..8cd06729169f 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -1025,13 +1025,9 @@ static inline bool page_expected_state(struct page *page, >> return true; >> } >> >> -static void free_pages_check_bad(struct page *page) >> +static inline const char *__check_page(struct page *page) >> { >> - const char *bad_reason; >> - unsigned long bad_flags; >> - >> - bad_reason = NULL; >> - bad_flags = 0; >> + const char *bad_reason = NULL; >> >> if (unlikely(atomic_read(&page->_mapcount) != -1)) >> bad_reason = "nonzero mapcount"; >> @@ -1039,14 +1035,23 @@ static void free_pages_check_bad(struct page *page) >> bad_reason = "non-NULL mapping"; >> if (unlikely(page_ref_count(page) != 0)) >> bad_reason = "nonzero _refcount"; >> - if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { >> - bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; >> - bad_flags = PAGE_FLAGS_CHECK_AT_FREE; >> - } >> #ifdef CONFIG_MEMCG >> if (unlikely(page->mem_cgroup)) >> bad_reason = "page still charged to cgroup"; >> #endif >> + return bad_reason; >> +} >> + >> +static void free_pages_check_bad(struct page *page) >> +{ >> + const char *bad_reason = NULL; >> + unsigned long bad_flags = 0; >> + >> + bad_reason = __check_page(page); >> + if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { >> + bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; >> + bad_flags = PAGE_FLAGS_CHECK_AT_FREE; >> + } >> bad_page(page, bad_reason, bad_flags); >> } >> >> @@ -2044,12 +2049,7 @@ static void check_new_page_bad(struct page *page) >> const char *bad_reason = NULL; >> unsigned long bad_flags = 0; >> >> - if (unlikely(atomic_read(&page->_mapcount) != -1)) >> - bad_reason = "nonzero mapcount"; >> - if (unlikely(page->mapping != NULL)) >> - bad_reason = "non-NULL mapping"; >> - if (unlikely(page_ref_count(page) != 0)) >> - bad_reason = "nonzero _refcount"; >> + bad_reason = __check_page(page); >> if (unlikely(page->flags & __PG_HWPOISON)) { >> bad_reason = "HWPoisoned (hardware-corrupted)"; >> bad_flags = __PG_HWPOISON; >> @@ -2061,10 +2061,6 @@ static void check_new_page_bad(struct page *page) >> bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set"; >> bad_flags = PAGE_FLAGS_CHECK_AT_PREP; >> } >> -#ifdef CONFIG_MEMCG >> - if (unlikely(page->mem_cgroup)) >> - bad_reason = "page still charged to cgroup"; >> -#endif >> bad_page(page, bad_reason, bad_flags); >> } >> > >I think this is compounding a previous problem in these functions: these >are all "if" clauses, not "else if" clauses so they are presumably ordered >based on least significant to most significant (we only see the last >bad_reason that we find). For the page->mem_cgroup check, this leaves >bad_flags set but it doesn't match bad_reason. > I have thought about this. And curious about the order of those reasons. >Could you instead fix the problem with these functions so that we actually >list *all* the problems with the page rather than only the last >conditional that is true? Sure, thanks for the suggestion.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index d047bf7d8fd4..8cd06729169f 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -1025,13 +1025,9 @@ static inline bool page_expected_state(struct page *page, return true; } -static void free_pages_check_bad(struct page *page) +static inline const char *__check_page(struct page *page) { - const char *bad_reason; - unsigned long bad_flags; - - bad_reason = NULL; - bad_flags = 0; + const char *bad_reason = NULL; if (unlikely(atomic_read(&page->_mapcount) != -1)) bad_reason = "nonzero mapcount"; @@ -1039,14 +1035,23 @@ static void free_pages_check_bad(struct page *page) bad_reason = "non-NULL mapping"; if (unlikely(page_ref_count(page) != 0)) bad_reason = "nonzero _refcount"; - if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { - bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; - bad_flags = PAGE_FLAGS_CHECK_AT_FREE; - } #ifdef CONFIG_MEMCG if (unlikely(page->mem_cgroup)) bad_reason = "page still charged to cgroup"; #endif + return bad_reason; +} + +static void free_pages_check_bad(struct page *page) +{ + const char *bad_reason = NULL; + unsigned long bad_flags = 0; + + bad_reason = __check_page(page); + if (unlikely(page->flags & PAGE_FLAGS_CHECK_AT_FREE)) { + bad_reason = "PAGE_FLAGS_CHECK_AT_FREE flag(s) set"; + bad_flags = PAGE_FLAGS_CHECK_AT_FREE; + } bad_page(page, bad_reason, bad_flags); } @@ -2044,12 +2049,7 @@ static void check_new_page_bad(struct page *page) const char *bad_reason = NULL; unsigned long bad_flags = 0; - if (unlikely(atomic_read(&page->_mapcount) != -1)) - bad_reason = "nonzero mapcount"; - if (unlikely(page->mapping != NULL)) - bad_reason = "non-NULL mapping"; - if (unlikely(page_ref_count(page) != 0)) - bad_reason = "nonzero _refcount"; + bad_reason = __check_page(page); if (unlikely(page->flags & __PG_HWPOISON)) { bad_reason = "HWPoisoned (hardware-corrupted)"; bad_flags = __PG_HWPOISON; @@ -2061,10 +2061,6 @@ static void check_new_page_bad(struct page *page) bad_reason = "PAGE_FLAGS_CHECK_AT_PREP flag set"; bad_flags = PAGE_FLAGS_CHECK_AT_PREP; } -#ifdef CONFIG_MEMCG - if (unlikely(page->mem_cgroup)) - bad_reason = "page still charged to cgroup"; -#endif bad_page(page, bad_reason, bad_flags); }
During free and new page, we did some check on the status of page struct. There is some common part, just extract them. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- mm/page_alloc.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-)