| Message ID | 20211105055058.3152564-2-naoya.horiguchi@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/hwpoison: fix unpoison_memory() | expand |
On Thu, Nov 4, 2021 at 10:52 PM Naoya Horiguchi <naoya.horiguchi@linux.dev> wrote: > > From: Naoya Horiguchi <naoya.horiguchi@nec.com> > > 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 <shy828301@gmail.com> > The memory failure handler and soft offline handler get simpler with this. > > Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > --- > 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 > >
On Fri, Nov 05, 2021 at 11:23:47AM -0700, Yang Shi wrote: > On Thu, Nov 4, 2021 at 10:52 PM Naoya Horiguchi > <naoya.horiguchi@linux.dev> wrote: > > > > From: Naoya Horiguchi <naoya.horiguchi@nec.com> > > > > 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? Right, I'll fix it. > > Looks good to me other than the above nit. Reviewed-by: Yang Shi > <shy828301@gmail.com> Thank you. - Naoya
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; }