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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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
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 <[email protected]>
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.
[[email protected]: 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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 <[email protected]>
Urghh.. Sorry about that.
Acked-by: Kirill A. Shutemov <[email protected]>
Fixes: 97ae17497e99 ("thp: implement refcounting for huge zero page")
Cc: [email protected] # v3.8+
--
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kirill A. Shutemov