[RFC 0/2] BUG raised when onlining HWPoisoned page

[RFC 0/2] BUG raised when onlining HWPoisoned page

[Laurent Dufour]

When a page is HWPoisoned and later offlined and onlined back, a BUG
warning is raised in the kernel:

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)
raw: 003ffff800200000 0000000000000000 0000000000000001 00000000ffffffff
raw: 5deadbeef0000100 5deadbeef0000200 0000000000000000 c0000007fe055800
page dumped because: page still charged to cgroup
page->mem_cgroup:c0000007fe055800
Modules linked in: pseries_rng rng_core vmx_crypto virtio_balloon ip_tables x_tables autofs4 virtio_blk virtio_net virtio_pci virtio_ring virtio
CPU: 34 PID: 5946 Comm: mem-on-off-test Tainted: G    B 4.11.0-rc7-hwp #1
Call Trace:
[c0000007e4a737f0] [c000000000958e8c] dump_stack+0xb0/0xf0 (unreliable)
[c0000007e4a73830] [c00000000021588c] bad_page+0x11c/0x190
[c0000007e4a738c0] [c00000000021757c] free_pcppages_bulk+0x46c/0x600
[c0000007e4a73990] [c00000000021924c] free_hot_cold_page+0x2ec/0x320
[c0000007e4a739e0] [c0000000002a6440] generic_online_page+0x50/0x70
[c0000007e4a73a10] [c0000000002a6184] online_pages_range+0x94/0xe0
[c0000007e4a73a70] [c00000000005a2b0] walk_system_ram_range+0xe0/0x120
[c0000007e4a73ac0] [c0000000002cce44] online_pages+0x2b4/0x6b0
[c0000007e4a73b60] [c000000000600558] memory_subsys_online+0x218/0x270
[c0000007e4a73bf0] [c0000000005dec84] device_online+0xb4/0x110
[c0000007e4a73c30] [c000000000600f00] store_mem_state+0xc0/0x190
[c0000007e4a73c70] [c0000000005da1d4] dev_attr_store+0x34/0x60
[c0000007e4a73c90] [c000000000377c70] sysfs_kf_write+0x60/0xa0
[c0000007e4a73cb0] [c0000000003769fc] kernfs_fop_write+0x16c/0x240
[c0000007e4a73d00] [c0000000002d1b0c] __vfs_write+0x3c/0x1b0
[c0000007e4a73d90] [c0000000002d34dc] vfs_write+0xcc/0x230
[c0000007e4a73de0] [c0000000002d50e0] SyS_write+0x60/0x110
[c0000007e4a73e30] [c00000000000b760] system_call+0x38/0xfc

This has been seen on x86 kvm guest, PowerPC bare metal system and KVM
guest.

The issue is that the onlined page has already the mem_cgroup field
set.

It seems that the mem_cgroup field should be cleared when the page is
poisoned, which is done in the first patch of this series.

Then when the page is onlined back, the BUG warning is no more
triggered, but the page is now available for use, and once a process
is using it, it got killed because of the memory error.
It seems that the page should be ignored when onlined, as it is when
it is offlined (introduced by commit b023f46813cd "memory-hotplug:
skip HWPoisoned page when offlining pages"). The second patch of this
series is skipping HWPoisoned page when the memory block is onlined
back.

To be honest, I don't feel so comfortable with this series. It seems
to fix the issue, but I'm not sure this is the right way to achieve
that.

Please advise.

Laurent Dufour (2):
  mm: Uncharge poisoned pages
  mm: skip HWPoisoned pages when onlining pages

 mm/memory-failure.c | 1 +
 mm/memory_hotplug.c | 2 ++
 2 files changed, 3 insertions(+)

-- 
2.7.4

[RFC 1/2] mm: Uncharge poisoned pages

[Laurent Dufour]

When page are poisoned, they should be uncharged from the root memory
cgroup.

Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
---
 mm/memory-failure.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 27f7210e7fab..00bd39d3d4cb 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -530,6 +530,7 @@ static const char * const action_page_types[] = {
 static int delete_from_lru_cache(struct page *p)
 {
 	if (!isolate_lru_page(p)) {
+		memcg_kmem_uncharge(p, 0);
 		/*
 		 * Clear sensible page flags, so that the buddy system won't
 		 * complain when the page is unpoison-and-freed.
-- 
2.7.4

[RFC 2/2] mm: skip HWPoisoned pages when onlining pages

[Laurent Dufour]

The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
offlining pages") skip the HWPoisoned pages when offlining pages, but
this should be skipped when onlining the pages too.

Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
---
 mm/memory_hotplug.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6fa7208bcd56..20e1fadc2369 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -942,6 +942,8 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
 	if (PageReserved(pfn_to_page(start_pfn)))
 		for (i = 0; i < nr_pages; i++) {
 			page = pfn_to_page(start_pfn + i);
+			if (PageHWPoison(page))
+				continue;
 			(*online_page_callback)(page);
 			onlined_pages++;
 		}
-- 
2.7.4

Re: [RFC 1/2] mm: Uncharge poisoned pages

[Naoya Horiguchi]

On Thu, Apr 20, 2017 at 11:26:01AM +0200, Laurent Dufour wrote:
> When page are poisoned, they should be uncharged from the root memory
> cgroup.

Could you include some information about what problem this patch tries
to solve?
# I know that you already explain it in patch 0/2, so you can simply
# copy from it.

>
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
>  mm/memory-failure.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 27f7210e7fab..00bd39d3d4cb 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -530,6 +530,7 @@ static const char * const action_page_types[] = {
>  static int delete_from_lru_cache(struct page *p)
>  {
>  	if (!isolate_lru_page(p)) {
> +		memcg_kmem_uncharge(p, 0);

This function is supposed to be called with if (memcg_kmem_enabled()) check,
so could you do like below?

+		if (memcg_kmem_enabled())
+			memcg_kmem_uncharge(p, 0);


And I feel that we can call this function outside if (!isolate_lru_page(p))
block, because isolate_lru_page could fail and then the error page is left
incompletely isolated. Such error page has PageHWPoison set, so I guess that
the reported bug still triggers on such case.

Thanks,
Naoya Horiguchi

Re: [RFC 1/2] mm: Uncharge poisoned pages

[Laurent Dufour]

On 24/04/2017 11:05, Naoya Horiguchi wrote:
> On Thu, Apr 20, 2017 at 11:26:01AM +0200, Laurent Dufour wrote:
>> When page are poisoned, they should be uncharged from the root memory
>> cgroup.
> 
> Could you include some information about what problem this patch tries
> to solve?
> # I know that you already explain it in patch 0/2, so you can simply
> # copy from it.

Thanks for the review, I will add the BUG's output in the next version.

> 
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>>  mm/memory-failure.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 27f7210e7fab..00bd39d3d4cb 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -530,6 +530,7 @@ static const char * const action_page_types[] = {
>>  static int delete_from_lru_cache(struct page *p)
>>  {
>>  	if (!isolate_lru_page(p)) {
>> +		memcg_kmem_uncharge(p, 0);
> 
> This function is supposed to be called with if (memcg_kmem_enabled()) check,
> so could you do like below?
> 
> +		if (memcg_kmem_enabled())
> +			memcg_kmem_uncharge(p, 0);
> 
> 
> And I feel that we can call this function outside if (!isolate_lru_page(p))
> block, because isolate_lru_page could fail and then the error page is left
> incompletely isolated. Such error page has PageHWPoison set, so I guess that
> the reported bug still triggers on such case.

I move the call to memcg_kmem_uncharge() outside if
(!isolate_lru_page(p)) and it seems to work as well.

I'll wait a bit for any other review to come and I'll send a new version.

Thanks,
Laurent.

Re: [RFC 2/2] mm: skip HWPoisoned pages when onlining pages

[Naoya Horiguchi]

On Thu, Apr 20, 2017 at 11:26:02AM +0200, Laurent Dufour wrote:
> The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
> offlining pages") skip the HWPoisoned pages when offlining pages, but
> this should be skipped when onlining the pages too.
>
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
>  mm/memory_hotplug.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6fa7208bcd56..20e1fadc2369 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -942,6 +942,8 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
>  	if (PageReserved(pfn_to_page(start_pfn)))
>  		for (i = 0; i < nr_pages; i++) {
>  			page = pfn_to_page(start_pfn + i);
> +			if (PageHWPoison(page))
> +				continue;

Is it OK that PageReserved (set by __offline_isolated_pages for non-buddy
hwpoisoned pages) still remains in this path?
If online_pages_range() is the reverse operation of __offline_isolated_pages(),
ClearPageReserved seems needed here.

Thanks,
Naoya Horiguchi

>  			(*online_page_callback)(page);
>  			onlined_pages++;
>  		}
> --
> 2.7.4
>
>

Re: [RFC 2/2] mm: skip HWPoisoned pages when onlining pages

[Laurent Dufour]

On 25/04/2017 10:00, Naoya Horiguchi wrote:
> On Thu, Apr 20, 2017 at 11:26:02AM +0200, Laurent Dufour wrote:
>> The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
>> offlining pages") skip the HWPoisoned pages when offlining pages, but
>> this should be skipped when onlining the pages too.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>>  mm/memory_hotplug.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 6fa7208bcd56..20e1fadc2369 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -942,6 +942,8 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
>>  	if (PageReserved(pfn_to_page(start_pfn)))
>>  		for (i = 0; i < nr_pages; i++) {
>>  			page = pfn_to_page(start_pfn + i);
>> +			if (PageHWPoison(page))
>> +				continue;
> 
> Is it OK that PageReserved (set by __offline_isolated_pages for non-buddy
> hwpoisoned pages) still remains in this path?

To be honest, I've no clue.

> If online_pages_range() is the reverse operation of __offline_isolated_pages(),
> ClearPageReserved seems needed here.

I added a call to ClearPageReserved in the if (PageHWPoison(..)) and run
some tests.
This seems to work fine as well, but I'm not sure about the side effect.

I'll add it to my next version.

Thanks,
Laurent.