[PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Ye Liu]

From: Ye Liu <liuye@kylinos.cn>

Consolidate the handling of unlikely conditions in the 
page_expected_state() function to reduce code duplication and improve 
readability.

Move the logic for handling __PG_HWPOISON flags from the 
check_new_page_bad() function to the page_expected_state() function, 
and remove check_new_page_bad().

Call bad_page() directly from the check_new_page() function if the page has
unexpected flags.

Simplify the code by reducing the number of functions and centralizing the
handling of unlikely conditions.

Signed-off-by: Ye Liu <liuye@kylinos.cn>
Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

---
V4: Use "imperative mood" to modify the commit and append parentheses 
to all function names.
V3: Delete 'This patch'.
V2: return true instead of false in the PageHWPoison branch.
---
 mm/page_alloc.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2842da893eea..e8b95c6a96c2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -903,6 +903,12 @@ static inline bool page_expected_state(struct page *page,
 			(page->flags & check_flags)))
 		return false;
 
+	if (unlikely(PageHWPoison(page))) {
+		/* Don't complain about hwpoisoned pages */
+		if (PageBuddy(page))
+			__ClearPageBuddy(page);
+	}
+
 	return true;
 }
 
@@ -1586,29 +1592,15 @@ 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;
 }
 
-- 
2.25.1

Re: [PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Markus Elfring]

…
> ---
> V4: … and append parentheses to all function names.

Collateral evolution for summary phrases?


> V3: Delete 'This patch'.
> V2: return true instead of false in the PageHWPoison branch.
> ---
>  mm/page_alloc.c | 24 ++++++++----------------
…

How do you think about to replace repeated marker lines by line breaks?

Regards,
Markus

Re: [PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Matthew Wilcox]

On Fri, Mar 28, 2025 at 09:47:57AM +0800, Ye Liu wrote:
> Consolidate the handling of unlikely conditions in the 
> page_expected_state() function to reduce code duplication and improve 
> readability.

I don't think this is an equivalent transformation.

Please, stop with these tweaky patches to incredibly sensitive core code.
Fix a problem, or leave it alone.  We are primarily short of reviewer
bandwidth.  You could help with that by reviewing other people's patches.
Sending patches of your own just adds to other people's workload.

Re: [PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Ye Liu]

在 2025/3/28 22:29, Matthew Wilcox 写道:
> On Fri, Mar 28, 2025 at 09:47:57AM +0800, Ye Liu wrote:
>> Consolidate the handling of unlikely conditions in the 
>> page_expected_state() function to reduce code duplication and improve 
>> readability.
> I don't think this is an equivalent transformation.
Could you explain it in detail?
> Please, stop with these tweaky patches to incredibly sensitive core code.
> Fix a problem, or leave it alone.  We are primarily short of reviewer
> bandwidth.  You could help with that by reviewing other people's patches.
> Sending patches of your own just adds to other people's workload.
Thank you for your feedback. I understand the sensitivity of core code
and respect the limitations on reviewer bandwidth. However, I believe
that reasonable optimizations should not be rejected solely because
they involve core code. If an improvement enhances performance,
readability, or maintainability without introducing risks, wouldn't
it be worth considering for review?

Regarding the reviewer shortage, I’d be happy to help by reviewing
other patches as well. Could you please share the process for becoming
a reviewer? What are the requirements or steps to get involved?

Additionally, I’d like to clarify that my intention in submitting
patches is to improve the project, not to create unnecessary workload
for others. If there’s a better way for me to contribute, please let
me know I'm more than willing to collaborate.                         


Thanks,
 Ye Liu

Re: [v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Markus Elfring]

> Regarding the reviewer shortage, I’d be happy to help by reviewing
> other patches as well. Could you please share the process for becoming
> a reviewer?

Under which circumstances would you dare to comment information from
other contributors?

How will your development interests evolve further?


>             What are the requirements or steps to get involved?

You can try to improve communication skills as needed.

Regards,
Markus

Re: [v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Ye Liu]

在 2025/3/31 20:21, Markus Elfring 写道:
>> Regarding the reviewer shortage, I’d be happy to help by reviewing
>> other patches as well. Could you please share the process for becoming
>> a reviewer?
> Under which circumstances would you dare to comment information from
> other contributors?
>
> How will your development interests evolve further?
>
>
>>             What are the requirements or steps to get involved?
> You can try to improve communication skills as needed.

I don’t see how I’ve offended anyone.
         -- I was simply asking about the process to become a reviewer.
What exactly is the issue with that?
How did you interpret my message to warrant such a response?

Thanks,
Ye Liu

Re: [v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Markus Elfring]

>>>             What are the requirements or steps to get involved?
>> You can try to improve communication skills as needed.
>
> I don’t see how I’ve offended anyone.

This might not have happened so far.

But some information from communication approaches can occasionally make
participants feel uncomfortable.


>          -- I was simply asking about the process to become a reviewer.
> What exactly is the issue with that?

There needs to be a desire to review selected items.
https://en.wikipedia.org/wiki/Code_review


> How did you interpret my message to warrant such a response?
I would like to help another bit with corresponding clarifications.
You can get further impressions and experiences from published review approaches,
can't you?

Regards,
Markus

Re: [v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Matthew Wilcox]

On Mon, Mar 31, 2025 at 08:51:28PM +0800, Ye Liu wrote:
> 
> 在 2025/3/31 20:21, Markus Elfring 写道:
> >> Regarding the reviewer shortage, I’d be happy to help by reviewing
> >> other patches as well. Could you please share the process for becoming
> >> a reviewer?
> > Under which circumstances would you dare to comment information from
> > other contributors?
> >
> > How will your development interests evolve further?
> >
> >
> >>             What are the requirements or steps to get involved?
> > You can try to improve communication skills as needed.
> 
> I don’t see how I’ve offended anyone.

Just to be clear, you should ignore Markus.  Don't reply to any of his
emails, and ignore the suggestions he makes.  That's what the rest of
us do.  He's been told many times that the way he interacts with people
is inappropriate, but he persists, and there's little we can do about it.

Re: [v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Ye Liu]

在 2025/3/31 23:53, Matthew Wilcox 写道:
> On Mon, Mar 31, 2025 at 08:51:28PM +0800, Ye Liu wrote:
>> 在 2025/3/31 20:21, Markus Elfring 写道:
>>>> Regarding the reviewer shortage, I’d be happy to help by reviewing
>>>> other patches as well. Could you please share the process for becoming
>>>> a reviewer?
>>> Under which circumstances would you dare to comment information from
>>> other contributors?
>>>
>>> How will your development interests evolve further?
>>>
>>>
>>>>             What are the requirements or steps to get involved?
>>> You can try to improve communication skills as needed.
>> I don’t see how I’ve offended anyone.
> Just to be clear, you should ignore Markus.  Don't reply to any of his
> emails, and ignore the suggestions he makes.  That's what the rest of
> us do.  He's been told many times that the way he interacts with people
> is inappropriate, but he persists, and there's little we can do about it.
Thanks for the reminder.

Re: [PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Matthew Wilcox]

On Mon, Mar 31, 2025 at 08:08:01PM +0800, Ye Liu wrote:
> 
> 在 2025/3/28 22:29, Matthew Wilcox 写道:
> > On Fri, Mar 28, 2025 at 09:47:57AM +0800, Ye Liu wrote:
> >> Consolidate the handling of unlikely conditions in the 
> >> page_expected_state() function to reduce code duplication and improve 
> >> readability.
> > I don't think this is an equivalent transformation.
> Could you explain it in detail?

page_expected_state() is called both at free and alloc.  I think
the correct behaviour on encountering a HWPOISON page should be
different at alloc and free, don't you?

> > Please, stop with these tweaky patches to incredibly sensitive core code.
> > Fix a problem, or leave it alone.  We are primarily short of reviewer
> > bandwidth.  You could help with that by reviewing other people's patches.
> > Sending patches of your own just adds to other people's workload.
> Thank you for your feedback. I understand the sensitivity of core code
> and respect the limitations on reviewer bandwidth. However, I believe
> that reasonable optimizations should not be rejected solely because
> they involve core code. If an improvement enhances performance,
> readability, or maintainability without introducing risks, wouldn't
> it be worth considering for review?

If it's a reasonable optimisation, absolutely!  But if it's an
optimisation, it should be accompanied with a benchmark showing an
improvement.  As far as improving readability, I'm not yet convinced
that you have the expertise to make that call.  Every change that is
made invalidates everybody else's mental model of "how this works".
So all changes carry a cost.  Sometimes that cost is worth paying,
other times it isn't.

> Regarding the reviewer shortage, I’d be happy to help by reviewing
> other patches as well. Could you please share the process for becoming
> a reviewer? What are the requirements or steps to get involved?

There is no process!  Choose a patch, read it, think about it.  What
problems might there be with it?  What may have been overlooked?
Is the commit message unclear to you, how could it be improved?
When you're done, send a Reviewed-by: tag (read the kernel process
documents for the full meaning of that tag).

Re: [PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Ye Liu]

在 2025/3/31 23:59, Matthew Wilcox 写道:
> On Mon, Mar 31, 2025 at 08:08:01PM +0800, Ye Liu wrote:
>> 在 2025/3/28 22:29, Matthew Wilcox 写道:
>>> On Fri, Mar 28, 2025 at 09:47:57AM +0800, Ye Liu wrote:
>>>> Consolidate the handling of unlikely conditions in the 
>>>> page_expected_state() function to reduce code duplication and improve 
>>>> readability.
>>> I don't think this is an equivalent transformation.
>> Could you explain it in detail?
> page_expected_state() is called both at free and alloc.  I think
> the correct behaviour on encountering a HWPOISON page should be
> different at alloc and free, don't you?
In the alloc process, this patch does not modify the code behavior.
Regarding the free process, the if (unlikely(PageHWPoison(page)) && !order)
code handles the case where order is 0. When order is not 0, it does not
matter if __ClearPageBuddy is used to process the last page of the compound
page, because page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; will also clear it.
Is that right?                                                             

>>> Please, stop with these tweaky patches to incredibly sensitive core code.
>>> Fix a problem, or leave it alone.  We are primarily short of reviewer
>>> bandwidth.  You could help with that by reviewing other people's patches.
>>> Sending patches of your own just adds to other people's workload.
>> Thank you for your feedback. I understand the sensitivity of core code
>> and respect the limitations on reviewer bandwidth. However, I believe
>> that reasonable optimizations should not be rejected solely because
>> they involve core code. If an improvement enhances performance,
>> readability, or maintainability without introducing risks, wouldn't
>> it be worth considering for review?
> If it's a reasonable optimisation, absolutely!  But if it's an
> optimisation, it should be accompanied with a benchmark showing an
> improvement.  As far as improving readability, I'm not yet convinced
> that you have the expertise to make that call.  Every change that is
> made invalidates everybody else's mental model of "how this works".
> So all changes carry a cost.  Sometimes that cost is worth paying,
> other times it isn't.
So we need to discuss the technical aspects first, right?
>> Regarding the reviewer shortage, I’d be happy to help by reviewing
>> other patches as well. Could you please share the process for becoming
>> a reviewer? What are the requirements or steps to get involved?
> There is no process!  Choose a patch, read it, think about it.  What
> problems might there be with it?  What may have been overlooked?
> Is the commit message unclear to you, how could it be improved?
> When you're done, send a Reviewed-by: tag (read the kernel process
> documents for the full meaning of that tag).
>
Thanks for your advice, I will try.


Thanks,

Ye

Re: [PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Vishal Moola (Oracle)]

On Wed, Apr 02, 2025 at 10:49:17AM +0800, Ye Liu wrote:
> 
> 在 2025/3/31 23:59, Matthew Wilcox 写道:
> > On Mon, Mar 31, 2025 at 08:08:01PM +0800, Ye Liu wrote:
> >> 在 2025/3/28 22:29, Matthew Wilcox 写道:
> >>> On Fri, Mar 28, 2025 at 09:47:57AM +0800, Ye Liu wrote:
> >>>> Consolidate the handling of unlikely conditions in the 
> >>>> page_expected_state() function to reduce code duplication and improve 
> >>>> readability.
> >>> I don't think this is an equivalent transformation.
> >> Could you explain it in detail?
> > page_expected_state() is called both at free and alloc.  I think
> > the correct behaviour on encountering a HWPOISON page should be
> > different at alloc and free, don't you?
> In the alloc process, this patch does not modify the code behavior.
> Regarding the free process, the if (unlikely(PageHWPoison(page)) && !order)
> code handles the case where order is 0. When order is not 0, it does not
> matter if __ClearPageBuddy is used to process the last page of the compound
> page, because page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; will also clear it.
> Is that right?                                                             

I believe a page's "Buddy" flag is stored in page->type, so maybe this
won't do what you expect?

Re: [PATCH v4] mm/page_alloc: Consolidate unlikely handling in page_expected_state

[Ye Liu]

在 2025/4/3 11:55, Vishal Moola (Oracle) 写道:
> On Wed, Apr 02, 2025 at 10:49:17AM +0800, Ye Liu wrote:
>> 在 2025/3/31 23:59, Matthew Wilcox 写道:
>>> On Mon, Mar 31, 2025 at 08:08:01PM +0800, Ye Liu wrote:
>>>> 在 2025/3/28 22:29, Matthew Wilcox 写道:
>>>>> On Fri, Mar 28, 2025 at 09:47:57AM +0800, Ye Liu wrote:
>>>>>> Consolidate the handling of unlikely conditions in the 
>>>>>> page_expected_state() function to reduce code duplication and improve 
>>>>>> readability.
>>>>> I don't think this is an equivalent transformation.
>>>> Could you explain it in detail?
>>> page_expected_state() is called both at free and alloc.  I think
>>> the correct behaviour on encountering a HWPOISON page should be
>>> different at alloc and free, don't you?
>> In the alloc process, this patch does not modify the code behavior.
>> Regarding the free process, the if (unlikely(PageHWPoison(page)) && !order)
>> code handles the case where order is 0. When order is not 0, it does not
>> matter if __ClearPageBuddy is used to process the last page of the compound
>> page, because page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; will also clear it.
>> Is that right?                                                             
> I believe a page's "Buddy" flag is stored in page->type, so maybe this
> won't do what you expect?


Got it, thanks! I originally thought that page->page_type would also be
cleared in the free process, meaning the free behavior wouldn't change,
but that’s not actually the case. I need to look into what impact it
would have if __ClearPageBuddy is also executed in the free process.   


Thanks,
Ye Liu