linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] HWpoison: further fixes and cleanups
@ 2020-09-02  9:45 Oscar Salvador
  2020-09-02  9:45 ` [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists Oscar Salvador
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Oscar Salvador @ 2020-09-02  9:45 UTC (permalink / raw)
  To: akpm
  Cc: naoya.horiguchi, mhocko, tony.luck, cai, linux-kernel, linux-mm,
	Oscar Salvador

The important bit of this patchset is patch#1, which is a fix to take off
HWPoison pages off a buddy freelist since it can lead to us having HWPoison
pages back in the game and no one noticing it is handling a HWPoison page.
So fix it (we did that already for soft_offline_page [1]).

The other patches are clean-ups and not that important, so if anything,
consider patch#1 for inclusion.

Thanks


Oscar Salvador (4):
  mm,hwpoison: Take free pages off the buddy freelists
  mm,hwpoison: Refactor madvise_inject_error
  mm,hwpoison: Drain pcplists before bailing out for non-buddy
    zero-refcount page
  mm,hwpoison: Drop unneeded pcplist draining

 mm/madvise.c        | 34 +++++++++++++---------------------
 mm/memory-failure.c | 39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 25 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists
  2020-09-02  9:45 [PATCH 0/4] HWpoison: further fixes and cleanups Oscar Salvador
@ 2020-09-02  9:45 ` Oscar Salvador
  2020-09-04  2:02   ` HORIGUCHI NAOYA(堀口 直也)
  2020-09-02  9:45 ` [PATCH 2/4] mm,hwpoison: Refactor madvise_inject_error Oscar Salvador
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Oscar Salvador @ 2020-09-02  9:45 UTC (permalink / raw)
  To: akpm
  Cc: naoya.horiguchi, mhocko, tony.luck, cai, linux-kernel, linux-mm,
	Oscar Salvador

The crux of the matter is that historically we left poisoned pages
in the buddy system because we have some checks in place when
allocating a page that a gatekeeper for poisoned pages.
Unfortunately, we do have other users (e.g: compaction [1]) that scan
buddy freelists and try to get a page from there without checking
whether the page is HWPoison.

As I stated already, I think it is fundamentally wrong to keep
HWPoison pages within the buddy systems, checks in place or not.

Let us fix this we same way we did for soft_offline [2], and take
the page off the buddy freelist, so it is completely unreachable.

Note that this is fairly simple to trigger, as we only need
to poison free buddy pages (madvise MADV_HWPOISON) and then we need
to run some sort of memory stress system.

Just for a matter of reference, I put a dump_page in compaction_alloc
to trigger for HWPoison patches:

kernel: page:0000000012b2982b refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x1d5db
kernel: flags: 0xfffffc0800000(hwpoison)
kernel: raw: 000fffffc0800000 ffffea00007573c8 ffffc90000857de0 0000000000000000
kernel: raw: 0000000000000001 0000000000000000 00000001ffffffff 0000000000000000
kernel: page dumped because: compaction_alloc

kernel: CPU: 4 PID: 123 Comm: kcompactd0 Tainted: G            E     5.9.0-rc2-mm1-1-default+ #5
kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
kernel: Call Trace:
kernel:  dump_stack+0x6d/0x8b
kernel:  compaction_alloc+0xb2/0xc0
kernel:  migrate_pages+0x2a6/0x12a0
kernel:  ? isolate_freepages+0xc80/0xc80
kernel:  ? __ClearPageMovable+0xb0/0xb0
kernel:  compact_zone+0x5eb/0x11c0
kernel:  ? finish_task_switch+0x74/0x300
kernel:  ? lock_timer_base+0xa8/0x170
kernel:  proactive_compact_node+0x89/0xf0
kernel:  ? kcompactd+0x2d0/0x3a0
kernel:  kcompactd+0x2d0/0x3a0
kernel:  ? finish_wait+0x80/0x80
kernel:  ? kcompactd_do_work+0x350/0x350
kernel:  kthread+0x118/0x130
kernel:  ? kthread_associate_blkcg+0xa0/0xa0
kernel:  ret_from_fork+0x22/0x30

After that, if e.g: someone faults in the page, that someone will get killed
unexpectedly.

[1] https://lore.kernel.org/linux-mm/20190826104144.GA7849@linux/T/#u
[2] https://patchwork.kernel.org/patch/11694847/

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 mm/memory-failure.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 696505f56910..d349dcb45056 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1325,6 +1325,7 @@ int memory_failure(unsigned long pfn, int flags)
 	struct dev_pagemap *pgmap;
 	int res;
 	unsigned long page_flags;
+	bool retry = true;
 
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
@@ -1364,10 +1365,20 @@ int memory_failure(unsigned long pfn, int flags)
 	 * In fact it's dangerous to directly bump up page count from 0,
 	 * that may make page_ref_freeze()/page_ref_unfreeze() mismatch.
 	 */
+try_again:
 	if (!(flags & MF_COUNT_INCREASED) && !get_hwpoison_page(p)) {
 		if (is_free_buddy_page(p)) {
-			action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
-			return 0;
+			if (take_page_off_buddy(p)) {
+				action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
+				return 0;
+			} else {
+				/* We lost the race, try again */
+				if (retry) {
+					retry = false;
+					goto try_again;
+				}
+				return -EBUSY;
+			}
 		} else {
 			action_result(pfn, MF_MSG_KERNEL_HIGH_ORDER, MF_IGNORED);
 			return -EBUSY;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/4] mm,hwpoison: Refactor madvise_inject_error
  2020-09-02  9:45 [PATCH 0/4] HWpoison: further fixes and cleanups Oscar Salvador
  2020-09-02  9:45 ` [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists Oscar Salvador
@ 2020-09-02  9:45 ` Oscar Salvador
  2020-09-04  2:04   ` HORIGUCHI NAOYA(堀口 直也)
  2020-09-02  9:45 ` [PATCH 3/4] mm,hwpoison: Drain pcplists before bailing out for non-buddy zero-refcount page Oscar Salvador
  2020-09-02  9:45 ` [PATCH 4/4] mm,hwpoison: Drop unneeded pcplist draining Oscar Salvador
  3 siblings, 1 reply; 8+ messages in thread
From: Oscar Salvador @ 2020-09-02  9:45 UTC (permalink / raw)
  To: akpm
  Cc: naoya.horiguchi, mhocko, tony.luck, cai, linux-kernel, linux-mm,
	Oscar Salvador, Oscar Salvador

Make a proper if-else condition for {hard,soft}-offline.

Signed-off-by: Oscar Salvador <osalvador@suse.com>
---
 mm/madvise.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index e32e7efbba0f..e92e06890b08 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -885,7 +885,6 @@ static long madvise_remove(struct vm_area_struct *vma,
 static int madvise_inject_error(int behavior,
 		unsigned long start, unsigned long end)
 {
-	struct page *page;
 	struct zone *zone;
 	unsigned long size;
 
@@ -895,6 +894,7 @@ static int madvise_inject_error(int behavior,
 
 	for (; start < end; start += size) {
 		unsigned long pfn;
+		struct page *page;
 		int ret;
 
 		ret = get_user_pages_fast(start, 1, 0, &page);
@@ -911,25 +911,21 @@ static int madvise_inject_error(int behavior,
 
 		if (behavior == MADV_SOFT_OFFLINE) {
 			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
-					pfn, start);
-
+				 pfn, start);
 			ret = soft_offline_page(pfn, MF_COUNT_INCREASED);
-			if (ret)
-				return ret;
-			continue;
+		} else {
+			pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
+				 pfn, start);
+			/*
+			 * Drop the page reference taken by get_user_pages_fast(). In
+			 * the absence of MF_COUNT_INCREASED the memory_failure()
+			 * routine is responsible for pinning the page to prevent it
+			 * from being released back to the page allocator.
+			 */
+			put_page(page);
+			ret = memory_failure(pfn, 0);
 		}
 
-		pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
-				pfn, start);
-
-		/*
-		 * Drop the page reference taken by get_user_pages_fast(). In
-		 * the absence of MF_COUNT_INCREASED the memory_failure()
-		 * routine is responsible for pinning the page to prevent it
-		 * from being released back to the page allocator.
-		 */
-		put_page(page);
-		ret = memory_failure(pfn, 0);
 		if (ret)
 			return ret;
 	}
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/4] mm,hwpoison: Drain pcplists before bailing out for non-buddy zero-refcount page
  2020-09-02  9:45 [PATCH 0/4] HWpoison: further fixes and cleanups Oscar Salvador
  2020-09-02  9:45 ` [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists Oscar Salvador
  2020-09-02  9:45 ` [PATCH 2/4] mm,hwpoison: Refactor madvise_inject_error Oscar Salvador
@ 2020-09-02  9:45 ` Oscar Salvador
  2020-09-02  9:45 ` [PATCH 4/4] mm,hwpoison: Drop unneeded pcplist draining Oscar Salvador
  3 siblings, 0 replies; 8+ messages in thread
From: Oscar Salvador @ 2020-09-02  9:45 UTC (permalink / raw)
  To: akpm
  Cc: naoya.horiguchi, mhocko, tony.luck, cai, linux-kernel, linux-mm,
	Oscar Salvador

A page with 0-refcount and !PageBuddy could perfectly be a pcppage.
Currently, we bail out with an error if we encounter such a page,
meaning that we do not handle pcppages neither from hard-offline
nor from soft-offline path.

Fix this by draining pcplists whenever we find this kind of page
and retry the check again.
It might be that pcplists have been spilled into the buddy allocator
and so we can handle it.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 mm/memory-failure.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d349dcb45056..62e0b7f30cd9 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -950,13 +950,13 @@ static int page_action(struct page_state *ps, struct page *p,
 }
 
 /**
- * get_hwpoison_page() - Get refcount for memory error handling:
+ * __get_hwpoison_page() - Get refcount for memory error handling:
  * @page:	raw error page (hit by memory error)
  *
  * Return: return 0 if failed to grab the refcount, otherwise true (some
  * non-zero value.)
  */
-static int get_hwpoison_page(struct page *page)
+static int __get_hwpoison_page(struct page *page)
 {
 	struct page *head = compound_head(page);
 
@@ -986,6 +986,26 @@ static int get_hwpoison_page(struct page *page)
 	return 0;
 }
 
+static int get_hwpoison_page(struct page *p)
+{
+	int ret;
+	bool drained = false;
+
+retry:
+	ret = __get_hwpoison_page(p);
+	if (!ret && !is_free_buddy_page(p) && !page_count(p) && !drained) {
+		/*
+		 * The page might be in a pcplist, so try to drain those
+		 * and see if we are lucky.
+		 */
+		drain_all_pages(page_zone(p));
+		drained = true;
+		goto retry;
+	}
+
+	return ret;
+}
+
 /*
  * Do all that is necessary to remove user space mappings. Unmap
  * the pages and send SIGBUS to the processes if the data was dirty.
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/4] mm,hwpoison: Drop unneeded pcplist draining
  2020-09-02  9:45 [PATCH 0/4] HWpoison: further fixes and cleanups Oscar Salvador
                   ` (2 preceding siblings ...)
  2020-09-02  9:45 ` [PATCH 3/4] mm,hwpoison: Drain pcplists before bailing out for non-buddy zero-refcount page Oscar Salvador
@ 2020-09-02  9:45 ` Oscar Salvador
  3 siblings, 0 replies; 8+ messages in thread
From: Oscar Salvador @ 2020-09-02  9:45 UTC (permalink / raw)
  To: akpm
  Cc: naoya.horiguchi, mhocko, tony.luck, cai, linux-kernel, linux-mm,
	Oscar Salvador

memory_failure and soft_offline_path paths now drain pcplists by calling
get_hwpoison_page.
memory_failure flags the page as HWPoison before, so that page cannot longer
go into a pcplist, and soft_offline_page only flags a page as HWPoison
if 1) we took the page off a buddy freelist 2) the page was in-use
and we migrated it 3) was a clean pagecache.

Because of that, a page cannot longer be poisoned and be in a pcplist.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 mm/madvise.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index e92e06890b08..e7cdfabd8aca 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -930,10 +930,6 @@ static int madvise_inject_error(int behavior,
 			return ret;
 	}
 
-	/* Ensure that all poisoned pages are removed from per-cpu lists */
-	for_each_populated_zone(zone)
-		drain_all_pages(zone);
-
 	return 0;
 }
 #endif
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists
  2020-09-02  9:45 ` [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists Oscar Salvador
@ 2020-09-04  2:02   ` HORIGUCHI NAOYA(堀口 直也)
  2020-09-04 11:40     ` Oscar Salvador
  0 siblings, 1 reply; 8+ messages in thread
From: HORIGUCHI NAOYA(堀口 直也) @ 2020-09-04  2:02 UTC (permalink / raw)
  To: Oscar Salvador; +Cc: akpm, mhocko, tony.luck, cai, linux-kernel, linux-mm

On Wed, Sep 02, 2020 at 11:45:07AM +0200, Oscar Salvador wrote:
> The crux of the matter is that historically we left poisoned pages
> in the buddy system because we have some checks in place when
> allocating a page that a gatekeeper for poisoned pages.
> Unfortunately, we do have other users (e.g: compaction [1]) that scan
> buddy freelists and try to get a page from there without checking
> whether the page is HWPoison.
> 
> As I stated already, I think it is fundamentally wrong to keep
> HWPoison pages within the buddy systems, checks in place or not.
> 
> Let us fix this we same way we did for soft_offline [2], and take
> the page off the buddy freelist, so it is completely unreachable.
> 
> Note that this is fairly simple to trigger, as we only need
> to poison free buddy pages (madvise MADV_HWPOISON) and then we need
> to run some sort of memory stress system.
> 
> Just for a matter of reference, I put a dump_page in compaction_alloc
> to trigger for HWPoison patches:
> 
> kernel: page:0000000012b2982b refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x1d5db
> kernel: flags: 0xfffffc0800000(hwpoison)
> kernel: raw: 000fffffc0800000 ffffea00007573c8 ffffc90000857de0 0000000000000000
> kernel: raw: 0000000000000001 0000000000000000 00000001ffffffff 0000000000000000
> kernel: page dumped because: compaction_alloc
> 
> kernel: CPU: 4 PID: 123 Comm: kcompactd0 Tainted: G            E     5.9.0-rc2-mm1-1-default+ #5
> kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> kernel: Call Trace:
> kernel:  dump_stack+0x6d/0x8b
> kernel:  compaction_alloc+0xb2/0xc0
> kernel:  migrate_pages+0x2a6/0x12a0
> kernel:  ? isolate_freepages+0xc80/0xc80
> kernel:  ? __ClearPageMovable+0xb0/0xb0
> kernel:  compact_zone+0x5eb/0x11c0
> kernel:  ? finish_task_switch+0x74/0x300
> kernel:  ? lock_timer_base+0xa8/0x170
> kernel:  proactive_compact_node+0x89/0xf0
> kernel:  ? kcompactd+0x2d0/0x3a0
> kernel:  kcompactd+0x2d0/0x3a0
> kernel:  ? finish_wait+0x80/0x80
> kernel:  ? kcompactd_do_work+0x350/0x350
> kernel:  kthread+0x118/0x130
> kernel:  ? kthread_associate_blkcg+0xa0/0xa0
> kernel:  ret_from_fork+0x22/0x30

Thanks for spotting this.

> After that, if e.g: someone faults in the page, that someone will get killed
> unexpectedly.
> 
> [1] https://lore.kernel.org/linux-mm/20190826104144.GA7849@linux/T/#u
> [2] https://patchwork.kernel.org/patch/11694847/
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  mm/memory-failure.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 696505f56910..d349dcb45056 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1325,6 +1325,7 @@ int memory_failure(unsigned long pfn, int flags)
>  	struct dev_pagemap *pgmap;
>  	int res;
>  	unsigned long page_flags;
> +	bool retry = true;
>  
>  	if (!sysctl_memory_failure_recovery)
>  		panic("Memory failure on page %lx", pfn);
> @@ -1364,10 +1365,20 @@ int memory_failure(unsigned long pfn, int flags)
>  	 * In fact it's dangerous to directly bump up page count from 0,
>  	 * that may make page_ref_freeze()/page_ref_unfreeze() mismatch.
>  	 */
> +try_again:
>  	if (!(flags & MF_COUNT_INCREASED) && !get_hwpoison_page(p)) {
>  		if (is_free_buddy_page(p)) {
> -			action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
> -			return 0;
> +			if (take_page_off_buddy(p)) {
> +				action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
> +				return 0;
> +			} else {
> +				/* We lost the race, try again */
> +				if (retry) {
> +					retry = false;
> +					goto try_again;
> +				}

You might need add calling action_result() here.

> +				return -EBUSY;
> +			}
>  		} else {
>  			action_result(pfn, MF_MSG_KERNEL_HIGH_ORDER, MF_IGNORED);
>  			return -EBUSY;

And the following block also handles buddy pages, so you could also call
take_page_off_buddy() here?

        /*
         * We ignore non-LRU pages for good reasons.
         * - PG_locked is only well defined for LRU pages and a few others
         * - to avoid races with __SetPageLocked()
         * - to avoid races with __SetPageSlab*() (and more non-atomic ops)
         * The check (unnecessarily) ignores LRU pages being isolated and
         * walked by the page reclaim code, however that's not a big loss.
         */
        shake_page(p, 0);
        /* shake_page could have turned it free. */
        if (!PageLRU(p) && is_free_buddy_page(p)) {
                if (flags & MF_COUNT_INCREASED)
                        action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
                else
                        action_result(pfn, MF_MSG_BUDDY_2ND, MF_DELAYED);
                return 0;
        }

Or if get_hwpoison_page() handles retry inside it as will be done in patch 3/4,
we don't need this block any more?

Thanks,
Naoya Horiguchi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/4] mm,hwpoison: Refactor madvise_inject_error
  2020-09-02  9:45 ` [PATCH 2/4] mm,hwpoison: Refactor madvise_inject_error Oscar Salvador
@ 2020-09-04  2:04   ` HORIGUCHI NAOYA(堀口 直也)
  0 siblings, 0 replies; 8+ messages in thread
From: HORIGUCHI NAOYA(堀口 直也) @ 2020-09-04  2:04 UTC (permalink / raw)
  To: Oscar Salvador
  Cc: akpm, mhocko, tony.luck, cai, linux-kernel, linux-mm, Oscar Salvador

On Wed, Sep 02, 2020 at 11:45:08AM +0200, Oscar Salvador wrote:
> Make a proper if-else condition for {hard,soft}-offline.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists
  2020-09-04  2:02   ` HORIGUCHI NAOYA(堀口 直也)
@ 2020-09-04 11:40     ` Oscar Salvador
  0 siblings, 0 replies; 8+ messages in thread
From: Oscar Salvador @ 2020-09-04 11:40 UTC (permalink / raw)
  To: HORIGUCHI NAOYA(堀口 直也)
  Cc: akpm, mhocko, tony.luck, cai, linux-kernel, linux-mm

On Fri, Sep 04, 2020 at 02:02:05AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Wed, Sep 02, 2020 at 11:45:07AM +0200, Oscar Salvador wrote:
> > The crux of the matter is that historically we left poisoned pages
> > in the buddy system because we have some checks in place when
> > allocating a page that a gatekeeper for poisoned pages.
> > Unfortunately, we do have other users (e.g: compaction [1]) that scan
> > buddy freelists and try to get a page from there without checking
> > whether the page is HWPoison.
> > 
> > As I stated already, I think it is fundamentally wrong to keep
> > HWPoison pages within the buddy systems, checks in place or not.
> > 
> > Let us fix this we same way we did for soft_offline [2], and take
> > the page off the buddy freelist, so it is completely unreachable.
> > 
> > Note that this is fairly simple to trigger, as we only need
> > to poison free buddy pages (madvise MADV_HWPOISON) and then we need
> > to run some sort of memory stress system.
> > 
> > Just for a matter of reference, I put a dump_page in compaction_alloc
> > to trigger for HWPoison patches:
> > 
> > kernel: page:0000000012b2982b refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x1d5db
> > kernel: flags: 0xfffffc0800000(hwpoison)
> > kernel: raw: 000fffffc0800000 ffffea00007573c8 ffffc90000857de0 0000000000000000
> > kernel: raw: 0000000000000001 0000000000000000 00000001ffffffff 0000000000000000
> > kernel: page dumped because: compaction_alloc
> > 
> > kernel: CPU: 4 PID: 123 Comm: kcompactd0 Tainted: G            E     5.9.0-rc2-mm1-1-default+ #5
> > kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> > kernel: Call Trace:
> > kernel:  dump_stack+0x6d/0x8b
> > kernel:  compaction_alloc+0xb2/0xc0
> > kernel:  migrate_pages+0x2a6/0x12a0
> > kernel:  ? isolate_freepages+0xc80/0xc80
> > kernel:  ? __ClearPageMovable+0xb0/0xb0
> > kernel:  compact_zone+0x5eb/0x11c0
> > kernel:  ? finish_task_switch+0x74/0x300
> > kernel:  ? lock_timer_base+0xa8/0x170
> > kernel:  proactive_compact_node+0x89/0xf0
> > kernel:  ? kcompactd+0x2d0/0x3a0
> > kernel:  kcompactd+0x2d0/0x3a0
> > kernel:  ? finish_wait+0x80/0x80
> > kernel:  ? kcompactd_do_work+0x350/0x350
> > kernel:  kthread+0x118/0x130
> > kernel:  ? kthread_associate_blkcg+0xa0/0xa0
> > kernel:  ret_from_fork+0x22/0x30
> 
> Thanks for spotting this.
> 
> > After that, if e.g: someone faults in the page, that someone will get killed
> > unexpectedly.
> > 
> > [1] https://lore.kernel.org/linux-mm/20190826104144.GA7849@linux/T/#u
> > [2] https://patchwork.kernel.org/patch/11694847/
> > 
> > Signed-off-by: Oscar Salvador <osalvador@suse.de>
> > ---
> >  mm/memory-failure.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 696505f56910..d349dcb45056 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -1325,6 +1325,7 @@ int memory_failure(unsigned long pfn, int flags)
> >  	struct dev_pagemap *pgmap;
> >  	int res;
> >  	unsigned long page_flags;
> > +	bool retry = true;
> >  
> >  	if (!sysctl_memory_failure_recovery)
> >  		panic("Memory failure on page %lx", pfn);
> > @@ -1364,10 +1365,20 @@ int memory_failure(unsigned long pfn, int flags)
> >  	 * In fact it's dangerous to directly bump up page count from 0,
> >  	 * that may make page_ref_freeze()/page_ref_unfreeze() mismatch.
> >  	 */
> > +try_again:
> >  	if (!(flags & MF_COUNT_INCREASED) && !get_hwpoison_page(p)) {
> >  		if (is_free_buddy_page(p)) {
> > -			action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
> > -			return 0;
> > +			if (take_page_off_buddy(p)) {
> > +				action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
> > +				return 0;
> > +			} else {
> > +				/* We lost the race, try again */
> > +				if (retry) {
> > +					retry = false;
> > +					goto try_again;
> > +				}
> 
> You might need add calling action_result() here.

Yeah, I figured we would need something like:

action_result(pfn, MF_MSG_BUDDY_2ND, MF_FAILED) ?
Although checking other parts of the code, MF_IGNORED can suit here as well,
and it is probably better.

> And the following block also handles buddy pages, so you could also call
> take_page_off_buddy() here?
> 
>         /*
>          * We ignore non-LRU pages for good reasons.
>          * - PG_locked is only well defined for LRU pages and a few others
>          * - to avoid races with __SetPageLocked()
>          * - to avoid races with __SetPageSlab*() (and more non-atomic ops)
>          * The check (unnecessarily) ignores LRU pages being isolated and
>          * walked by the page reclaim code, however that's not a big loss.
>          */
>         shake_page(p, 0);
>         /* shake_page could have turned it free. */
>         if (!PageLRU(p) && is_free_buddy_page(p)) {
>                 if (flags & MF_COUNT_INCREASED)
>                         action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
>                 else
>                         action_result(pfn, MF_MSG_BUDDY_2ND, MF_DELAYED);
>                 return 0;
>         }
> 
> Or if get_hwpoison_page() handles retry inside it as will be done in patch 3/4,
> we don't need this block any more?

I __think__ we do not need the following anymore:

         if (!PageLRU(p) && is_free_buddy_page(p)) {
                 if (flags & MF_COUNT_INCREASED)
                         action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
                 else
                         action_result(pfn, MF_MSG_BUDDY_2ND, MF_DELAYED);
                 return 0;
         }

We have this block because shake_page() (among other things) sends pcp-pages
back to the buddy freelists, so we could have the chance to handle the page.
Since we will be doing that in get_hwpoison_page, and such scenario should
have been handled in the block from above, I think it is safe to skip that
block.

But I think that the skape_page() has to stay as it flushes pages from pagevecs
to their respective LRU lists, otherwise we could do the wrong thing in:

	if (!PageTransTail(p) && !PageLRU(p))
		goto identify_page_state;

So, I will send V2 with a 5th patch cleaning that up as I do not want to get
it convulated with the fix itself.

thanks Naoya

-- 
Oscar Salvador
SUSE L3

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-09-04 11:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-02  9:45 [PATCH 0/4] HWpoison: further fixes and cleanups Oscar Salvador
2020-09-02  9:45 ` [PATCH 1/4] mm,hwpoison: Take free pages off the buddy freelists Oscar Salvador
2020-09-04  2:02   ` HORIGUCHI NAOYA(堀口 直也)
2020-09-04 11:40     ` Oscar Salvador
2020-09-02  9:45 ` [PATCH 2/4] mm,hwpoison: Refactor madvise_inject_error Oscar Salvador
2020-09-04  2:04   ` HORIGUCHI NAOYA(堀口 直也)
2020-09-02  9:45 ` [PATCH 3/4] mm,hwpoison: Drain pcplists before bailing out for non-buddy zero-refcount page Oscar Salvador
2020-09-02  9:45 ` [PATCH 4/4] mm,hwpoison: Drop unneeded pcplist draining Oscar Salvador

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).