Hi,
I updated the unpoison patchset based ou discussions over v2.
Please see individual patches for details of updates.
----- (cover letter copied from v2) -----
Main purpose of this series is to sync unpoison code to recent changes
around how hwpoison code takes page refcount. Unpoison should work or
simply fail (without crash) if impossible.
The recent works of keeping hwpoison pages in shmem pagecache introduce
a new state of hwpoisoned pages, but unpoison for such pages is not
supported yet with this series.
It seems that soft-offline and unpoison can be used as general purpose
page offline/online mechanism (not in the context of memory error). I
think that we need some additional works to realize it because currently
soft-offline and unpoison are assumed not to happen so frequently
(print out too many messages for aggressive usecases). But anyway this
could be another interesting next topic.
v1: https://lore.kernel.org/linux-mm/[email protected]/
v2: https://lore.kernel.org/linux-mm/[email protected]/
Thanks,
Naoya Horiguchi
---
Summary:
Naoya Horiguchi (3):
mm/hwpoison: mf_mutex for soft offline and unpoison
mm/hwpoison: remove MF_MSG_BUDDY_2ND and MF_MSG_POISONED_HUGE
mm/hwpoison: fix unpoison_memory()
include/linux/mm.h | 3 +-
include/linux/page-flags.h | 4 ++
include/ras/ras_event.h | 2 -
mm/memory-failure.c | 169 ++++++++++++++++++++++++++++-----------------
mm/page_alloc.c | 23 ++++++
5 files changed, 133 insertions(+), 68 deletions(-)
From: Naoya Horiguchi <[email protected]>
Originally mf_mutex is introduced to serialize multiple MCE events, but
it is not that useful to allow unpoison to run in parallel with memory_failure()
and soft offline. So apply mf_they to soft offline and unpoison.
The memory failure handler and soft offline handler get simpler with this.
Signed-off-by: Naoya Horiguchi <[email protected]>
---
ChangeLog v3:
- merge with "mm/hwpoison: remove race consideration"
- update description
ChangeLog v2:
- add mutex_unlock() in "page already poisoned" path in soft_offline_page().
(Thanks to Ding Hui)
---
mm/memory-failure.c | 62 +++++++++++++--------------------------------
1 file changed, 18 insertions(+), 44 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index e8c38e27b753..d29c79de6034 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1507,14 +1507,6 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
lock_page(head);
page_flags = head->flags;
- if (!PageHWPoison(head)) {
- pr_err("Memory failure: %#lx: just unpoisoned\n", pfn);
- num_poisoned_pages_dec();
- unlock_page(head);
- put_page(head);
- return 0;
- }
-
/*
* TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
* simply disable it. In order to make it work properly, we need
@@ -1628,6 +1620,8 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
return rc;
}
+static DEFINE_MUTEX(mf_mutex);
+
/**
* memory_failure - Handle memory failure of a page.
* @pfn: Page Number of the corrupted page
@@ -1654,7 +1648,6 @@ int memory_failure(unsigned long pfn, int flags)
int res = 0;
unsigned long page_flags;
bool retry = true;
- static DEFINE_MUTEX(mf_mutex);
if (!sysctl_memory_failure_recovery)
panic("Memory failure on page %lx", pfn);
@@ -1788,16 +1781,6 @@ int memory_failure(unsigned long pfn, int flags)
*/
page_flags = p->flags;
- /*
- * unpoison always clear PG_hwpoison inside page lock
- */
- if (!PageHWPoison(p)) {
- pr_err("Memory failure: %#lx: just unpoisoned\n", pfn);
- num_poisoned_pages_dec();
- unlock_page(p);
- put_page(p);
- goto unlock_mutex;
- }
if (hwpoison_filter(p)) {
if (TestClearPageHWPoison(p))
num_poisoned_pages_dec();
@@ -1978,6 +1961,7 @@ int unpoison_memory(unsigned long pfn)
struct page *page;
struct page *p;
int freeit = 0;
+ int ret = 0;
unsigned long flags = 0;
static DEFINE_RATELIMIT_STATE(unpoison_rs, DEFAULT_RATELIMIT_INTERVAL,
DEFAULT_RATELIMIT_BURST);
@@ -1988,39 +1972,30 @@ int unpoison_memory(unsigned long pfn)
p = pfn_to_page(pfn);
page = compound_head(p);
+ mutex_lock(&mf_mutex);
+
if (!PageHWPoison(p)) {
unpoison_pr_info("Unpoison: Page was already unpoisoned %#lx\n",
pfn, &unpoison_rs);
- return 0;
+ goto unlock_mutex;
}
if (page_count(page) > 1) {
unpoison_pr_info("Unpoison: Someone grabs the hwpoison page %#lx\n",
pfn, &unpoison_rs);
- return 0;
+ goto unlock_mutex;
}
if (page_mapped(page)) {
unpoison_pr_info("Unpoison: Someone maps the hwpoison page %#lx\n",
pfn, &unpoison_rs);
- return 0;
+ goto unlock_mutex;
}
if (page_mapping(page)) {
unpoison_pr_info("Unpoison: the hwpoison page has non-NULL mapping %#lx\n",
pfn, &unpoison_rs);
- return 0;
- }
-
- /*
- * unpoison_memory() can encounter thp only when the thp is being
- * worked by memory_failure() and the page lock is not held yet.
- * In such case, we yield to memory_failure() and make unpoison fail.
- */
- if (!PageHuge(page) && PageTransHuge(page)) {
- unpoison_pr_info("Unpoison: Memory failure is now running on %#lx\n",
- pfn, &unpoison_rs);
- return 0;
+ goto unlock_mutex;
}
if (!get_hwpoison_page(p, flags)) {
@@ -2028,29 +2003,23 @@ int unpoison_memory(unsigned long pfn)
num_poisoned_pages_dec();
unpoison_pr_info("Unpoison: Software-unpoisoned free page %#lx\n",
pfn, &unpoison_rs);
- return 0;
+ goto unlock_mutex;
}
- lock_page(page);
- /*
- * This test is racy because PG_hwpoison is set outside of page lock.
- * That's acceptable because that won't trigger kernel panic. Instead,
- * the PG_hwpoison page will be caught and isolated on the entrance to
- * the free buddy page pool.
- */
if (TestClearPageHWPoison(page)) {
unpoison_pr_info("Unpoison: Software-unpoisoned page %#lx\n",
pfn, &unpoison_rs);
num_poisoned_pages_dec();
freeit = 1;
}
- unlock_page(page);
put_page(page);
if (freeit && !(pfn == my_zero_pfn(0) && page_count(p) == 1))
put_page(page);
- return 0;
+unlock_mutex:
+ mutex_unlock(&mf_mutex);
+ return ret;
}
EXPORT_SYMBOL(unpoison_memory);
@@ -2231,9 +2200,12 @@ int soft_offline_page(unsigned long pfn, int flags)
return -EIO;
}
+ mutex_lock(&mf_mutex);
+
if (PageHWPoison(page)) {
pr_info("%s: %#lx page already poisoned\n", __func__, pfn);
put_ref_page(ref_page);
+ mutex_unlock(&mf_mutex);
return 0;
}
@@ -2251,5 +2223,7 @@ int soft_offline_page(unsigned long pfn, int flags)
}
}
+ mutex_unlock(&mf_mutex);
+
return ret;
}
--
2.25.1
From: Naoya Horiguchi <[email protected]>
These action_page_types are no longer used, so remove them.
Signed-off-by: Naoya Horiguchi <[email protected]>
Acked-by: Yang Shi <[email protected]>
---
include/linux/mm.h | 2 --
include/ras/ras_event.h | 2 --
mm/memory-failure.c | 2 --
3 files changed, 6 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a7e4a9e7d807..7941bca871dc 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3247,7 +3247,6 @@ enum mf_action_page_type {
MF_MSG_KERNEL_HIGH_ORDER,
MF_MSG_SLAB,
MF_MSG_DIFFERENT_COMPOUND,
- MF_MSG_POISONED_HUGE,
MF_MSG_HUGE,
MF_MSG_FREE_HUGE,
MF_MSG_NON_PMD_HUGE,
@@ -3262,7 +3261,6 @@ enum mf_action_page_type {
MF_MSG_CLEAN_LRU,
MF_MSG_TRUNCATED_LRU,
MF_MSG_BUDDY,
- MF_MSG_BUDDY_2ND,
MF_MSG_DAX,
MF_MSG_UNSPLIT_THP,
MF_MSG_UNKNOWN,
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 0bdbc0d17d2f..d0337a41141c 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -358,7 +358,6 @@ TRACE_EVENT(aer_event,
EM ( MF_MSG_KERNEL_HIGH_ORDER, "high-order kernel page" ) \
EM ( MF_MSG_SLAB, "kernel slab page" ) \
EM ( MF_MSG_DIFFERENT_COMPOUND, "different compound page after locking" ) \
- EM ( MF_MSG_POISONED_HUGE, "huge page already hardware poisoned" ) \
EM ( MF_MSG_HUGE, "huge page" ) \
EM ( MF_MSG_FREE_HUGE, "free huge page" ) \
EM ( MF_MSG_NON_PMD_HUGE, "non-pmd-sized huge page" ) \
@@ -373,7 +372,6 @@ TRACE_EVENT(aer_event,
EM ( MF_MSG_CLEAN_LRU, "clean LRU page" ) \
EM ( MF_MSG_TRUNCATED_LRU, "already truncated LRU page" ) \
EM ( MF_MSG_BUDDY, "free buddy page" ) \
- EM ( MF_MSG_BUDDY_2ND, "free buddy page (2nd try)" ) \
EM ( MF_MSG_DAX, "dax page" ) \
EM ( MF_MSG_UNSPLIT_THP, "unsplit thp" ) \
EMe ( MF_MSG_UNKNOWN, "unknown page" )
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d29c79de6034..722036539b44 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -723,7 +723,6 @@ static const char * const action_page_types[] = {
[MF_MSG_KERNEL_HIGH_ORDER] = "high-order kernel page",
[MF_MSG_SLAB] = "kernel slab page",
[MF_MSG_DIFFERENT_COMPOUND] = "different compound page after locking",
- [MF_MSG_POISONED_HUGE] = "huge page already hardware poisoned",
[MF_MSG_HUGE] = "huge page",
[MF_MSG_FREE_HUGE] = "free huge page",
[MF_MSG_NON_PMD_HUGE] = "non-pmd-sized huge page",
@@ -738,7 +737,6 @@ static const char * const action_page_types[] = {
[MF_MSG_CLEAN_LRU] = "clean LRU page",
[MF_MSG_TRUNCATED_LRU] = "already truncated LRU page",
[MF_MSG_BUDDY] = "free buddy page",
- [MF_MSG_BUDDY_2ND] = "free buddy page (2nd try)",
[MF_MSG_DAX] = "dax page",
[MF_MSG_UNSPLIT_THP] = "unsplit thp",
[MF_MSG_UNKNOWN] = "unknown page",
--
2.25.1
On 05.11.21 06:50, Naoya Horiguchi wrote:
> Hi,
>
> I updated the unpoison patchset based ou discussions over v2.
> Please see individual patches for details of updates.
>
> ----- (cover letter copied from v2) -----
> Main purpose of this series is to sync unpoison code to recent changes
> around how hwpoison code takes page refcount. Unpoison should work or
> simply fail (without crash) if impossible.
>
> The recent works of keeping hwpoison pages in shmem pagecache introduce
> a new state of hwpoisoned pages, but unpoison for such pages is not
> supported yet with this series.
>
> It seems that soft-offline and unpoison can be used as general purpose
> page offline/online mechanism (not in the context of memory error).
I'm not sure what the target use case would be TBH ... for proper memory
offlining/memory hotunplug we have to offline whole memory blocks. For
memory ballooning based mechanisms we simply allocate random free pages
and eventually trigger reclaim to make more random free pages available.
For memory hotunplug via virtio-mem we're using alloc_contig_range() to
allocate ranges of interest we logically unplug.
The only benefit compared to alloc_contig_range() might be that we can
offline smaller chunks -- alloc_contig_range() isn't optimized for
sub-MAX_ORDER granularity yet. But then, alloc_contig_range() should
much rather be extended.
Long story short, I'm not sure there is a sane use case for this
"general purpose page offline/online mechanism" ...
--
Thanks,
David / dhildenb
On Fri, Nov 05, 2021 at 11:58:15AM +0100, David Hildenbrand wrote:
> On 05.11.21 06:50, Naoya Horiguchi wrote:
> > Hi,
> >
> > I updated the unpoison patchset based ou discussions over v2.
> > Please see individual patches for details of updates.
> >
> > ----- (cover letter copied from v2) -----
> > Main purpose of this series is to sync unpoison code to recent changes
> > around how hwpoison code takes page refcount. Unpoison should work or
> > simply fail (without crash) if impossible.
> >
> > The recent works of keeping hwpoison pages in shmem pagecache introduce
> > a new state of hwpoisoned pages, but unpoison for such pages is not
> > supported yet with this series.
> >
> > It seems that soft-offline and unpoison can be used as general purpose
> > page offline/online mechanism (not in the context of memory error).
>
> I'm not sure what the target use case would be TBH ... for proper memory
> offlining/memory hotunplug we have to offline whole memory blocks. For
> memory ballooning based mechanisms we simply allocate random free pages
> and eventually trigger reclaim to make more random free pages available.
> For memory hotunplug via virtio-mem we're using alloc_contig_range() to
> allocate ranges of interest we logically unplug.
I heard about it from two people independently and I think that that's maybe
a rough idea, so if no one shows the clear use case or someone logically
shows that we don't need it, I do not head for it.
>
> The only benefit compared to alloc_contig_range() might be that we can
> offline smaller chunks -- alloc_contig_range() isn't optimized for
> sub-MAX_ORDER granularity yet. But then, alloc_contig_range() should
> much rather be extended.
If alloc_contig_range() supports memory offline in arbitrary size of
granurality (including a single page), maybe soft offline can be (partially
I guess) unified to it.
Thanks,
Naoya Horiguchi
>
> Long story short, I'm not sure there is a sane use case for this
> "general purpose page offline/online mechanism" ...
On 05.11.21 12:49, Naoya Horiguchi wrote:
> On Fri, Nov 05, 2021 at 11:58:15AM +0100, David Hildenbrand wrote:
>> On 05.11.21 06:50, Naoya Horiguchi wrote:
>>> Hi,
>>>
>>> I updated the unpoison patchset based ou discussions over v2.
>>> Please see individual patches for details of updates.
>>>
>>> ----- (cover letter copied from v2) -----
>>> Main purpose of this series is to sync unpoison code to recent changes
>>> around how hwpoison code takes page refcount. Unpoison should work or
>>> simply fail (without crash) if impossible.
>>>
>>> The recent works of keeping hwpoison pages in shmem pagecache introduce
>>> a new state of hwpoisoned pages, but unpoison for such pages is not
>>> supported yet with this series.
>>>
>>> It seems that soft-offline and unpoison can be used as general purpose
>>> page offline/online mechanism (not in the context of memory error).
>>
>> I'm not sure what the target use case would be TBH ... for proper memory
>> offlining/memory hotunplug we have to offline whole memory blocks. For
>> memory ballooning based mechanisms we simply allocate random free pages
>> and eventually trigger reclaim to make more random free pages available.
>> For memory hotunplug via virtio-mem we're using alloc_contig_range() to
>> allocate ranges of interest we logically unplug.
>
> I heard about it from two people independently and I think that that's maybe
> a rough idea, so if no one shows the clear use case or someone logically
> shows that we don't need it, I do not head for it.
I'd love to learn about use cases!
>
>>
>> The only benefit compared to alloc_contig_range() might be that we can
>> offline smaller chunks -- alloc_contig_range() isn't optimized for
>> sub-MAX_ORDER granularity yet. But then, alloc_contig_range() should
>> much rather be extended.
>
> If alloc_contig_range() supports memory offline in arbitrary size of
> granurality (including a single page), maybe soft offline can be (partially
> I guess) unified to it.
Conceptually, memory offlining, alloc_contig_range(), soft-offlining all
perform roughly the same thing just with different flavors: evacuate a
given PFN area and decide what shall happen with it.
After memory offlining, memory cannot get reused (e.g., allocated via
the buddy) before re-onlining that memory. It's some kind of "fake
allocation" + advanced magic to actually remove the memory from the system.
alloc_contig_range() is just a "real" allocation, and can be used (e.g.,
by virtio-mem) for fake offlining by some additional magic on top.
soft-offlining is just another special type of "special allocation",
however, focused on individual page.
The biggest difference between soft-offlining and
alloc_contig_range()+memory offlining is right now the granularity.
While alloc_contig_range() can be used to allocate individual pages on
ZONE_MOVABLE and MIGRATE_CMA, it cannot deal with other zones with such
small granularity yet -- too many false negatives, meaning an allocation
might fail although the single page actually could get allocated.
--
Thanks,
David / dhildenb
On Thu, Nov 4, 2021 at 10:52 PM Naoya Horiguchi
<[email protected]> wrote:
>
> From: Naoya Horiguchi <[email protected]>
>
> Originally mf_mutex is introduced to serialize multiple MCE events, but
> it is not that useful to allow unpoison to run in parallel with memory_failure()
> and soft offline. So apply mf_they to soft offline and unpoison.
^^^^^^^^
A typo? It should be mf_mutex, right?
Looks good to me other than the above nit. Reviewed-by: Yang Shi
<[email protected]>
> The memory failure handler and soft offline handler get simpler with this.
>
> Signed-off-by: Naoya Horiguchi <[email protected]>
> ---
> ChangeLog v3:
> - merge with "mm/hwpoison: remove race consideration"
> - update description
>
> ChangeLog v2:
> - add mutex_unlock() in "page already poisoned" path in soft_offline_page().
> (Thanks to Ding Hui)
> ---
> mm/memory-failure.c | 62 +++++++++++++--------------------------------
> 1 file changed, 18 insertions(+), 44 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index e8c38e27b753..d29c79de6034 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1507,14 +1507,6 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
> lock_page(head);
> page_flags = head->flags;
>
> - if (!PageHWPoison(head)) {
> - pr_err("Memory failure: %#lx: just unpoisoned\n", pfn);
> - num_poisoned_pages_dec();
> - unlock_page(head);
> - put_page(head);
> - return 0;
> - }
> -
> /*
> * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
> * simply disable it. In order to make it work properly, we need
> @@ -1628,6 +1620,8 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
> return rc;
> }
>
> +static DEFINE_MUTEX(mf_mutex);
> +
> /**
> * memory_failure - Handle memory failure of a page.
> * @pfn: Page Number of the corrupted page
> @@ -1654,7 +1648,6 @@ int memory_failure(unsigned long pfn, int flags)
> int res = 0;
> unsigned long page_flags;
> bool retry = true;
> - static DEFINE_MUTEX(mf_mutex);
>
> if (!sysctl_memory_failure_recovery)
> panic("Memory failure on page %lx", pfn);
> @@ -1788,16 +1781,6 @@ int memory_failure(unsigned long pfn, int flags)
> */
> page_flags = p->flags;
>
> - /*
> - * unpoison always clear PG_hwpoison inside page lock
> - */
> - if (!PageHWPoison(p)) {
> - pr_err("Memory failure: %#lx: just unpoisoned\n", pfn);
> - num_poisoned_pages_dec();
> - unlock_page(p);
> - put_page(p);
> - goto unlock_mutex;
> - }
> if (hwpoison_filter(p)) {
> if (TestClearPageHWPoison(p))
> num_poisoned_pages_dec();
> @@ -1978,6 +1961,7 @@ int unpoison_memory(unsigned long pfn)
> struct page *page;
> struct page *p;
> int freeit = 0;
> + int ret = 0;
> unsigned long flags = 0;
> static DEFINE_RATELIMIT_STATE(unpoison_rs, DEFAULT_RATELIMIT_INTERVAL,
> DEFAULT_RATELIMIT_BURST);
> @@ -1988,39 +1972,30 @@ int unpoison_memory(unsigned long pfn)
> p = pfn_to_page(pfn);
> page = compound_head(p);
>
> + mutex_lock(&mf_mutex);
> +
> if (!PageHWPoison(p)) {
> unpoison_pr_info("Unpoison: Page was already unpoisoned %#lx\n",
> pfn, &unpoison_rs);
> - return 0;
> + goto unlock_mutex;
> }
>
> if (page_count(page) > 1) {
> unpoison_pr_info("Unpoison: Someone grabs the hwpoison page %#lx\n",
> pfn, &unpoison_rs);
> - return 0;
> + goto unlock_mutex;
> }
>
> if (page_mapped(page)) {
> unpoison_pr_info("Unpoison: Someone maps the hwpoison page %#lx\n",
> pfn, &unpoison_rs);
> - return 0;
> + goto unlock_mutex;
> }
>
> if (page_mapping(page)) {
> unpoison_pr_info("Unpoison: the hwpoison page has non-NULL mapping %#lx\n",
> pfn, &unpoison_rs);
> - return 0;
> - }
> -
> - /*
> - * unpoison_memory() can encounter thp only when the thp is being
> - * worked by memory_failure() and the page lock is not held yet.
> - * In such case, we yield to memory_failure() and make unpoison fail.
> - */
> - if (!PageHuge(page) && PageTransHuge(page)) {
> - unpoison_pr_info("Unpoison: Memory failure is now running on %#lx\n",
> - pfn, &unpoison_rs);
> - return 0;
> + goto unlock_mutex;
> }
>
> if (!get_hwpoison_page(p, flags)) {
> @@ -2028,29 +2003,23 @@ int unpoison_memory(unsigned long pfn)
> num_poisoned_pages_dec();
> unpoison_pr_info("Unpoison: Software-unpoisoned free page %#lx\n",
> pfn, &unpoison_rs);
> - return 0;
> + goto unlock_mutex;
> }
>
> - lock_page(page);
> - /*
> - * This test is racy because PG_hwpoison is set outside of page lock.
> - * That's acceptable because that won't trigger kernel panic. Instead,
> - * the PG_hwpoison page will be caught and isolated on the entrance to
> - * the free buddy page pool.
> - */
> if (TestClearPageHWPoison(page)) {
> unpoison_pr_info("Unpoison: Software-unpoisoned page %#lx\n",
> pfn, &unpoison_rs);
> num_poisoned_pages_dec();
> freeit = 1;
> }
> - unlock_page(page);
>
> put_page(page);
> if (freeit && !(pfn == my_zero_pfn(0) && page_count(p) == 1))
> put_page(page);
>
> - return 0;
> +unlock_mutex:
> + mutex_unlock(&mf_mutex);
> + return ret;
> }
> EXPORT_SYMBOL(unpoison_memory);
>
> @@ -2231,9 +2200,12 @@ int soft_offline_page(unsigned long pfn, int flags)
> return -EIO;
> }
>
> + mutex_lock(&mf_mutex);
> +
> if (PageHWPoison(page)) {
> pr_info("%s: %#lx page already poisoned\n", __func__, pfn);
> put_ref_page(ref_page);
> + mutex_unlock(&mf_mutex);
> return 0;
> }
>
> @@ -2251,5 +2223,7 @@ int soft_offline_page(unsigned long pfn, int flags)
> }
> }
>
> + mutex_unlock(&mf_mutex);
> +
> return ret;
> }
> --
> 2.25.1
>
>