[PATCH 0/3] HWPOISON, hugetlbfs: small bug fixes

[PATCH 0/3] HWPOISON, hugetlbfs: small bug fixes

[Naoya Horiguchi]

Hi,

I found some small bugs about memory error handling on hugepages by my
testing on the recent kernel, so I wrote patches for them.
Can I have your reviews or comments on them?

Thanks,
Naoya

[PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Naoya Horiguchi]

This patch fixes the warning from __list_del_entry() which is triggered
when a process tries to do free_huge_page() for a hwpoisoned hugepage.

Originally, page->lru of hugetlbfs head page was dangling when the
hugepage was in use. This behavior has changed by commit 0edaecfab218d7
("hugetlb: add a list for tracking in-use HugeTLB pages"), where hugepages
in use are linked to hugepage_activelist. HWpoisoned hugepages should not
be charged to any process, so we introduce another list to link hwpoisoned
hugepages.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 include/linux/hugetlb.h | 3 +++
 mm/hugetlb.c            | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git v3.7-rc8.orig/include/linux/hugetlb.h v3.7-rc8/include/linux/hugetlb.h
index 2251648..13858ba 100644
--- v3.7-rc8.orig/include/linux/hugetlb.h
+++ v3.7-rc8/include/linux/hugetlb.h
@@ -230,6 +230,9 @@ struct hstate {
 	unsigned long nr_overcommit_huge_pages;
 	struct list_head hugepage_activelist;
 	struct list_head hugepage_freelists[MAX_NUMNODES];
+#ifdef CONFIG_MEMORY_FAILURE
+	struct list_head hugepage_hwpoisonedlist;
+#endif
 	unsigned int nr_huge_pages_node[MAX_NUMNODES];
 	unsigned int free_huge_pages_node[MAX_NUMNODES];
 	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
diff --git v3.7-rc8.orig/mm/hugetlb.c v3.7-rc8/mm/hugetlb.c
index 59a0059..e61a749 100644
--- v3.7-rc8.orig/mm/hugetlb.c
+++ v3.7-rc8/mm/hugetlb.c
@@ -1939,6 +1939,7 @@ void __init hugetlb_add_hstate(unsigned order)
 	for (i = 0; i < MAX_NUMNODES; ++i)
 		INIT_LIST_HEAD(&h->hugepage_freelists[i]);
 	INIT_LIST_HEAD(&h->hugepage_activelist);
+	INIT_LIST_HEAD(&h->hugepage_hwpoisonedlist);
 	h->next_nid_to_alloc = first_node(node_states[N_HIGH_MEMORY]);
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
@@ -3170,7 +3171,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
 
 	spin_lock(&hugetlb_lock);
 	if (is_hugepage_on_freelist(hpage)) {
-		list_del(&hpage->lru);
+		list_move(&hpage->lru, &h->hugepage_hwpoisonedlist);
 		set_page_refcounted(hpage);
 		h->free_huge_pages--;
 		h->free_huge_pages_node[nid]--;
-- 
1.7.11.7

[PATCH 2/3] HWPOISON, hugetlbfs: fix "bad pmd" warning in unmapping hwpoisoned hugepage

[Naoya Horiguchi]

When a process which used a hwpoisoned hugepage tries to exit() or
munmap(), the kernel can print out "bad pmd" message because page
table walker in free_pgtables() encounters 'hwpoisoned entry' on pmd.

This is because currently we fail to clear the hwpoisoned entry in
__unmap_hugepage_range(), so this patch simply does it.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git v3.7-rc8.orig/mm/hugetlb.c v3.7-rc8/mm/hugetlb.c
index e61a749..fe7c2a7 100644
--- v3.7-rc8.orig/mm/hugetlb.c
+++ v3.7-rc8/mm/hugetlb.c
@@ -2387,8 +2387,10 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		/*
 		 * HWPoisoned hugepage is already unmapped and dropped reference
 		 */
-		if (unlikely(is_hugetlb_entry_hwpoisoned(pte)))
+		if (unlikely(is_hugetlb_entry_hwpoisoned(pte))) {
+			pte_clear(mm, address, ptep);
 			continue;
+		}
 
 		page = pte_page(pte);
 		/*
-- 
1.7.11.7

[PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning

[Naoya Horiguchi]

Memory error handling on hugepages can break a RSS counter, which emits
a message like "Bad rss-counter state mm:ffff88040abecac0 idx:1 val:-1".
This is because PageAnon returns true for hugepage (this behavior is
necessary for reverse mapping to work on hugetlbfs).

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/rmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git v3.7-rc8.orig/mm/rmap.c v3.7-rc8/mm/rmap.c
index 2ee1ef0..df54ef0 100644
--- v3.7-rc8.orig/mm/rmap.c
+++ v3.7-rc8/mm/rmap.c
@@ -1235,7 +1235,9 @@ int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 	update_hiwater_rss(mm);
 
 	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
-		if (PageAnon(page))
+		if (PageHuge(page))
+			;
+		else if (PageAnon(page))
 			dec_mm_counter(mm, MM_ANONPAGES);
 		else
 			dec_mm_counter(mm, MM_FILEPAGES);
-- 
1.7.11.7

RE: [PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning

[Luck, Tony]

	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
-		if (PageAnon(page))
+		if (PageHuge(page))
+			;
+		else if (PageAnon(page))
 			dec_mm_counter(mm, MM_ANONPAGES);
 		else
 			dec_mm_counter(mm, MM_FILEPAGES);

This style minimizes the "diff" ... but wouldn't it be nicer to say:

		if (!PageHuge(page)) {
			old code in here
		}

-Tony

RE: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Luck, Tony]

> This patch fixes the warning from __list_del_entry() which is triggered
> when a process tries to do free_huge_page() for a hwpoisoned hugepage.

Ultimately it would be nice to avoid poisoning huge pages. Generally we know the
location of the poison to a cache line granularity (but sometimes only to a 4K
granularity) ... and it is rather inefficient to take an entire 2M page out of service.
With 1G pages things would be even worse!!

It also makes life harder for applications that would like to catch the SIGBUS
and try to take their own recovery actions. Losing more data than they really
need to will make it less likely that they can do something to work around the
loss.

Has anyone looked at how hard it might be to have the code in memory-failure.c
break up a huge page and only poison the 4K that needs to be taken out of service?

-Tony

Re: [PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning

[Naoya Horiguchi]

Hi Tony,

On Wed, Dec 05, 2012 at 10:04:50PM +0000, Luck, Tony wrote:
> 	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
> -		if (PageAnon(page))
> +		if (PageHuge(page))
> +			;
> +		else if (PageAnon(page))
>  			dec_mm_counter(mm, MM_ANONPAGES);
>  		else
>  			dec_mm_counter(mm, MM_FILEPAGES);
> 
> This style minimizes the "diff" ... but wouldn't it be nicer to say:
> 
> 		if (!PageHuge(page)) {
> 			old code in here
> 		}
> 

I think this need more lines in diff because old code should be
indented without any logical change.

Thanks,
Naoya

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Andrew Morton]

On Wed,  5 Dec 2012 16:47:36 -0500
Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:

> This patch fixes the warning from __list_del_entry() which is triggered
> when a process tries to do free_huge_page() for a hwpoisoned hugepage.
> 
> Originally, page->lru of hugetlbfs head page was dangling when the
> hugepage was in use. This behavior has changed by commit 0edaecfab218d7
> ("hugetlb: add a list for tracking in-use HugeTLB pages"), where hugepages
> in use are linked to hugepage_activelist. HWpoisoned hugepages should not
> be charged to any process, so we introduce another list to link hwpoisoned
> hugepages.
> 
> ...
>
> --- v3.7-rc8.orig/include/linux/hugetlb.h
> +++ v3.7-rc8/include/linux/hugetlb.h
> @@ -230,6 +230,9 @@ struct hstate {
>  	unsigned long nr_overcommit_huge_pages;
>  	struct list_head hugepage_activelist;
>  	struct list_head hugepage_freelists[MAX_NUMNODES];
> +#ifdef CONFIG_MEMORY_FAILURE
> +	struct list_head hugepage_hwpoisonedlist;
> +#endif
>  	unsigned int nr_huge_pages_node[MAX_NUMNODES];
>  	unsigned int free_huge_pages_node[MAX_NUMNODES];
>  	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
> diff --git v3.7-rc8.orig/mm/hugetlb.c v3.7-rc8/mm/hugetlb.c
> index 59a0059..e61a749 100644
> --- v3.7-rc8.orig/mm/hugetlb.c
> +++ v3.7-rc8/mm/hugetlb.c
> @@ -1939,6 +1939,7 @@ void __init hugetlb_add_hstate(unsigned order)
>  	for (i = 0; i < MAX_NUMNODES; ++i)
>  		INIT_LIST_HEAD(&h->hugepage_freelists[i]);
>  	INIT_LIST_HEAD(&h->hugepage_activelist);
> +	INIT_LIST_HEAD(&h->hugepage_hwpoisonedlist);
>  	h->next_nid_to_alloc = first_node(node_states[N_HIGH_MEMORY]);
>  	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
>  	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
> @@ -3170,7 +3171,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
>  
>  	spin_lock(&hugetlb_lock);
>  	if (is_hugepage_on_freelist(hpage)) {
> -		list_del(&hpage->lru);
> +		list_move(&hpage->lru, &h->hugepage_hwpoisonedlist);
>  		set_page_refcounted(hpage);
>  		h->free_huge_pages--;
>  		h->free_huge_pages_node[nid]--;

Do we actually need to new list?  We could use list_del_init() to leave
the page's list_head pointing at itself.  In this state, it is its own
list_head and further list_del()s are a no-op.

I don't know whether this would trigger list-debug warnings.

Re: [PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning

[Andrew Morton]

On Wed,  5 Dec 2012 17:14:33 -0500
Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:

> Hi Tony,
> 
> On Wed, Dec 05, 2012 at 10:04:50PM +0000, Luck, Tony wrote:
> > 	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
> > -		if (PageAnon(page))
> > +		if (PageHuge(page))
> > +			;
> > +		else if (PageAnon(page))
> >  			dec_mm_counter(mm, MM_ANONPAGES);
> >  		else
> >  			dec_mm_counter(mm, MM_FILEPAGES);
> > 
> > This style minimizes the "diff" ... but wouldn't it be nicer to say:
> > 
> > 		if (!PageHuge(page)) {
> > 			old code in here
> > 		}
> > 
> 
> I think this need more lines in diff because old code should be
> indented without any logical change.

I do agree with Tony on this.  While it is nice to keep the diff
looking simple, it is more important that the resulting code be clean
and idiomatic.

--- a/mm/rmap.c~hwpoison-hugetlbfs-fix-rss-counter-warning-fix
+++ a/mm/rmap.c
@@ -1249,14 +1249,14 @@ int try_to_unmap_one(struct page *page, 
 	update_hiwater_rss(mm);
 
 	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
-		if (PageHuge(page))
-			;
-		else if (PageAnon(page))
-			dec_mm_counter(mm, MM_ANONPAGES);
-		else
-			dec_mm_counter(mm, MM_FILEPAGES);
-		set_pte_at(mm, address, pte,
-				swp_entry_to_pte(make_hwpoison_entry(page)));
+		if (!PageHuge(page)) {
+			if (PageAnon(page))
+				dec_mm_counter(mm, MM_ANONPAGES);
+			else
+				dec_mm_counter(mm, MM_FILEPAGES);
+			set_pte_at(mm, address, pte,
+				   swp_entry_to_pte(make_hwpoison_entry(page)));
+		}
 	} else if (PageAnon(page)) {
 		swp_entry_t entry = { .val = page_private(page) };
 
_

Re: [PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning

[Naoya Horiguchi]

On Thu, Dec 06, 2012 at 02:40:08PM -0800, Andrew Morton wrote:
...
> > On Wed, Dec 05, 2012 at 10:04:50PM +0000, Luck, Tony wrote:
> > > 	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
> > > -		if (PageAnon(page))
> > > +		if (PageHuge(page))
> > > +			;
> > > +		else if (PageAnon(page))
> > >  			dec_mm_counter(mm, MM_ANONPAGES);
> > >  		else
> > >  			dec_mm_counter(mm, MM_FILEPAGES);
> > > 
> > > This style minimizes the "diff" ... but wouldn't it be nicer to say:
> > > 
> > > 		if (!PageHuge(page)) {
> > > 			old code in here
> > > 		}
> > > 
> > 
> > I think this need more lines in diff because old code should be
> > indented without any logical change.
> 
> I do agree with Tony on this.  While it is nice to keep the diff
> looking simple, it is more important that the resulting code be clean
> and idiomatic.

OK, I agree.

> --- a/mm/rmap.c~hwpoison-hugetlbfs-fix-rss-counter-warning-fix
> +++ a/mm/rmap.c
> @@ -1249,14 +1249,14 @@ int try_to_unmap_one(struct page *page, 
>  	update_hiwater_rss(mm);
>  
>  	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
> -		if (PageHuge(page))
> -			;
> -		else if (PageAnon(page))
> -			dec_mm_counter(mm, MM_ANONPAGES);
> -		else
> -			dec_mm_counter(mm, MM_FILEPAGES);
> -		set_pte_at(mm, address, pte,
> -				swp_entry_to_pte(make_hwpoison_entry(page)));
> +		if (!PageHuge(page)) {
> +			if (PageAnon(page))
> +				dec_mm_counter(mm, MM_ANONPAGES);
> +			else
> +				dec_mm_counter(mm, MM_FILEPAGES);
> +			set_pte_at(mm, address, pte,
> +				   swp_entry_to_pte(make_hwpoison_entry(page)));
> +		}

This set_pte_at() should come outside the if-block, or error containment
does not work.

Thanks,
Naoya

>  	} else if (PageAnon(page)) {
>  		swp_entry_t entry = { .val = page_private(page) };
>  
> _
> 

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Naoya Horiguchi]

On Thu, Dec 06, 2012 at 02:36:52PM -0800, Andrew Morton wrote:
> On Wed,  5 Dec 2012 16:47:36 -0500
> Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> 
> > This patch fixes the warning from __list_del_entry() which is triggered
> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
> > 
> > Originally, page->lru of hugetlbfs head page was dangling when the
> > hugepage was in use. This behavior has changed by commit 0edaecfab218d7
> > ("hugetlb: add a list for tracking in-use HugeTLB pages"), where hugepages
> > in use are linked to hugepage_activelist. HWpoisoned hugepages should not
> > be charged to any process, so we introduce another list to link hwpoisoned
> > hugepages.
> > 
> > ...
> >
> > --- v3.7-rc8.orig/include/linux/hugetlb.h
> > +++ v3.7-rc8/include/linux/hugetlb.h
> > @@ -230,6 +230,9 @@ struct hstate {
> >  	unsigned long nr_overcommit_huge_pages;
> >  	struct list_head hugepage_activelist;
> >  	struct list_head hugepage_freelists[MAX_NUMNODES];
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +	struct list_head hugepage_hwpoisonedlist;
> > +#endif
> >  	unsigned int nr_huge_pages_node[MAX_NUMNODES];
> >  	unsigned int free_huge_pages_node[MAX_NUMNODES];
> >  	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
> > diff --git v3.7-rc8.orig/mm/hugetlb.c v3.7-rc8/mm/hugetlb.c
> > index 59a0059..e61a749 100644
> > --- v3.7-rc8.orig/mm/hugetlb.c
> > +++ v3.7-rc8/mm/hugetlb.c
> > @@ -1939,6 +1939,7 @@ void __init hugetlb_add_hstate(unsigned order)
> >  	for (i = 0; i < MAX_NUMNODES; ++i)
> >  		INIT_LIST_HEAD(&h->hugepage_freelists[i]);
> >  	INIT_LIST_HEAD(&h->hugepage_activelist);
> > +	INIT_LIST_HEAD(&h->hugepage_hwpoisonedlist);
> >  	h->next_nid_to_alloc = first_node(node_states[N_HIGH_MEMORY]);
> >  	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
> >  	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
> > @@ -3170,7 +3171,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
> >  
> >  	spin_lock(&hugetlb_lock);
> >  	if (is_hugepage_on_freelist(hpage)) {
> > -		list_del(&hpage->lru);
> > +		list_move(&hpage->lru, &h->hugepage_hwpoisonedlist);
> >  		set_page_refcounted(hpage);
> >  		h->free_huge_pages--;
> >  		h->free_huge_pages_node[nid]--;
> 
> Do we actually need to new list?  We could use list_del_init() to leave
> the page's list_head pointing at itself.  In this state, it is its own
> list_head and further list_del()s are a no-op.

OK, it's better, thanks.

> I don't know whether this would trigger list-debug warnings.

I tested your idea (with attached patch) and confirmed that
we never get the warnings.

Thanks,
Naoya
---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Thu, 6 Dec 2012 20:54:30 -0500
Subject: [PATCH v2] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned
 hugepage

This patch fixes the warning from __list_del_entry() which is triggered
when a process tries to do free_huge_page() for a hwpoisoned hugepage.

ChangeLog v2:
 - simply use list_del_init instead of introducing new hugepage list

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 59a0059..9308752 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3170,7 +3170,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
 
 	spin_lock(&hugetlb_lock);
 	if (is_hugepage_on_freelist(hpage)) {
-		list_del(&hpage->lru);
+		list_del_init(&hpage->lru);
 		set_page_refcounted(hpage);
 		h->free_huge_pages--;
 		h->free_huge_pages_node[nid]--;
-- 
1.7.11.7

Re: [PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning

[Andrew Morton]

On Thu,  6 Dec 2012 20:22:42 -0500 Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:

> > --- a/mm/rmap.c~hwpoison-hugetlbfs-fix-rss-counter-warning-fix
> > +++ a/mm/rmap.c
> > @@ -1249,14 +1249,14 @@ int try_to_unmap_one(struct page *page, 
> >  	update_hiwater_rss(mm);
> >  
> >  	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
> > -		if (PageHuge(page))
> > -			;
> > -		else if (PageAnon(page))
> > -			dec_mm_counter(mm, MM_ANONPAGES);
> > -		else
> > -			dec_mm_counter(mm, MM_FILEPAGES);
> > -		set_pte_at(mm, address, pte,
> > -				swp_entry_to_pte(make_hwpoison_entry(page)));
> > +		if (!PageHuge(page)) {
> > +			if (PageAnon(page))
> > +				dec_mm_counter(mm, MM_ANONPAGES);
> > +			else
> > +				dec_mm_counter(mm, MM_FILEPAGES);
> > +			set_pte_at(mm, address, pte,
> > +				   swp_entry_to_pte(make_hwpoison_entry(page)));
> > +		}
> 
> This set_pte_at() should come outside the if-block, or error containment
> does not work.

Doh.  C is really hard!

--- a/mm/rmap.c~hwpoison-hugetlbfs-fix-rss-counter-warning-fix-fix
+++ a/mm/rmap.c
@@ -1254,9 +1254,9 @@ int try_to_unmap_one(struct page *page, 
 				dec_mm_counter(mm, MM_ANONPAGES);
 			else
 				dec_mm_counter(mm, MM_FILEPAGES);
-			set_pte_at(mm, address, pte,
-				   swp_entry_to_pte(make_hwpoison_entry(page)));
 		}
+		set_pte_at(mm, address, pte,
+			   swp_entry_to_pte(make_hwpoison_entry(page)));
 	} else if (PageAnon(page)) {
 		swp_entry_t entry = { .val = page_private(page) };
 
_

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Naoya Horiguchi]

On Wed, Dec 05, 2012 at 10:13:42PM +0000, Luck, Tony wrote:
> > This patch fixes the warning from __list_del_entry() which is triggered
> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
> 
> Ultimately it would be nice to avoid poisoning huge pages. Generally we know the
> location of the poison to a cache line granularity (but sometimes only to a 4K
> granularity) ... and it is rather inefficient to take an entire 2M page out of service.
> With 1G pages things would be even worse!!

Thanks for the comment.
And yes, it's remaining work to be done.

> It also makes life harder for applications that would like to catch the SIGBUS
> and try to take their own recovery actions. Losing more data than they really
> need to will make it less likely that they can do something to work around the
> loss.
> 
> Has anyone looked at how hard it might be to have the code in memory-failure.c
> break up a huge page and only poison the 4K that needs to be taken out of service?

This work is one of my interest and became a bit easier than used to be,
because now transparent hugepage works commonly and some of code can be
copied from or shared with it.

Thanks,
Naoya

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Andrew Morton]

On Thu,  6 Dec 2012 21:03:44 -0500 Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:

> This patch fixes the warning from __list_del_entry() which is triggered
> when a process tries to do free_huge_page() for a hwpoisoned hugepage.
> 
> ChangeLog v2:
>  - simply use list_del_init instead of introducing new hugepage list
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/hugetlb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 59a0059..9308752 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3170,7 +3170,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
>  
>  	spin_lock(&hugetlb_lock);
>  	if (is_hugepage_on_freelist(hpage)) {
> -		list_del(&hpage->lru);
> +		list_del_init(&hpage->lru);

Can we please have a code comment in here explaining why
list_del_init() is used?

>  		set_page_refcounted(hpage);
>  		h->free_huge_pages--;
>  		h->free_huge_pages_node[nid]--;

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Aneesh Kumar K.V]

Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> writes:

> On Thu, Dec 06, 2012 at 02:36:52PM -0800, Andrew Morton wrote:
>> On Wed,  5 Dec 2012 16:47:36 -0500
>> Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
>> 
>> > This patch fixes the warning from __list_del_entry() which is triggered
>> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
>> > 
>> > Originally, page->lru of hugetlbfs head page was dangling when the
>> > hugepage was in use. This behavior has changed by commit 0edaecfab218d7
>> > ("hugetlb: add a list for tracking in-use HugeTLB pages"), where hugepages
>> > in use are linked to hugepage_activelist. HWpoisoned hugepages should not
>> > be charged to any process, so we introduce another list to link hwpoisoned
>> > hugepages.
>> > 
>> > ...
>> >
>> > --- v3.7-rc8.orig/include/linux/hugetlb.h
>> > +++ v3.7-rc8/include/linux/hugetlb.h
>> > @@ -230,6 +230,9 @@ struct hstate {
>> >  	unsigned long nr_overcommit_huge_pages;
>> >  	struct list_head hugepage_activelist;
>> >  	struct list_head hugepage_freelists[MAX_NUMNODES];
>> > +#ifdef CONFIG_MEMORY_FAILURE
>> > +	struct list_head hugepage_hwpoisonedlist;
>> > +#endif
>> >  	unsigned int nr_huge_pages_node[MAX_NUMNODES];
>> >  	unsigned int free_huge_pages_node[MAX_NUMNODES];
>> >  	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
>> > diff --git v3.7-rc8.orig/mm/hugetlb.c v3.7-rc8/mm/hugetlb.c
>> > index 59a0059..e61a749 100644
>> > --- v3.7-rc8.orig/mm/hugetlb.c
>> > +++ v3.7-rc8/mm/hugetlb.c
>> > @@ -1939,6 +1939,7 @@ void __init hugetlb_add_hstate(unsigned order)
>> >  	for (i = 0; i < MAX_NUMNODES; ++i)
>> >  		INIT_LIST_HEAD(&h->hugepage_freelists[i]);
>> >  	INIT_LIST_HEAD(&h->hugepage_activelist);
>> > +	INIT_LIST_HEAD(&h->hugepage_hwpoisonedlist);
>> >  	h->next_nid_to_alloc = first_node(node_states[N_HIGH_MEMORY]);
>> >  	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
>> >  	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
>> > @@ -3170,7 +3171,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
>> >  
>> >  	spin_lock(&hugetlb_lock);
>> >  	if (is_hugepage_on_freelist(hpage)) {
>> > -		list_del(&hpage->lru);
>> > +		list_move(&hpage->lru, &h->hugepage_hwpoisonedlist);
>> >  		set_page_refcounted(hpage);
>> >  		h->free_huge_pages--;
>> >  		h->free_huge_pages_node[nid]--;
>> 
>> Do we actually need to new list?  We could use list_del_init() to leave
>> the page's list_head pointing at itself.  In this state, it is its own
>> list_head and further list_del()s are a no-op.
>
> OK, it's better, thanks.
>
>> I don't know whether this would trigger list-debug warnings.
>
> I tested your idea (with attached patch) and confirmed that
> we never get the warnings.
>
> Thanks,
> Naoya
> ---
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Date: Thu, 6 Dec 2012 20:54:30 -0500
> Subject: [PATCH v2] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned
>  hugepage
>
> This patch fixes the warning from __list_del_entry() which is triggered
> when a process tries to do free_huge_page() for a hwpoisoned hugepage.


Can you get a dump stack for that. I am confused because the page was
already in freelist, and we deleted it from the list and set the
refcount to 1. So how are we reaching free_huge_page() again ?


>
> ChangeLog v2:
>  - simply use list_del_init instead of introducing new hugepage list
>
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/hugetlb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 59a0059..9308752 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3170,7 +3170,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
>
>  	spin_lock(&hugetlb_lock);
>  	if (is_hugepage_on_freelist(hpage)) {
> -		list_del(&hpage->lru);
> +		list_del_init(&hpage->lru);
>  		set_page_refcounted(hpage);
>  		h->free_huge_pages--;
>  		h->free_huge_pages_node[nid]--;
> -- 

-aneesh

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Naoya Horiguchi]

On Thu, Dec 06, 2012 at 06:20:24PM -0800, Andrew Morton wrote:
> On Thu,  6 Dec 2012 21:03:44 -0500 Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> 
> > This patch fixes the warning from __list_del_entry() which is triggered
> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
> > 
> > ChangeLog v2:
> >  - simply use list_del_init instead of introducing new hugepage list
> > 
> > Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > ---
> >  mm/hugetlb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 59a0059..9308752 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -3170,7 +3170,7 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
> >  
> >  	spin_lock(&hugetlb_lock);
> >  	if (is_hugepage_on_freelist(hpage)) {
> > -		list_del(&hpage->lru);
> > +		list_del_init(&hpage->lru);
> 
> Can we please have a code comment in here explaining why
> list_del_init() is used?

OK, I added it.
---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Thu, 6 Dec 2012 20:54:30 -0500
Subject: [PATCH v3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned
 hugepage

This patch fixes the warning from __list_del_entry() which is triggered
when a process tries to do free_huge_page() for a hwpoisoned hugepage.

ChangeLog v3:
 - add comment

ChangeLog v2:
 - simply use list_del_init instead of introducing new hugepage list

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/hugetlb.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 59a0059..df03345 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3170,7 +3170,12 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
 
 	spin_lock(&hugetlb_lock);
 	if (is_hugepage_on_freelist(hpage)) {
-		list_del(&hpage->lru);
+		/* 
+		 * Hwpoisoned hugepage isn't linked to activelist or freelist,
+		 * but dangling hpage->lru can trigger list-debug warnings,
+		 * so let it point to itself with list_del_init().
+		 */
+		list_del_init(&hpage->lru);
 		set_page_refcounted(hpage);
 		h->free_huge_pages--;
 		h->free_huge_pages_node[nid]--;
-- 
1.7.11.7

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Naoya Horiguchi]

On Fri, Dec 07, 2012 at 11:06:41AM +0530, Aneesh Kumar K.V wrote:
...
> > From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > Date: Thu, 6 Dec 2012 20:54:30 -0500
> > Subject: [PATCH v2] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned
> >  hugepage
> >
> > This patch fixes the warning from __list_del_entry() which is triggered
> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
> 
> 
> Can you get a dump stack for that. I am confused because the page was
> already in freelist, and we deleted it from the list and set the
> refcount to 1. So how are we reaching free_huge_page() again ?

free_huge_page() can be called for hwpoisoned hugepage from unpoison_memory().
This function gets refcount once and clears PageHWPoison, and then puts
refcount twice to return the hugepage back to free pool.
The second put_page() finally reaches free_huge_page().

Thanks,
Naoya

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Aneesh Kumar K.V]

Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> writes:

> On Fri, Dec 07, 2012 at 11:06:41AM +0530, Aneesh Kumar K.V wrote:
> ...
>> > From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> > Date: Thu, 6 Dec 2012 20:54:30 -0500
>> > Subject: [PATCH v2] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned
>> >  hugepage
>> >
>> > This patch fixes the warning from __list_del_entry() which is triggered
>> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
>> 
>> 
>> Can you get a dump stack for that. I am confused because the page was
>> already in freelist, and we deleted it from the list and set the
>> refcount to 1. So how are we reaching free_huge_page() again ?
>
> free_huge_page() can be called for hwpoisoned hugepage from unpoison_memory().
> This function gets refcount once and clears PageHWPoison, and then puts
> refcount twice to return the hugepage back to free pool.
> The second put_page() finally reaches free_huge_page().
>

Can we add this also to the commit message ?. With that you can add

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

Thanks
-aneesh

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Naoya Horiguchi]

On Fri, Dec 07, 2012 at 01:24:45PM +0530, Aneesh Kumar K.V wrote:
> Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> writes:
> 
> > On Fri, Dec 07, 2012 at 11:06:41AM +0530, Aneesh Kumar K.V wrote:
> > ...
> >> > From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> >> > Date: Thu, 6 Dec 2012 20:54:30 -0500
> >> > Subject: [PATCH v2] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned
> >> >  hugepage
> >> >
> >> > This patch fixes the warning from __list_del_entry() which is triggered
> >> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
> >> 
> >> 
> >> Can you get a dump stack for that. I am confused because the page was
> >> already in freelist, and we deleted it from the list and set the
> >> refcount to 1. So how are we reaching free_huge_page() again ?
> >
> > free_huge_page() can be called for hwpoisoned hugepage from unpoison_memory().
> > This function gets refcount once and clears PageHWPoison, and then puts
> > refcount twice to return the hugepage back to free pool.
> > The second put_page() finally reaches free_huge_page().
> >
> 
> Can we add this also to the commit message ?. With that you can add
> 
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

OK, I added it.
Thanks for the review!

Naoya
---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Thu, 6 Dec 2012 20:54:30 -0500
Subject: [PATCH v4] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned
 hugepage

This patch fixes the warning from __list_del_entry() which is triggered
when a process tries to do free_huge_page() for a hwpoisoned hugepage.

ChangeLog v4:
 - Add comment about when the warning is triggered

ChangeLog v3:
 - Add comment

ChangeLog v2:
 - simply use list_del_init instead of introducing new hugepage list

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 mm/hugetlb.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 59a0059..2511bcb 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3170,7 +3170,13 @@ int dequeue_hwpoisoned_huge_page(struct page *hpage)
 
 	spin_lock(&hugetlb_lock);
 	if (is_hugepage_on_freelist(hpage)) {
-		list_del(&hpage->lru);
+		/* 
+		 * Hwpoisoned hugepage isn't linked to activelist or freelist,
+		 * but dangling hpage->lru can trigger list-debug warnings
+		 * (this happens when we call unpoison_memory() on it),
+		 * so let it point to itself with list_del_init().
+		 */
+		list_del_init(&hpage->lru);
 		set_page_refcounted(hpage);
 		h->free_huge_pages--;
 		h->free_huge_pages_node[nid]--;
-- 
1.7.11.7

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Andrew Morton]

On Fri,  7 Dec 2012 10:49:57 -0500
Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:

> This patch fixes the warning from __list_del_entry() which is triggered
> when a process tries to do free_huge_page() for a hwpoisoned hugepage.

This changelog is very short.  In fact it is too short, resulting in
others having to ask questions about the patch.  When this happens,
please treat it as a sign that the changelog needs additional
information - so that other readers will not feel a need to ask the
same questions!

I added this paragraph:

: free_huge_page() can be called for hwpoisoned hugepage from
: unpoison_memory().  This function gets refcount once and clears
: PageHWPoison, and then puts refcount twice to return the hugepage back to
: free pool.  The second put_page() finally reaches free_huge_page().



Also, is the description accurate?  Is the __list_del_entry() warning
the only problem?

Or is it the case that this bug will cause memory corruption?  If so
then the patch is pretty important and is probably needed in -stable as
well?  I haven't checked how far back in time the bug exists.

Re: [PATCH 1/3] HWPOISON, hugetlbfs: fix warning on freeing hwpoisoned hugepage

[Naoya Horiguchi]

On Fri, Dec 07, 2012 at 02:34:14PM -0800, Andrew Morton wrote:
> On Fri,  7 Dec 2012 10:49:57 -0500
> Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
>
> > This patch fixes the warning from __list_del_entry() which is triggered
> > when a process tries to do free_huge_page() for a hwpoisoned hugepage.
>
> This changelog is very short.  In fact it is too short, resulting in
> others having to ask questions about the patch.  When this happens,
> please treat it as a sign that the changelog needs additional
> information - so that other readers will not feel a need to ask the
> same questions!

OK, I'll be careful after this.

> I added this paragraph:
>
> : free_huge_page() can be called for hwpoisoned hugepage from
> : unpoison_memory().  This function gets refcount once and clears
> : PageHWPoison, and then puts refcount twice to return the hugepage back to
> : free pool.  The second put_page() finally reaches free_huge_page().
>
>
>
> Also, is the description accurate?  Is the __list_del_entry() warning
> the only problem?

Right, this description is correct and this warning is the only problem.

> Or is it the case that this bug will cause memory corruption?  If so
> then the patch is pretty important and is probably needed in -stable as
> well?  I haven't checked how far back in time the bug exists.

There's no memory corruption even if we leave this bug unfixed, because
in unpoisoning (only way to change the status of hwpoisoned hugepage),
there are two possible operations on page->lru as shown below:

  static void free_huge_page(struct page *page)
  {
          ...
          if (h->surplus_huge_pages_node[nid] && huge_page_order(h) < MAX_ORDER) {
                  /* remove the page from active list */
                  list_del(&page->lru);
                  update_and_free_page(h, page);
                  h->surplus_huge_pages--;
                  h->surplus_huge_pages_node[nid]--;
          } else {
                  arch_clear_hugepage_flags(page);
                  enqueue_huge_page(h, page); /* list_move inside this function*/
          }
          ...
  }

, but both path do simply list_del() or list_move(), so there's no
difference (except for warning) after this block whether page->lru is
dangling or pointing to itself.

This bug was introduced recently on commit 0edaecfab218d747d30de
("hugetlb: add a list for tracking in-use HugeTLB pages").

Naoya

Re: [PATCH 3/3] HWPOISON, hugetlbfs: fix RSS-counter warning

[Simon Jeons]

On Wed, 2012-12-05 at 16:47 -0500, Naoya Horiguchi wrote:
> Memory error handling on hugepages can break a RSS counter, which emits
> a message like "Bad rss-counter state mm:ffff88040abecac0 idx:1 val:-1".
> This is because PageAnon returns true for hugepage (this behavior is

Could you explain why PageAnon returns true for hugepage?

> necessary for reverse mapping to work on hugetlbfs).
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/rmap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git v3.7-rc8.orig/mm/rmap.c v3.7-rc8/mm/rmap.c
> index 2ee1ef0..df54ef0 100644
> --- v3.7-rc8.orig/mm/rmap.c
> +++ v3.7-rc8/mm/rmap.c
> @@ -1235,7 +1235,9 @@ int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  	update_hiwater_rss(mm);
>  
>  	if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
> -		if (PageAnon(page))
> +		if (PageHuge(page))
> +			;
> +		else if (PageAnon(page))
>  			dec_mm_counter(mm, MM_ANONPAGES);
>  		else
>  			dec_mm_counter(mm, MM_FILEPAGES);