| Message ID | 20250321011304.549908-1-ye.liu@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/page_alloc: Consolidate unlikely handling in page_expected_state | expand |
On 3/21/25 06:43, Ye Liu wrote: > From: Ye Liu <liuye@kylinos.cn> > > This patch consolidates the handling of unlikely conditions in the > page_expected_state function, reducing code duplication and improving > readability. > > Previously, the check_new_page_bad function contained logic to handle > __PG_HWPOISON flags, which was called from check_new_page. This patch > moves the handling of __PG_HWPOISON flags into the page_expected_state > function and removes the check_new_page_bad function. The check_new_page > function now directly calls bad_page if the page has unexpected flags. > > This change simplifies the code by reducing the number of functions and > centralizing the unlikely condition handling in one place. > > Signed-off-by: Ye Liu <liuye@kylinos.cn> > --- > mm/page_alloc.c | 26 ++++++++++---------------- > 1 file changed, 10 insertions(+), 16 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 2842da893eea..d8d04ac1d709 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -892,6 +892,13 @@ static inline bool page_expected_state(struct page *page, > if (unlikely(atomic_read(&page->_mapcount) != -1)) > return false; > > + if (unlikely(PageHWPoison(page))) { > + /* Don't complain about hwpoisoned pages */ > + if (PageBuddy(page)) > + __ClearPageBuddy(page); > + return false; Should this be return 'true' instead ? Let's consider a scenario where PageHWPoison(page) is true. Previously bad_page() was not getting called as check_new_page_bad() will return earlier before reaching bad_page(). But now with the proposed change here page_expected_state() returns false and hence bad_page() still gets called later on in check_new_page(). There is a change in behaviour - or am I missing something here ? > + } > + > if (unlikely((unsigned long)page->mapping | > page_ref_count(page) | > #ifdef CONFIG_MEMCG > @@ -1586,29 +1593,16 @@ static __always_inline void page_del_and_expand(struct zone *zone, > account_freepages(zone, -nr_pages, migratetype); > } > > -static void check_new_page_bad(struct page *page) > -{ > - if (unlikely(PageHWPoison(page))) { > - /* Don't complain about hwpoisoned pages */ > - if (PageBuddy(page)) > - __ClearPageBuddy(page); > - return; > - } > - > - bad_page(page, > - page_bad_reason(page, PAGE_FLAGS_CHECK_AT_PREP)); > -} > - > /* > * This page is about to be returned from the page allocator > */ > static bool check_new_page(struct page *page) > { > - if (likely(page_expected_state(page, > - PAGE_FLAGS_CHECK_AT_PREP|__PG_HWPOISON))) > + if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_PREP))) > return false; > > - check_new_page_bad(page); > + bad_page(page, > + page_bad_reason(page, PAGE_FLAGS_CHECK_AT_PREP)); > return true; > } >
在 2025/3/21 13:40, Anshuman Khandual 写道: > > On 3/21/25 06:43, Ye Liu wrote: >> From: Ye Liu <liuye@kylinos.cn> >> >> This patch consolidates the handling of unlikely conditions in the >> page_expected_state function, reducing code duplication and improving >> readability. >> >> Previously, the check_new_page_bad function contained logic to handle >> __PG_HWPOISON flags, which was called from check_new_page. This patch >> moves the handling of __PG_HWPOISON flags into the page_expected_state >> function and removes the check_new_page_bad function. The check_new_page >> function now directly calls bad_page if the page has unexpected flags. >> >> This change simplifies the code by reducing the number of functions and >> centralizing the unlikely condition handling in one place. >> >> Signed-off-by: Ye Liu <liuye@kylinos.cn> >> --- >> mm/page_alloc.c | 26 ++++++++++---------------- >> 1 file changed, 10 insertions(+), 16 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index 2842da893eea..d8d04ac1d709 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -892,6 +892,13 @@ static inline bool page_expected_state(struct page *page, >> if (unlikely(atomic_read(&page->_mapcount) != -1)) >> return false; >> >> + if (unlikely(PageHWPoison(page))) { >> + /* Don't complain about hwpoisoned pages */ >> + if (PageBuddy(page)) >> + __ClearPageBuddy(page); >> + return false; > Should this be return 'true' instead ? > > Let's consider a scenario where PageHWPoison(page) is true. > > Previously bad_page() was not getting called as check_new_page_bad() will > return earlier before reaching bad_page(). > > But now with the proposed change here page_expected_state() returns false > and hence bad_page() still gets called later on in check_new_page(). > > There is a change in behaviour - or am I missing something here ? Thank you for the suggestion. You're right, it makes sense to return true in this case. I'll update the patch accordingly. Appreciate your feedback! > >> + } >> + >> if (unlikely((unsigned long)page->mapping | >> page_ref_count(page) | >> #ifdef CONFIG_MEMCG >> @@ -1586,29 +1593,16 @@ static __always_inline void page_del_and_expand(struct zone *zone, >> account_freepages(zone, -nr_pages, migratetype); >> } >> >> -static void check_new_page_bad(struct page *page) >> -{ >> - if (unlikely(PageHWPoison(page))) { >> - /* Don't complain about hwpoisoned pages */ >> - if (PageBuddy(page)) >> - __ClearPageBuddy(page); >> - return; >> - } >> - >> - bad_page(page, >> - page_bad_reason(page, PAGE_FLAGS_CHECK_AT_PREP)); >> -} >> - >> /* >> * This page is about to be returned from the page allocator >> */ >> static bool check_new_page(struct page *page) >> { >> - if (likely(page_expected_state(page, >> - PAGE_FLAGS_CHECK_AT_PREP|__PG_HWPOISON))) >> + if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_PREP))) >> return false; >> >> - check_new_page_bad(page); >> + bad_page(page, >> + page_bad_reason(page, PAGE_FLAGS_CHECK_AT_PREP)); >> return true; >> } >>
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 2842da893eea..d8d04ac1d709 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -892,6 +892,13 @@ static inline bool page_expected_state(struct page *page, if (unlikely(atomic_read(&page->_mapcount) != -1)) return false; + if (unlikely(PageHWPoison(page))) { + /* Don't complain about hwpoisoned pages */ + if (PageBuddy(page)) + __ClearPageBuddy(page); + return false; + } + if (unlikely((unsigned long)page->mapping | page_ref_count(page) | #ifdef CONFIG_MEMCG @@ -1586,29 +1593,16 @@ static __always_inline void page_del_and_expand(struct zone *zone, account_freepages(zone, -nr_pages, migratetype); } -static void check_new_page_bad(struct page *page) -{ - if (unlikely(PageHWPoison(page))) { - /* Don't complain about hwpoisoned pages */ - if (PageBuddy(page)) - __ClearPageBuddy(page); - return; - } - - bad_page(page, - page_bad_reason(page, PAGE_FLAGS_CHECK_AT_PREP)); -} - /* * This page is about to be returned from the page allocator */ static bool check_new_page(struct page *page) { - if (likely(page_expected_state(page, - PAGE_FLAGS_CHECK_AT_PREP|__PG_HWPOISON))) + if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_PREP))) return false; - check_new_page_bad(page); + bad_page(page, + page_bad_reason(page, PAGE_FLAGS_CHECK_AT_PREP)); return true; }