Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630553 > unrolled thread

[PATCH v2 0/2] BUG raised when onlining HWPoisoned page

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] BUG raised when onlining HWPoisoned page Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-04-25 16:30 +0200
    [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-04-25 16:30 +0200
      Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Balbir Singh <bsingharora@gmail.com> - 2017-04-26 04:20 +0200
        Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-04-26 05:30 +0200
          Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Balbir Singh <bsingharora@gmail.com> - 2017-04-28 05:00 +0200
          Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Michal Hocko <mhocko@kernel.org> - 2017-04-28 08:40 +0200
            Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Michal Hocko <mhocko@kernel.org> - 2017-04-28 09:00 +0200
              Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Michal Hocko <mhocko@kernel.org> - 2017-05-10 09:50 +0200
            Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages Michal Hocko <mhocko@kernel.org> - 2017-04-28 09:00 +0200
    [PATCH v2 1/2] mm: Uncharge poisoned pages Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-04-25 16:30 +0200
      Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-04-26 02:00 +0200
      Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Balbir Singh <bsingharora@gmail.com> - 2017-04-26 04:00 +0200
        Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-04-26 05:20 +0200
          Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Balbir Singh <bsingharora@gmail.com> - 2017-04-26 05:50 +0200
            Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-04-26 06:50 +0200
              Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Balbir Singh <bsingharora@gmail.com> - 2017-04-26 11:10 +0200
                Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-04-28 11:40 +0200
      Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Michal Hocko <mhocko@kernel.org> - 2017-04-27 16:40 +0200
        Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Andi Kleen <andi@firstfloor.org> - 2017-04-27 23:00 +0200
          Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Michal Hocko <mhocko@kernel.org> - 2017-04-28 08:10 +0200
            Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Michal Hocko <mhocko@kernel.org> - 2017-04-28 09:40 +0200
              Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-04-28 11:20 +0200
                Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Michal Hocko <mhocko@kernel.org> - 2017-04-28 15:50 +0200
                  Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-05-02 17:00 +0200
                    Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Michal Hocko <mhocko@kernel.org> - 2017-05-02 21:00 +0200
                      Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-05-03 13:40 +0200
                      Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Balbir Singh <bsingharora@gmail.com> - 2017-05-04 03:30 +0200
                        Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-05-08 12:50 +0200
                          Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Balbir Singh <bsingharora@gmail.com> - 2017-05-09 03:50 +0200
                      Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-05-08 05:10 +0200
                        Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Michal Hocko <mhocko@kernel.org> - 2017-05-09 11:20 +0200
                          Re: [PATCH v2 1/2] mm: Uncharge poisoned pages Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-05-10 01:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1632581 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

[CC Johannes and Vladimir - the patch is
http://lkml.kernel.org/r/1493130472-22843-2-git-send-email-ldufour@linux.vnet.ibm.com]

On Fri 28-04-17 08:07:55, Michal Hocko wrote:
> On Thu 27-04-17 13:51:23, Andi Kleen wrote:
> > Michal Hocko <mhocko@kernel.org> writes:
> > 
> > > On Tue 25-04-17 16:27:51, Laurent Dufour wrote:
> > >> When page are poisoned, they should be uncharged from the root memory
> > >> cgroup.
> > >> 
> > >> This is required to avoid a BUG raised when the page is onlined back:
> > >> BUG: Bad page state in process mem-on-off-test  pfn:7ae3b
> > >> page:f000000001eb8ec0 count:0 mapcount:0 mapping:          (null)
> > >> index:0x1
> > >> flags: 0x3ffff800200000(hwpoison)
> > >
> > > My knowledge of memory poisoning is very rudimentary but aren't those
> > > pages supposed to leak and never come back? In other words isn't the
> > > hoplug code broken because it should leave them alone?
> > 
> > Yes that would be the right interpretation. If it was really offlined
> > due to a hardware error the memory will be poisoned and any access
> > could cause a machine check.
> 
> OK, thanks for the clarification. Then I am not sure the patch is
> correct. Why do we need to uncharge that page at all?

Now, I have realized that we actually want to uncharge that page because
it will pin the memcg and we do not want to have that memcg and its
whole hierarchy pinned as well. This used to work before the charge
rework 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") I guess
because we used to uncharge on page cache removal.

I do not think the patch is correct, though. memcg_kmem_enabled() will
check whether kmem accounting is enabled and we are talking about page
cache pages here. You should be using mem_cgroup_uncharge instead.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1632713 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On 28/04/2017 09:31, Michal Hocko wrote:
> [CC Johannes and Vladimir - the patch is
> http://lkml.kernel.org/r/1493130472-22843-2-git-send-email-ldufour@linux.vnet.ibm.com]
> 
> On Fri 28-04-17 08:07:55, Michal Hocko wrote:
>> On Thu 27-04-17 13:51:23, Andi Kleen wrote:
>>> Michal Hocko <mhocko@kernel.org> writes:
>>>
>>>> On Tue 25-04-17 16:27:51, Laurent Dufour wrote:
>>>>> When page are poisoned, they should be uncharged from the root memory
>>>>> cgroup.
>>>>>
>>>>> This is required to avoid a BUG raised when the page is onlined back:
>>>>> BUG: Bad page state in process mem-on-off-test  pfn:7ae3b
>>>>> page:f000000001eb8ec0 count:0 mapcount:0 mapping:          (null)
>>>>> index:0x1
>>>>> flags: 0x3ffff800200000(hwpoison)
>>>>
>>>> My knowledge of memory poisoning is very rudimentary but aren't those
>>>> pages supposed to leak and never come back? In other words isn't the
>>>> hoplug code broken because it should leave them alone?
>>>
>>> Yes that would be the right interpretation. If it was really offlined
>>> due to a hardware error the memory will be poisoned and any access
>>> could cause a machine check.
>>
>> OK, thanks for the clarification. Then I am not sure the patch is
>> correct. Why do we need to uncharge that page at all?
> 
> Now, I have realized that we actually want to uncharge that page because
> it will pin the memcg and we do not want to have that memcg and its
> whole hierarchy pinned as well. This used to work before the charge
> rework 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") I guess
> because we used to uncharge on page cache removal.
> 
> I do not think the patch is correct, though. memcg_kmem_enabled() will
> check whether kmem accounting is enabled and we are talking about page
> cache pages here. You should be using mem_cgroup_uncharge instead.

Thanks for the review Michal.

I was not comfortable either with this patch.

I did some tests calling mem_cgroup_uncharge() when isolate_lru_page()
succeeds only, so not calling it if isolate_lru_page() failed.

This seems to work as well, so if everyone agree on that, I'll send a
new version soon.

Cheers,
Laurent.

[toc] | [prev] | [next] | [standalone]

#1632904 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On Fri 28-04-17 11:17:34, Laurent Dufour wrote:
> On 28/04/2017 09:31, Michal Hocko wrote:
> > [CC Johannes and Vladimir - the patch is
> > http://lkml.kernel.org/r/1493130472-22843-2-git-send-email-ldufour@linux.vnet.ibm.com]
> > 
> > On Fri 28-04-17 08:07:55, Michal Hocko wrote:
> >> On Thu 27-04-17 13:51:23, Andi Kleen wrote:
> >>> Michal Hocko <mhocko@kernel.org> writes:
> >>>
> >>>> On Tue 25-04-17 16:27:51, Laurent Dufour wrote:
> >>>>> When page are poisoned, they should be uncharged from the root memory
> >>>>> cgroup.
> >>>>>
> >>>>> This is required to avoid a BUG raised when the page is onlined back:
> >>>>> BUG: Bad page state in process mem-on-off-test  pfn:7ae3b
> >>>>> page:f000000001eb8ec0 count:0 mapcount:0 mapping:          (null)
> >>>>> index:0x1
> >>>>> flags: 0x3ffff800200000(hwpoison)
> >>>>
> >>>> My knowledge of memory poisoning is very rudimentary but aren't those
> >>>> pages supposed to leak and never come back? In other words isn't the
> >>>> hoplug code broken because it should leave them alone?
> >>>
> >>> Yes that would be the right interpretation. If it was really offlined
> >>> due to a hardware error the memory will be poisoned and any access
> >>> could cause a machine check.
> >>
> >> OK, thanks for the clarification. Then I am not sure the patch is
> >> correct. Why do we need to uncharge that page at all?
> > 
> > Now, I have realized that we actually want to uncharge that page because
> > it will pin the memcg and we do not want to have that memcg and its
> > whole hierarchy pinned as well. This used to work before the charge
> > rework 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") I guess
> > because we used to uncharge on page cache removal.
> > 
> > I do not think the patch is correct, though. memcg_kmem_enabled() will
> > check whether kmem accounting is enabled and we are talking about page
> > cache pages here. You should be using mem_cgroup_uncharge instead.
> 
> Thanks for the review Michal.
> 
> I was not comfortable either with this patch.
> 
> I did some tests calling mem_cgroup_uncharge() when isolate_lru_page()
> succeeds only, so not calling it if isolate_lru_page() failed.

Wait a moment. This cannot possibly work. isolate_lru_page asumes page
count > 0 and increments the counter so the resulting page count is > 1
I have only now realized that we have VM_BUG_ON_PAGE(page_count(page), page)
in uncharge_list().

This is getting quite hairy. What is the expected page count of the
hwpoison page? I guess we would need to update the VM_BUG_ON in the
memcg uncharge code to ignore the page count of hwpoison pages if it can
be arbitrary.

Before we go any further, is there any documentation about the expected
behavior and the state of the hwpoison pages? I have a very bad feeling
that the current behavior is quite arbitrary and "testing driven"
holes plugging will make it only more messy. So let's start with the
clear description of what should happen with the hwpoison pages.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1634474 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On 28/04/2017 15:48, Michal Hocko wrote:
> On Fri 28-04-17 11:17:34, Laurent Dufour wrote:
>> On 28/04/2017 09:31, Michal Hocko wrote:
>>> [CC Johannes and Vladimir - the patch is
>>> http://lkml.kernel.org/r/1493130472-22843-2-git-send-email-ldufour@linux.vnet.ibm.com]
>>>
>>> On Fri 28-04-17 08:07:55, Michal Hocko wrote:
>>>> On Thu 27-04-17 13:51:23, Andi Kleen wrote:
>>>>> Michal Hocko <mhocko@kernel.org> writes:
>>>>>
>>>>>> On Tue 25-04-17 16:27:51, Laurent Dufour wrote:
>>>>>>> When page are poisoned, they should be uncharged from the root memory
>>>>>>> cgroup.
>>>>>>>
>>>>>>> This is required to avoid a BUG raised when the page is onlined back:
>>>>>>> BUG: Bad page state in process mem-on-off-test  pfn:7ae3b
>>>>>>> page:f000000001eb8ec0 count:0 mapcount:0 mapping:          (null)
>>>>>>> index:0x1
>>>>>>> flags: 0x3ffff800200000(hwpoison)
>>>>>>
>>>>>> My knowledge of memory poisoning is very rudimentary but aren't those
>>>>>> pages supposed to leak and never come back? In other words isn't the
>>>>>> hoplug code broken because it should leave them alone?
>>>>>
>>>>> Yes that would be the right interpretation. If it was really offlined
>>>>> due to a hardware error the memory will be poisoned and any access
>>>>> could cause a machine check.
>>>>
>>>> OK, thanks for the clarification. Then I am not sure the patch is
>>>> correct. Why do we need to uncharge that page at all?
>>>
>>> Now, I have realized that we actually want to uncharge that page because
>>> it will pin the memcg and we do not want to have that memcg and its
>>> whole hierarchy pinned as well. This used to work before the charge
>>> rework 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") I guess
>>> because we used to uncharge on page cache removal.
>>>
>>> I do not think the patch is correct, though. memcg_kmem_enabled() will
>>> check whether kmem accounting is enabled and we are talking about page
>>> cache pages here. You should be using mem_cgroup_uncharge instead.
>>
>> Thanks for the review Michal.
>>
>> I was not comfortable either with this patch.
>>
>> I did some tests calling mem_cgroup_uncharge() when isolate_lru_page()
>> succeeds only, so not calling it if isolate_lru_page() failed.
> 
> Wait a moment. This cannot possibly work. isolate_lru_page asumes page
> count > 0 and increments the counter so the resulting page count is > 1
> I have only now realized that we have VM_BUG_ON_PAGE(page_count(page), page)
> in uncharge_list().

My mistake, my kernel was not build with CONFIG_DEBUG_VM set.
You're right this cannot work this way.

> This is getting quite hairy. What is the expected page count of the
> hwpoison page? I guess we would need to update the VM_BUG_ON in the
> memcg uncharge code to ignore the page count of hwpoison pages if it can
> be arbitrary.

Based on the experiment I did, page count == 2 when isolate_lru_page()
succeeds, even in the case of a poisoned page. In my case I think this
is because the page is still used by the process which is calling madvise().

I'm wondering if I'm looking at the right place. May be the poisoned
page should remain attach to the memory_cgroup until no one is using it.
In that case this means that something should be done when the page is
off-lined... I've to dig further here.

> 
> Before we go any further, is there any documentation about the expected
> behavior and the state of the hwpoison pages? I have a very bad feeling
> that the current behavior is quite arbitrary and "testing driven"
> holes plugging will make it only more messy. So let's start with the
> clear description of what should happen with the hwpoison pages.

I didn't find any documentation about that. The root cause is that a bug
message is displayed when a poisoned page is off-lined, may be this is
in that path that something is missing.

Cheers,
Laurent.

[toc] | [prev] | [next] | [standalone]

#1634581 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On Tue 02-05-17 16:59:30, Laurent Dufour wrote:
> On 28/04/2017 15:48, Michal Hocko wrote:
[...]
> > This is getting quite hairy. What is the expected page count of the
> > hwpoison page?

OK, so from the quick check of the hwpoison code it seems that the ref
count will be > 1 (from get_hwpoison_page).

> > I guess we would need to update the VM_BUG_ON in the
> > memcg uncharge code to ignore the page count of hwpoison pages if it can
> > be arbitrary.
> 
> Based on the experiment I did, page count == 2 when isolate_lru_page()
> succeeds, even in the case of a poisoned page.

that would make some sense to me. The page should have been already
unmapped therefore but memory_failure increases the ref count and 1 is
for isolate_lru_page().

> In my case I think this
> is because the page is still used by the process which is calling madvise().
> 
> I'm wondering if I'm looking at the right place. May be the poisoned
> page should remain attach to the memory_cgroup until no one is using it.
> In that case this means that something should be done when the page is
> off-lined... I've to dig further here.

No, AFAIU the page will not drop the reference count down to 0 in most
cases. Maybe there are some scenarios where this can happen but I would
expect that the poisoned page will be mapped and in use most of the time
and won't drop down 0. And then we should really uncharge it because it
will pin the memcg and make it unfreeable which doesn't seem to be what
we want.  So does the following work reasonable? Andi, Johannes, what do
you think? I cannot say I would be really comfortable touching hwpoison
code as I really do not understand the workflow. Maybe we want to move
this uncharge down to memory_failure() right before we report success?
---
From 8bf0791bcf35996a859b6d33fb5494e5b53de49d Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 2 May 2017 20:32:24 +0200
Subject: [PATCH] hwpoison, memcg: forcibly uncharge LRU pages

Laurent Dufour has noticed that hwpoinsoned pages are kept charged. In
his particular case he has hit a bad_page("page still charged to cgroup")
when onlining a hwpoison page. While this looks like something that shouldn't
happen in the first place because onlining hwpages and returning them to
the page allocator makes only little sense it shows a real problem.

hwpoison pages do not get freed usually so we do not uncharge them (at
least not since 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")).
Each charge pins memcg (since e8ea14cc6ead ("mm: memcontrol: take a css
reference for each charged page")) as well and so the mem_cgroup and the
associated state will never go away. Fix this leak by forcibly
uncharging a LRU hwpoisoned page in delete_from_lru_cache(). We also
have to tweak uncharge_list because it cannot rely on zero ref count
for these pages.

Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memcontrol.c     | 2 +-
 mm/memory-failure.c | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 16c556ac103d..4cf26059adb1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5527,7 +5527,7 @@ static void uncharge_list(struct list_head *page_list)
 		next = page->lru.next;
 
 		VM_BUG_ON_PAGE(PageLRU(page), page);
-		VM_BUG_ON_PAGE(page_count(page), page);
+		VM_BUG_ON_PAGE(!PageHWPoison(page) && page_count(page), page);
 
 		if (!page->mem_cgroup)
 			continue;
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 8a6bd3a9eb1e..4497d9619bb4 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -541,6 +541,13 @@ static int delete_from_lru_cache(struct page *p)
 		 */
 		ClearPageActive(p);
 		ClearPageUnevictable(p);
+
+		/*
+		 * Poisoned page might never drop its ref count to 0 so we have to
+		 * uncharge it manually from its memcg.
+		 */
+		mem_cgroup_uncharge(p);
+
 		/*
 		 * drop the page count elevated by isolate_lru_page()
 		 */
-- 
2.11.0

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1634899 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On 02/05/2017 20:55, Michal Hocko wrote:
> On Tue 02-05-17 16:59:30, Laurent Dufour wrote:
>> On 28/04/2017 15:48, Michal Hocko wrote:
> [...]
>>> This is getting quite hairy. What is the expected page count of the
>>> hwpoison page?
> 
> OK, so from the quick check of the hwpoison code it seems that the ref
> count will be > 1 (from get_hwpoison_page).
> 
>>> I guess we would need to update the VM_BUG_ON in the
>>> memcg uncharge code to ignore the page count of hwpoison pages if it can
>>> be arbitrary.
>>
>> Based on the experiment I did, page count == 2 when isolate_lru_page()
>> succeeds, even in the case of a poisoned page.
> 
> that would make some sense to me. The page should have been already
> unmapped therefore but memory_failure increases the ref count and 1 is
> for isolate_lru_page().
> 
>> In my case I think this
>> is because the page is still used by the process which is calling madvise().
>>
>> I'm wondering if I'm looking at the right place. May be the poisoned
>> page should remain attach to the memory_cgroup until no one is using it.
>> In that case this means that something should be done when the page is
>> off-lined... I've to dig further here.
> 
> No, AFAIU the page will not drop the reference count down to 0 in most
> cases. Maybe there are some scenarios where this can happen but I would
> expect that the poisoned page will be mapped and in use most of the time
> and won't drop down 0. And then we should really uncharge it because it
> will pin the memcg and make it unfreeable which doesn't seem to be what
> we want.  So does the following work reasonable? Andi, Johannes, what do
> you think? I cannot say I would be really comfortable touching hwpoison
> code as I really do not understand the workflow. Maybe we want to move
> this uncharge down to memory_failure() right before we report success?
> ---
> From 8bf0791bcf35996a859b6d33fb5494e5b53de49d Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 2 May 2017 20:32:24 +0200
> Subject: [PATCH] hwpoison, memcg: forcibly uncharge LRU pages
> 
> Laurent Dufour has noticed that hwpoinsoned pages are kept charged. In
> his particular case he has hit a bad_page("page still charged to cgroup")
> when onlining a hwpoison page. While this looks like something that shouldn't
> happen in the first place because onlining hwpages and returning them to
> the page allocator makes only little sense it shows a real problem.
> 
> hwpoison pages do not get freed usually so we do not uncharge them (at
> least not since 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")).
> Each charge pins memcg (since e8ea14cc6ead ("mm: memcontrol: take a css
> reference for each charged page")) as well and so the mem_cgroup and the
> associated state will never go away. Fix this leak by forcibly
> uncharging a LRU hwpoisoned page in delete_from_lru_cache(). We also
> have to tweak uncharge_list because it cannot rely on zero ref count
> for these pages.
> 
> Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>

FWIW:
Tested-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>

> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/memcontrol.c     | 2 +-
>  mm/memory-failure.c | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 16c556ac103d..4cf26059adb1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5527,7 +5527,7 @@ static void uncharge_list(struct list_head *page_list)
>  		next = page->lru.next;
> 
>  		VM_BUG_ON_PAGE(PageLRU(page), page);
> -		VM_BUG_ON_PAGE(page_count(page), page);
> +		VM_BUG_ON_PAGE(!PageHWPoison(page) && page_count(page), page);
> 
>  		if (!page->mem_cgroup)
>  			continue;
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 8a6bd3a9eb1e..4497d9619bb4 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -541,6 +541,13 @@ static int delete_from_lru_cache(struct page *p)
>  		 */
>  		ClearPageActive(p);
>  		ClearPageUnevictable(p);
> +
> +		/*
> +		 * Poisoned page might never drop its ref count to 0 so we have to
> +		 * uncharge it manually from its memcg.
> +		 */
> +		mem_cgroup_uncharge(p);
> +
>  		/*
>  		 * drop the page count elevated by isolate_lru_page()
>  		 */
> 

[toc] | [prev] | [next] | [standalone]

#1635363 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

> @@ -5527,7 +5527,7 @@ static void uncharge_list(struct list_head *page_list)
>  		next = page->lru.next;
>  
>  		VM_BUG_ON_PAGE(PageLRU(page), page);
> -		VM_BUG_ON_PAGE(page_count(page), page);
> +		VM_BUG_ON_PAGE(!PageHWPoison(page) && page_count(page), page);
>  
>  		if (!page->mem_cgroup)
>  			continue;
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 8a6bd3a9eb1e..4497d9619bb4 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -541,6 +541,13 @@ static int delete_from_lru_cache(struct page *p)
>  		 */
>  		ClearPageActive(p);
>  		ClearPageUnevictable(p);
> +
> +		/*
> +		 * Poisoned page might never drop its ref count to 0 so we have to
> +		 * uncharge it manually from its memcg.
> +		 */
> +		mem_cgroup_uncharge(p);
> +

Yep, that is the right fix

https://lkml.org/lkml/2017/4/26/133

Reviewed-by: Balbir Singh <bsingharora@gmail.com>

[toc] | [prev] | [next] | [standalone]

#1637369 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On 04/05/2017 03:21, Balbir Singh wrote:
>> @@ -5527,7 +5527,7 @@ static void uncharge_list(struct list_head *page_list)
>>  		next = page->lru.next;
>>  
>>  		VM_BUG_ON_PAGE(PageLRU(page), page);
>> -		VM_BUG_ON_PAGE(page_count(page), page);
>> +		VM_BUG_ON_PAGE(!PageHWPoison(page) && page_count(page), page);
>>  
>>  		if (!page->mem_cgroup)
>>  			continue;
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 8a6bd3a9eb1e..4497d9619bb4 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -541,6 +541,13 @@ static int delete_from_lru_cache(struct page *p)
>>  		 */
>>  		ClearPageActive(p);
>>  		ClearPageUnevictable(p);
>> +
>> +		/*
>> +		 * Poisoned page might never drop its ref count to 0 so we have to
>> +		 * uncharge it manually from its memcg.
>> +		 */
>> +		mem_cgroup_uncharge(p);
>> +
> 
> Yep, that is the right fix
> 
> https://lkml.org/lkml/2017/4/26/133

Sorry Balbir,

You pointed this out since the beginning but I missed your comment.
My mistake.

Thanks,
Laurent.

[toc] | [prev] | [next] | [standalone]

#1637813 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On Mon, 2017-05-08 at 12:42 +0200, Laurent Dufour wrote:
> Sorry Balbir,
> 
> You pointed this out since the beginning but I missed your comment.
> My mistake.
>

No worries, as long as the right thing gets in

Balbir Singh 

[toc] | [prev] | [next] | [standalone]

#1637194 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On Tue, May 02, 2017 at 08:55:07PM +0200, Michal Hocko wrote:
> On Tue 02-05-17 16:59:30, Laurent Dufour wrote:
> > On 28/04/2017 15:48, Michal Hocko wrote:
> [...]
> > > This is getting quite hairy. What is the expected page count of the
> > > hwpoison page?
> 
> OK, so from the quick check of the hwpoison code it seems that the ref
> count will be > 1 (from get_hwpoison_page).
> 
> > > I guess we would need to update the VM_BUG_ON in the
> > > memcg uncharge code to ignore the page count of hwpoison pages if it can
> > > be arbitrary.
> > 
> > Based on the experiment I did, page count == 2 when isolate_lru_page()
> > succeeds, even in the case of a poisoned page.
> 
> that would make some sense to me. The page should have been already
> unmapped therefore but memory_failure increases the ref count and 1 is
> for isolate_lru_page().

# sorry for late reply, I was on holidays last week...

Right, and the refcount taken for memory_failure is not freed after
memory_failure() returns. unpoison_memory() does free the refcount.

> 
> > In my case I think this
> > is because the page is still used by the process which is calling madvise().
> > 
> > I'm wondering if I'm looking at the right place. May be the poisoned
> > page should remain attach to the memory_cgroup until no one is using it.
> > In that case this means that something should be done when the page is
> > off-lined... I've to dig further here.
> 
> No, AFAIU the page will not drop the reference count down to 0 in most
> cases. Maybe there are some scenarios where this can happen but I would
> expect that the poisoned page will be mapped and in use most of the time
> and won't drop down 0. And then we should really uncharge it because it
> will pin the memcg and make it unfreeable which doesn't seem to be what
> we want.  So does the following work reasonable? Andi, Johannes, what do
> you think? I cannot say I would be really comfortable touching hwpoison
> code as I really do not understand the workflow. Maybe we want to move
> this uncharge down to memory_failure() right before we report success?

memory_failure() can be called for any types of page (including slab or
any kernel/driver pages), and the reported problem seems happen only on
in-use user pages, so uncharging in delete_from_lru_cache() as done below
looks better to me.

> ---
> From 8bf0791bcf35996a859b6d33fb5494e5b53de49d Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 2 May 2017 20:32:24 +0200
> Subject: [PATCH] hwpoison, memcg: forcibly uncharge LRU pages
> 
> Laurent Dufour has noticed that hwpoinsoned pages are kept charged. In
> his particular case he has hit a bad_page("page still charged to cgroup")
> when onlining a hwpoison page.

> While this looks like something that shouldn't
> happen in the first place because onlining hwpages and returning them to
> the page allocator makes only little sense it shows a real problem.
> 
> hwpoison pages do not get freed usually so we do not uncharge them (at
> least not since 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")).
> Each charge pins memcg (since e8ea14cc6ead ("mm: memcontrol: take a css
> reference for each charged page")) as well and so the mem_cgroup and the
> associated state will never go away. Fix this leak by forcibly
> uncharging a LRU hwpoisoned page in delete_from_lru_cache(). We also
> have to tweak uncharge_list because it cannot rely on zero ref count
> for these pages.
> 
> Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

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

> ---
>  mm/memcontrol.c     | 2 +-
>  mm/memory-failure.c | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 16c556ac103d..4cf26059adb1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5527,7 +5527,7 @@ static void uncharge_list(struct list_head *page_list)
>  		next = page->lru.next;
>  
>  		VM_BUG_ON_PAGE(PageLRU(page), page);
> -		VM_BUG_ON_PAGE(page_count(page), page);
> +		VM_BUG_ON_PAGE(!PageHWPoison(page) && page_count(page), page);
>  
>  		if (!page->mem_cgroup)
>  			continue;
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 8a6bd3a9eb1e..4497d9619bb4 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -541,6 +541,13 @@ static int delete_from_lru_cache(struct page *p)
>  		 */
>  		ClearPageActive(p);
>  		ClearPageUnevictable(p);
> +
> +		/*
> +		 * Poisoned page might never drop its ref count to 0 so we have to
> +		 * uncharge it manually from its memcg.
> +		 */
> +		mem_cgroup_uncharge(p);
> +
>  		/*
>  		 * drop the page count elevated by isolate_lru_page()
>  		 */
> -- 
> 2.11.0
> 
> -- 
> Michal Hocko
> SUSE Labs
> 

[toc] | [prev] | [next] | [standalone]

#1637999 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On Mon 08-05-17 02:58:36, Naoya Horiguchi wrote:
> On Tue, May 02, 2017 at 08:55:07PM +0200, Michal Hocko wrote:
> > On Tue 02-05-17 16:59:30, Laurent Dufour wrote:
> > > On 28/04/2017 15:48, Michal Hocko wrote:
> > [...]
> > > > This is getting quite hairy. What is the expected page count of the
> > > > hwpoison page?
> > 
> > OK, so from the quick check of the hwpoison code it seems that the ref
> > count will be > 1 (from get_hwpoison_page).
> > 
> > > > I guess we would need to update the VM_BUG_ON in the
> > > > memcg uncharge code to ignore the page count of hwpoison pages if it can
> > > > be arbitrary.
> > > 
> > > Based on the experiment I did, page count == 2 when isolate_lru_page()
> > > succeeds, even in the case of a poisoned page.
> > 
> > that would make some sense to me. The page should have been already
> > unmapped therefore but memory_failure increases the ref count and 1 is
> > for isolate_lru_page().
> 
> # sorry for late reply, I was on holidays last week...
> 
> Right, and the refcount taken for memory_failure is not freed after
> memory_failure() returns. unpoison_memory() does free the refcount.

OK, from the charge POV this would be safe because we clear page->memcg
so it wouldn't get uncharged more times.

> > > In my case I think this
> > > is because the page is still used by the process which is calling madvise().
> > > 
> > > I'm wondering if I'm looking at the right place. May be the poisoned
> > > page should remain attach to the memory_cgroup until no one is using it.
> > > In that case this means that something should be done when the page is
> > > off-lined... I've to dig further here.
> > 
> > No, AFAIU the page will not drop the reference count down to 0 in most
> > cases. Maybe there are some scenarios where this can happen but I would
> > expect that the poisoned page will be mapped and in use most of the time
> > and won't drop down 0. And then we should really uncharge it because it
> > will pin the memcg and make it unfreeable which doesn't seem to be what
> > we want.  So does the following work reasonable? Andi, Johannes, what do
> > you think? I cannot say I would be really comfortable touching hwpoison
> > code as I really do not understand the workflow. Maybe we want to move
> > this uncharge down to memory_failure() right before we report success?
> 
> memory_failure() can be called for any types of page (including slab or
> any kernel/driver pages), and the reported problem seems happen only on
> in-use user pages, so uncharging in delete_from_lru_cache() as done below
> looks better to me.

Yeah, we do see problems only for LRU/page cache pages but my
understanding is that error_states (e.g. me_kernel for the kernel
memory) might change in the future and then we wouldn't catch the same
bug, no?

> > ---
> > From 8bf0791bcf35996a859b6d33fb5494e5b53de49d Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Tue, 2 May 2017 20:32:24 +0200
> > Subject: [PATCH] hwpoison, memcg: forcibly uncharge LRU pages
> > 
> > Laurent Dufour has noticed that hwpoinsoned pages are kept charged. In
> > his particular case he has hit a bad_page("page still charged to cgroup")
> > when onlining a hwpoison page.
> 
> > While this looks like something that shouldn't
> > happen in the first place because onlining hwpages and returning them to
> > the page allocator makes only little sense it shows a real problem.
> > 
> > hwpoison pages do not get freed usually so we do not uncharge them (at
> > least not since 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")).
> > Each charge pins memcg (since e8ea14cc6ead ("mm: memcontrol: take a css
> > reference for each charged page")) as well and so the mem_cgroup and the
> > associated state will never go away. Fix this leak by forcibly
> > uncharging a LRU hwpoisoned page in delete_from_lru_cache(). We also
> > have to tweak uncharge_list because it cannot rely on zero ref count
> > for these pages.
> > 
> > Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
> > Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks! I will wait a day or two for Johannes and repost the patch.
Andrew could you drop
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-uncharge-poisoned-pages.patch
in the mean time, please?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1638441 — Re: [PATCH v2 1/2] mm: Uncharge poisoned pages

On Tue, May 09, 2017 at 11:18:23AM +0200, Michal Hocko wrote:
> On Mon 08-05-17 02:58:36, Naoya Horiguchi wrote:
> > On Tue, May 02, 2017 at 08:55:07PM +0200, Michal Hocko wrote:
> > > On Tue 02-05-17 16:59:30, Laurent Dufour wrote:
> > > > On 28/04/2017 15:48, Michal Hocko wrote:
> > > [...]
> > > > > This is getting quite hairy. What is the expected page count of the
> > > > > hwpoison page?
> > > 
> > > OK, so from the quick check of the hwpoison code it seems that the ref
> > > count will be > 1 (from get_hwpoison_page).
> > > 
> > > > > I guess we would need to update the VM_BUG_ON in the
> > > > > memcg uncharge code to ignore the page count of hwpoison pages if it can
> > > > > be arbitrary.
> > > > 
> > > > Based on the experiment I did, page count == 2 when isolate_lru_page()
> > > > succeeds, even in the case of a poisoned page.
> > > 
> > > that would make some sense to me. The page should have been already
> > > unmapped therefore but memory_failure increases the ref count and 1 is
> > > for isolate_lru_page().
> > 
> > # sorry for late reply, I was on holidays last week...
> > 
> > Right, and the refcount taken for memory_failure is not freed after
> > memory_failure() returns. unpoison_memory() does free the refcount.
> 
> OK, from the charge POV this would be safe because we clear page->memcg
> so it wouldn't get uncharged more times.
> 
> > > > In my case I think this
> > > > is because the page is still used by the process which is calling madvise().
> > > > 
> > > > I'm wondering if I'm looking at the right place. May be the poisoned
> > > > page should remain attach to the memory_cgroup until no one is using it.
> > > > In that case this means that something should be done when the page is
> > > > off-lined... I've to dig further here.
> > > 
> > > No, AFAIU the page will not drop the reference count down to 0 in most
> > > cases. Maybe there are some scenarios where this can happen but I would
> > > expect that the poisoned page will be mapped and in use most of the time
> > > and won't drop down 0. And then we should really uncharge it because it
> > > will pin the memcg and make it unfreeable which doesn't seem to be what
> > > we want.  So does the following work reasonable? Andi, Johannes, what do
> > > you think? I cannot say I would be really comfortable touching hwpoison
> > > code as I really do not understand the workflow. Maybe we want to move
> > > this uncharge down to memory_failure() right before we report success?
> > 
> > memory_failure() can be called for any types of page (including slab or
> > any kernel/driver pages), and the reported problem seems happen only on
> > in-use user pages, so uncharging in delete_from_lru_cache() as done below
> > looks better to me.
> 
> Yeah, we do see problems only for LRU/page cache pages but my
> understanding is that error_states (e.g. me_kernel for the kernel
> memory) might change in the future and then we wouldn't catch the same
> bug, no?

Right about future change, and we will see the same bug. I guess that the
first target of kernel page is slab page, and memcg_kmem_uncharge() will
be used there. Implementors/Reviewers should care about uncharging when the
time comes.

Thanks,
Naoya Horiguchi

> 
> > > ---
> > > From 8bf0791bcf35996a859b6d33fb5494e5b53de49d Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <mhocko@suse.com>
> > > Date: Tue, 2 May 2017 20:32:24 +0200
> > > Subject: [PATCH] hwpoison, memcg: forcibly uncharge LRU pages
> > > 
> > > Laurent Dufour has noticed that hwpoinsoned pages are kept charged. In
> > > his particular case he has hit a bad_page("page still charged to cgroup")
> > > when onlining a hwpoison page.
> > 
> > > While this looks like something that shouldn't
> > > happen in the first place because onlining hwpages and returning them to
> > > the page allocator makes only little sense it shows a real problem.
> > > 
> > > hwpoison pages do not get freed usually so we do not uncharge them (at
> > > least not since 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")).
> > > Each charge pins memcg (since e8ea14cc6ead ("mm: memcontrol: take a css
> > > reference for each charged page")) as well and so the mem_cgroup and the
> > > associated state will never go away. Fix this leak by forcibly
> > > uncharging a LRU hwpoisoned page in delete_from_lru_cache(). We also
> > > have to tweak uncharge_list because it cannot rely on zero ref count
> > > for these pages.
> > > 
> > > Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
> > > Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > 
> > Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> 
> Thanks! I will wait a day or two for Johannes and repost the patch.
> Andrew could you drop
> http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-uncharge-poisoned-pages.patch
> in the mean time, please?
> 
> -- 
> Michal Hocko
> SUSE Labs
> 

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web