Re: [PATCH 1/2] mm: fix logic error of bulkfree_pcp_prepare

[Andrew Morton]

On Tue, 13 Sep 2022 14:10:45 +0800 "zhaoyang.huang" <zhaoyang.huang@xxxxxxxxxx> wrote:

> From: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> 
> free_pages_check return 0 when result is ok while bulkfree_pcp_prepare
> treat it as false wrongly.

It's called check_free_page().

And that's a poor name because the name doesn't communicate what a
true/false return value means - was the page good or bad?

So I'd propose this renaming:

From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Subject: mm/page_alloc.c: rename check_free_page() to free_page_is_bad()
Date: Tue Sep 13 03:20:48 PM PDT 2022

The name "check_free_page()" provides no information regarding its return
value when the page is indeed found to be bad.

Renaming it to "free_page_is_bad()" makes it clear that a `true' return
value means the page was bad.

And make it return a bool, not an int.

Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---

 mm/page_alloc.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

--- a/mm/page_alloc.c~a
+++ a/mm/page_alloc.c
@@ -1290,20 +1290,20 @@ static const char *page_bad_reason(struc
 	return bad_reason;
 }
 
-static void check_free_page_bad(struct page *page)
+static void free_page_bad_report(struct page *page)
 {
 	bad_page(page,
 		 page_bad_reason(page, PAGE_FLAGS_CHECK_AT_FREE));
 }
 
-static inline int check_free_page(struct page *page)
+static inline bool free_page_bad(struct page *page)
 {
 	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE)))
-		return 0;
+		return false;
 
 	/* Something has gone sideways, find it */
-	check_free_page_bad(page);
-	return 1;
+	free_page_bad_report(page);
+	return true;
 }
 
 static int free_tail_pages_check(struct page *head_page, struct page *page)
@@ -1436,7 +1436,7 @@ static __always_inline bool free_pages_p
 		for (i = 1; i < (1 << order); i++) {
 			if (compound)
 				bad += free_tail_pages_check(page, page + i);
-			if (unlikely(check_free_page(page + i))) {
+			if (unlikely(free_page_bad(page + i))) {
 				bad++;
 				continue;
 			}
@@ -1448,7 +1448,7 @@ static __always_inline bool free_pages_p
 	if (memcg_kmem_enabled() && PageMemcgKmem(page))
 		__memcg_kmem_uncharge_page(page, order);
 	if (check_free)
-		bad += check_free_page(page);
+		bad += free_page_bad(page);
 	if (bad)
 		return false;
 
@@ -1510,7 +1510,7 @@ static bool free_pcp_prepare(struct page
 static bool bulkfree_pcp_prepare(struct page *page)
 {
 	if (debug_pagealloc_enabled_static())
-		return check_free_page(page);
+		return free_page_bad(page);
 	else
 		return false;
 }
@@ -1531,7 +1531,7 @@ static bool free_pcp_prepare(struct page
 
 static bool bulkfree_pcp_prepare(struct page *page)
 {
-	return check_free_page(page);
+	return free_page_bad(page);
 }
 #endif /* CONFIG_DEBUG_VM */
 
_

And bulkfree_pcp_prepare() is pretty bad as well - how about we
document the dang return value?

--- a/mm/page_alloc.c~b
+++ a/mm/page_alloc.c
@@ -1507,6 +1507,7 @@ static bool free_pcp_prepare(struct page
 	return free_pages_prepare(page, order, true, FPI_NONE);
 }
 
+/* return true if this page has an inappropriate state */
 static bool bulkfree_pcp_prepare(struct page *page)
 {
 	if (debug_pagealloc_enabled_static())
_

> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1424,7 +1424,7 @@ static bool free_pcp_prepare(struct page *page, unsigned int order)
>  static bool bulkfree_pcp_prepare(struct page *page)
>  {
>  	if (debug_pagealloc_enabled_static())
> -		return check_free_page(page);
> +		return !check_free_page(page);
>  	else
>  		return false;
>  }
> @@ -1445,7 +1445,7 @@ static bool free_pcp_prepare(struct page *page, unsigned int order)
>  
>  static bool bulkfree_pcp_prepare(struct page *page)
>  {
> -	return check_free_page(page);
> +	return !check_free_page(page);
>  }
>  #endif /* CONFIG_DEBUG_VM */

And after clarifying these things, your patch seems incorrect.

free_pcppages_bulk() does

			if (bulkfree_pcp_prepare(page))
				continue;

in other words, it leaks the page if it was found to be messed up?