[PATCH 1/2] mm: free compound page with correct order

[PATCH 1/2] mm: free compound page with correct order

[Yu Zhao]

Compound page should be freed by put_page() or free_pages() with
correct order. Not doing so with causing the tail pages leaked.

The compound order can be obtained by compound_order() or use
HPAGE_PMD_ORDER in our case. Some people would argue the latter
is faster but I prefer the former which is more general.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/huge_memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 74c78aa..780d12c 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -200,7 +200,7 @@ retry:
 	preempt_disable();
 	if (cmpxchg(&huge_zero_page, NULL, zero_page)) {
 		preempt_enable();
-		__free_page(zero_page);
+		__free_pages(zero_page, compound_order(zero_page));
 		goto retry;
 	}
 
@@ -232,7 +232,7 @@ static unsigned long shrink_huge_zero_page_scan(struct shrinker *shrink,
 	if (atomic_cmpxchg(&huge_zero_refcount, 1, 0) == 1) {
 		struct page *zero_page = xchg(&huge_zero_page, NULL);
 		BUG_ON(zero_page == NULL);
-		__free_page(zero_page);
+		__free_pages(zero_page, compound_order(zero_page));
 		return HPAGE_PMD_NR;
 	}
 
-- 
2.1.0.rc2.206.gedb03e5

--
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>

[PATCH 2/2] mm: verify compound order when freeing a page

[Yu Zhao]

This allows us to easily catch the bug fixed in previous patch.

Here we also verify whether a page is tail page or not -- tail
pages are supposed to be freed along with their head, not by
themselves.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 736d8e1..2bcc770 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -750,6 +750,9 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	int i;
 	int bad = 0;
 
+	VM_BUG_ON(PageTail(page));
+	VM_BUG_ON(PageHead(page) && compound_order(page) != order);
+
 	trace_mm_page_free(page, order);
 	kmemcheck_free_shadow(page, order);
 
-- 
2.1.0.rc2.206.gedb03e5

--
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 2/2] mm: verify compound order when freeing a page

[David Cohen]

Hi Yu,

On Tue, Oct 14, 2014 at 01:16:40PM -0700, Yu Zhao wrote:
> This allows us to easily catch the bug fixed in previous patch.

Is the word "previous" a good way to relate patches after merged?
Maybe you could either detail the bug here or be more verbose about the
patch you're referring.

> 
> Here we also verify whether a page is tail page or not -- tail
> pages are supposed to be freed along with their head, not by
> themselves.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  mm/page_alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 736d8e1..2bcc770 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -750,6 +750,9 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
>  	int i;
>  	int bad = 0;
>  
> +	VM_BUG_ON(PageTail(page));
> +	VM_BUG_ON(PageHead(page) && compound_order(page) != order);

It may be too severe. AFAIU we're not talking about a fatal error.
How about VM_WARN_ON()?

Br, David Cohen

> +
>  	trace_mm_page_free(page, order);
>  	kmemcheck_free_shadow(page, order);
>  
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
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 2/2] mm: verify compound order when freeing a page

[Sasha Levin]

On 10/14/2014 04:29 PM, David Cohen wrote:
>> +	VM_BUG_ON(PageTail(page));
>> > +	VM_BUG_ON(PageHead(page) && compound_order(page) != order);
> It may be too severe. AFAIU we're not talking about a fatal error.
> How about VM_WARN_ON()?

VM_BUG_ON() should catch anything which is not "supposed" to happen,
and not just the severe stuff. Unlike BUG_ON, VM_BUG_ON only gets
hit with mm debugging enabled.


Thanks,
Sasha

--
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 2/2] mm: verify compound order when freeing a page

[David Cohen]

On Tue, Oct 14, 2014 at 07:08:43PM -0400, Sasha Levin wrote:
> On 10/14/2014 04:29 PM, David Cohen wrote:
> >> +	VM_BUG_ON(PageTail(page));
> >> > +	VM_BUG_ON(PageHead(page) && compound_order(page) != order);
> > It may be too severe. AFAIU we're not talking about a fatal error.
> > How about VM_WARN_ON()?
> 
> VM_BUG_ON() should catch anything which is not "supposed" to happen,
> and not just the severe stuff. Unlike BUG_ON, VM_BUG_ON only gets
> hit with mm debugging enabled.

Thanks for pointing that out :)
VM_WARN_ON*() is recent, so there isn't much examples when to use it.
I considered the below case similar to this patch. But your point does
make sense anyway.

commit 82f71ae4a2b829a25971bdf54b4d0d3d69d3c8b7
Author: Konstantin Khlebnikov <koct9i@gmail.com>
Date:   Wed Aug 6 16:06:36 2014 -0700

    mm: catch memory commitment underflow
    
    Print a warning (if CONFIG_DEBUG_VM=y) when memory commitment becomes
    too negative.
    
    This shouldn't happen any more - the previous two patches fixed the
    committed_as underflow issues.

    [akpm@linux-foundation.org: use VM_WARN_ONCE, per Dave]


Br, David

> 
> 
> Thanks,
> Sasha
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
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 1/2] mm: free compound page with correct order

[Kirill A. Shutemov]

On Tue, Oct 14, 2014 at 01:16:39PM -0700, Yu Zhao wrote:
> Compound page should be freed by put_page() or free_pages() with
> correct order. Not doing so with causing the tail pages leaked.
> 
> The compound order can be obtained by compound_order() or use
> HPAGE_PMD_ORDER in our case. Some people would argue the latter
> is faster but I prefer the former which is more general.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>

Urghh.. Sorry about that.

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: 97ae17497e99 ("thp: implement refcounting for huge zero page")
Cc: stable@vger.kernel.org # v3.8+

-- 
 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 2/2] mm: verify compound order when freeing a page

[Kirill A. Shutemov]

On Tue, Oct 14, 2014 at 01:16:40PM -0700, Yu Zhao wrote:
> This allows us to easily catch the bug fixed in previous patch.
> 
> Here we also verify whether a page is tail page or not -- tail
> pages are supposed to be freed along with their head, not by
> themselves.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  mm/page_alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 736d8e1..2bcc770 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -750,6 +750,9 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
>  	int i;
>  	int bad = 0;
>  
> +	VM_BUG_ON(PageTail(page));
> +	VM_BUG_ON(PageHead(page) && compound_order(page) != order);
> +

Use VM_BUG_ON_PAGE(), please.

>  	trace_mm_page_free(page, order);
>  	kmemcheck_free_shadow(page, order);
>  
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
 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>