[PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Kirill A. Shutemov]

The only caller is __free_one_page(). By the time we should have
page->flags to be cleared already:

 - for 0-order pages though PCP list:
	free_hot_cold_page()
		free_pages_prepare()
			free_pages_check()
				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
		<put the page to PCP list>

	free_pcppages_bulk()
		page = <withdraw pages from PCP list>
		__free_one_page(page)

 - for non-0-order pages:
	__free_pages_ok()
		free_pages_prepare()
			free_pages_check()
				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
		free_one_page()
			__free_one_page()

So there's no way PageCompound() will return true in __free_one_page().
Let's remove dead destroy_compound_page() and put assert for page->flags
there instead.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/page_alloc.c | 35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1bb65e6f48dd..5e75380dacab 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -381,36 +381,6 @@ void prep_compound_page(struct page *page, unsigned long order)
 	}
 }
 
-/* update __split_huge_page_refcount if you change this function */
-static int destroy_compound_page(struct page *page, unsigned long order)
-{
-	int i;
-	int nr_pages = 1 << order;
-	int bad = 0;
-
-	if (unlikely(compound_order(page) != order)) {
-		bad_page(page, "wrong compound order", 0);
-		bad++;
-	}
-
-	__ClearPageHead(page);
-
-	for (i = 1; i < nr_pages; i++) {
-		struct page *p = page + i;
-
-		if (unlikely(!PageTail(p))) {
-			bad_page(page, "PageTail not set", 0);
-			bad++;
-		} else if (unlikely(p->first_page != page)) {
-			bad_page(page, "first_page not consistent", 0);
-			bad++;
-		}
-		__ClearPageTail(p);
-	}
-
-	return bad;
-}
-
 static inline void prep_zero_page(struct page *page, unsigned int order,
 							gfp_t gfp_flags)
 {
@@ -613,10 +583,7 @@ static inline void __free_one_page(struct page *page,
 	int max_order = MAX_ORDER;
 
 	VM_BUG_ON(!zone_is_initialized(zone));
-
-	if (unlikely(PageCompound(page)))
-		if (unlikely(destroy_compound_page(page, order)))
-			return;
+	VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
 
 	VM_BUG_ON(migratetype == -1);
 	if (is_migrate_isolate(migratetype)) {
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Vlastimil Babka]

On 01/05/2015 12:46 PM, Kirill A. Shutemov wrote:
> The only caller is __free_one_page(). By the time we should have
> page->flags to be cleared already:
> 
>  - for 0-order pages though PCP list:

Can there even be a 0-order compound page? I guess not, so this is just confusing?

Otherwise it seems like you are right and it's a dead code to be removed. I
tried to check history to see when it was actually needed, but seems it predates
git.

Acked-by: Vlastimil Babka <vbabka@suse.cz>


> 	free_hot_cold_page()
> 		free_pages_prepare()
> 			free_pages_check()
> 				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> 		<put the page to PCP list>
> 
> 	free_pcppages_bulk()
> 		page = <withdraw pages from PCP list>
> 		__free_one_page(page)
> 
>  - for non-0-order pages:
> 	__free_pages_ok()
> 		free_pages_prepare()
> 			free_pages_check()
> 				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> 		free_one_page()
> 			__free_one_page()
> 
> So there's no way PageCompound() will return true in __free_one_page().
> Let's remove dead destroy_compound_page() and put assert for page->flags
> there instead.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  mm/page_alloc.c | 35 +----------------------------------
>  1 file changed, 1 insertion(+), 34 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1bb65e6f48dd..5e75380dacab 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -381,36 +381,6 @@ void prep_compound_page(struct page *page, unsigned long order)
>  	}
>  }
>  
> -/* update __split_huge_page_refcount if you change this function */
> -static int destroy_compound_page(struct page *page, unsigned long order)
> -{
> -	int i;
> -	int nr_pages = 1 << order;
> -	int bad = 0;
> -
> -	if (unlikely(compound_order(page) != order)) {
> -		bad_page(page, "wrong compound order", 0);
> -		bad++;
> -	}
> -
> -	__ClearPageHead(page);
> -
> -	for (i = 1; i < nr_pages; i++) {
> -		struct page *p = page + i;
> -
> -		if (unlikely(!PageTail(p))) {
> -			bad_page(page, "PageTail not set", 0);
> -			bad++;
> -		} else if (unlikely(p->first_page != page)) {
> -			bad_page(page, "first_page not consistent", 0);
> -			bad++;
> -		}
> -		__ClearPageTail(p);
> -	}
> -
> -	return bad;
> -}
> -
>  static inline void prep_zero_page(struct page *page, unsigned int order,
>  							gfp_t gfp_flags)
>  {
> @@ -613,10 +583,7 @@ static inline void __free_one_page(struct page *page,
>  	int max_order = MAX_ORDER;
>  
>  	VM_BUG_ON(!zone_is_initialized(zone));
> -
> -	if (unlikely(PageCompound(page)))
> -		if (unlikely(destroy_compound_page(page, order)))
> -			return;
> +	VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
>  
>  	VM_BUG_ON(migratetype == -1);
>  	if (is_migrate_isolate(migratetype)) {
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Kirill A. Shutemov]

On Tue, Jan 06, 2015 at 06:43:49PM +0100, Vlastimil Babka wrote:
> On 01/05/2015 12:46 PM, Kirill A. Shutemov wrote:
> > The only caller is __free_one_page(). By the time we should have
> > page->flags to be cleared already:
> > 
> >  - for 0-order pages though PCP list:
> 
> Can there even be a 0-order compound page? I guess not, so this is just confusing?

No, it can't.

Since I propose the VM_BUG_ON(page->flags), I tried to make point that
flags are cleared by the point in any case.

> Otherwise it seems like you are right and it's a dead code to be removed. I
> tried to check history to see when it was actually needed, but seems it predates
> git.

Yeah. But we keep updating it...

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Andrew Morton]

On Mon,  5 Jan 2015 13:46:22 +0200 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> The only caller is __free_one_page(). By the time we should have
> page->flags to be cleared already:
> 
>  - for 0-order pages though PCP list:
> 	free_hot_cold_page()
> 		free_pages_prepare()
> 			free_pages_check()
> 				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> 		<put the page to PCP list>
> 
> 	free_pcppages_bulk()
> 		page = <withdraw pages from PCP list>
> 		__free_one_page(page)
> 
>  - for non-0-order pages:
> 	__free_pages_ok()
> 		free_pages_prepare()
> 			free_pages_check()
> 				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> 		free_one_page()
> 			__free_one_page()
> 
> So there's no way PageCompound() will return true in __free_one_page().
> Let's remove dead destroy_compound_page() and put assert for page->flags
> there instead.

Well.  An alternative would be to fix up the call site so those
useful-looking checks actually get to check things.  Perhaps under
CONFIG_DEBUG_VM.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Kirill A. Shutemov]

Andrew Morton wrote:
> On Mon,  5 Jan 2015 13:46:22 +0200 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > The only caller is __free_one_page(). By the time we should have
> > page->flags to be cleared already:
> > 
> >  - for 0-order pages though PCP list:
> > 	free_hot_cold_page()
> > 		free_pages_prepare()
> > 			free_pages_check()
> > 				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> > 		<put the page to PCP list>
> > 
> > 	free_pcppages_bulk()
> > 		page = <withdraw pages from PCP list>
> > 		__free_one_page(page)
> > 
> >  - for non-0-order pages:
> > 	__free_pages_ok()
> > 		free_pages_prepare()
> > 			free_pages_check()
> > 				page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> > 		free_one_page()
> > 			__free_one_page()
> > 
> > So there's no way PageCompound() will return true in __free_one_page().
> > Let's remove dead destroy_compound_page() and put assert for page->flags
> > there instead.
> 
> Well.  An alternative would be to fix up the call site so those
> useful-looking checks actually get to check things.  Perhaps under
> CONFIG_DEBUG_VM.

Something like this?

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Andrew Morton]

On Thu,  8 Jan 2015 16:10:04 +0200 (EET) "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> Something like this?
> 
> >From 5fd481c1c521112e9cea407f5a2644c9f93d0e14 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Thu, 8 Jan 2015 15:59:23 +0200
> Subject: [PATCH] mm: more checks on free_pages_prepare() for tail pages
> 
> Apart form being dead, destroy_compound_page() did some potentially
> useful checks. Let's re-introduce them in free_pages_prepare(), where
> they can be acctually triggered.
> 
> compound_order() assert is already in free_pages_prepare(). We have few
> checks for tail pages left.
> 

I'm thinking we avoid the overhead unless CONFIG_DEBUG_VM?


From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm-more-checks-on-free_pages_prepare-for-tail-pages-fix

Make it conditional on CONFIG_DEBUG_VM, make free_tail_pages_check()
return void.

Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/page_alloc.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff -puN mm/page_alloc.c~mm-more-checks-on-free_pages_prepare-for-tail-pages-fix mm/page_alloc.c
--- a/mm/page_alloc.c~mm-more-checks-on-free_pages_prepare-for-tail-pages-fix
+++ a/mm/page_alloc.c
@@ -764,20 +764,26 @@ static void free_one_page(struct zone *z
 	spin_unlock(&zone->lock);
 }
 
-static int free_tail_pages_check(struct page *head_page, struct page *page)
+#ifdef CONFIG_DEBUG_VM
+static void free_tail_pages_check(struct page *head_page, struct page *page)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_VM))
-		return 0;
+		return;
 	if (unlikely(!PageTail(page))) {
 		bad_page(page, "PageTail not set", 0);
-		return 1;
+		return;
 	}
 	if (unlikely(page->first_page != head_page)) {
 		bad_page(page, "first_page not consistent", 0);
-		return 1;
+		return;
 	}
-	return 0;
 }
+#else
+static inline void free_tail_pages_check(struct page *head_page,
+					 struct page *page)
+{
+}
+#endif
 
 static bool free_pages_prepare(struct page *page, unsigned int order)
 {
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Kirill A. Shutemov]

On Fri, Jan 09, 2015 at 04:24:19PM -0800, Andrew Morton wrote:
> On Thu,  8 Jan 2015 16:10:04 +0200 (EET) "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > Something like this?
> > 
> > >From 5fd481c1c521112e9cea407f5a2644c9f93d0e14 Mon Sep 17 00:00:00 2001
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > Date: Thu, 8 Jan 2015 15:59:23 +0200
> > Subject: [PATCH] mm: more checks on free_pages_prepare() for tail pages
> > 
> > Apart form being dead, destroy_compound_page() did some potentially
> > useful checks. Let's re-introduce them in free_pages_prepare(), where
> > they can be acctually triggered.
> > 
> > compound_order() assert is already in free_pages_prepare(). We have few
> > checks for tail pages left.
> > 
> 
> I'm thinking we avoid the overhead unless CONFIG_DEBUG_VM?

That's why there's "if (!IS_ENABLED(CONFIG_DEBUG_VM))". Is it wrong in
some way?
I didn't check, but I assume compiler is smart enough to get rid of
free_tail_pages_check() if CONFIG_DEBUG_VM is not defined. No?


-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Andrew Morton]

On Sat, 10 Jan 2015 02:41:43 +0200 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Fri, Jan 09, 2015 at 04:24:19PM -0800, Andrew Morton wrote:
> > On Thu,  8 Jan 2015 16:10:04 +0200 (EET) "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > 
> > > Something like this?
> > > 
> > > >From 5fd481c1c521112e9cea407f5a2644c9f93d0e14 Mon Sep 17 00:00:00 2001
> > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > Date: Thu, 8 Jan 2015 15:59:23 +0200
> > > Subject: [PATCH] mm: more checks on free_pages_prepare() for tail pages
> > > 
> > > Apart form being dead, destroy_compound_page() did some potentially
> > > useful checks. Let's re-introduce them in free_pages_prepare(), where
> > > they can be acctually triggered.
> > > 
> > > compound_order() assert is already in free_pages_prepare(). We have few
> > > checks for tail pages left.
> > > 
> > 
> > I'm thinking we avoid the overhead unless CONFIG_DEBUG_VM?
> 
> That's why there's "if (!IS_ENABLED(CONFIG_DEBUG_VM))". Is it wrong in
> some way?
> I didn't check, but I assume compiler is smart enough to get rid of
> free_tail_pages_check() if CONFIG_DEBUG_VM is not defined. No?

doh, OK.  I updated the
mm-more-checks-on-free_pages_prepare-for-tail-pages.patch changelog to
reflect this and did

--- a/mm/page_alloc.c~mm-more-checks-on-free_pages_prepare-for-tail-pages-fix
+++ a/mm/page_alloc.c
@@ -764,19 +764,18 @@ static void free_one_page(struct zone *z
 	spin_unlock(&zone->lock);
 }
 
-static int free_tail_pages_check(struct page *head_page, struct page *page)
+static void free_tail_pages_check(struct page *head_page, struct page *page)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_VM))
-		return 0;
+		return;
 	if (unlikely(!PageTail(page))) {
 		bad_page(page, "PageTail not set", 0);
-		return 1;
+		return;
 	}
 	if (unlikely(page->first_page != head_page)) {
 		bad_page(page, "first_page not consistent", 0);
-		return 1;
+		return;
 	}
-	return 0;
 }
 
 static bool free_pages_prepare(struct page *page, unsigned int order)
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc.c: drop dead destroy_compound_page()

[Kirill A. Shutemov]

On Fri, Jan 09, 2015 at 05:06:42PM -0800, Andrew Morton wrote:
> On Sat, 10 Jan 2015 02:41:43 +0200 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Fri, Jan 09, 2015 at 04:24:19PM -0800, Andrew Morton wrote:
> > > On Thu,  8 Jan 2015 16:10:04 +0200 (EET) "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > > 
> > > > Something like this?
> > > > 
> > > > >From 5fd481c1c521112e9cea407f5a2644c9f93d0e14 Mon Sep 17 00:00:00 2001
> > > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > > Date: Thu, 8 Jan 2015 15:59:23 +0200
> > > > Subject: [PATCH] mm: more checks on free_pages_prepare() for tail pages
> > > > 
> > > > Apart form being dead, destroy_compound_page() did some potentially
> > > > useful checks. Let's re-introduce them in free_pages_prepare(), where
> > > > they can be acctually triggered.
> > > > 
> > > > compound_order() assert is already in free_pages_prepare(). We have few
> > > > checks for tail pages left.
> > > > 
> > > 
> > > I'm thinking we avoid the overhead unless CONFIG_DEBUG_VM?
> > 
> > That's why there's "if (!IS_ENABLED(CONFIG_DEBUG_VM))". Is it wrong in
> > some way?
> > I didn't check, but I assume compiler is smart enough to get rid of
> > free_tail_pages_check() if CONFIG_DEBUG_VM is not defined. No?
> 
> doh, OK.  I updated the
> mm-more-checks-on-free_pages_prepare-for-tail-pages.patch changelog to
> reflect this and did
> 
> --- a/mm/page_alloc.c~mm-more-checks-on-free_pages_prepare-for-tail-pages-fix
> +++ a/mm/page_alloc.c
> @@ -764,19 +764,18 @@ static void free_one_page(struct zone *z
>  	spin_unlock(&zone->lock);
>  }
>  
> -static int free_tail_pages_check(struct page *head_page, struct page *page)
> +static void free_tail_pages_check(struct page *head_page, struct page *page)
>  {
>  	if (!IS_ENABLED(CONFIG_DEBUG_VM))
> -		return 0;
> +		return;
>  	if (unlikely(!PageTail(page))) {
>  		bad_page(page, "PageTail not set", 0);
> -		return 1;
> +		return;
>  	}
>  	if (unlikely(page->first_page != head_page)) {
>  		bad_page(page, "first_page not consistent", 0);
> -		return 1;
> +		return;
>  	}
> -	return 0;
>  }
>  
>  static bool free_pages_prepare(struct page *page, unsigned int order)
> _

Oops. I wanted this return code to be accounted into 'bad' in
free_pages_prepare() instead. Incremental patch:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index cf327e2eea6f..ee37d1e0c969 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -798,7 +798,7 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
        bad += free_pages_check(page);
        for (i = 1; i < (1 << order); i++) {
                if (compound)
-                       free_tail_pages_check(page, page + i);
+                       bad += free_tail_pages_check(page, page + i);
                bad += free_pages_check(page + i);
        }
        if (bad)
-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>