On Tue, Dec 11, 2012 at 08:18:27PM +0800, Xishi Qiu wrote:
>1) move poisoned page check at the beginning of the function.
>2) add page_lock to avoid unpoison clear the flag.
>
>Signed-off-by: Xishi Qiu <qiuxishi@xxxxxxxxxx>
>Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxx>
>---
> mm/memory-failure.c | 43 ++++++++++++++++++++++---------------------
> 1 files changed, 22 insertions(+), 21 deletions(-)
>
>diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>index 8b20278..9b74983 100644
>--- a/mm/memory-failure.c
>+++ b/mm/memory-failure.c
>@@ -1419,18 +1419,17 @@ static int soft_offline_huge_page(struct page *page, int flags)
> unsigned long pfn = page_to_pfn(page);
> struct page *hpage = compound_head(page);
>
>+ if (PageHWPoison(hpage)) {
>+ pr_info("soft offline: %#lx hugepage already poisoned\n", pfn);
>+ return -EBUSY;
>+ }
>+
> ret = get_any_page(page, pfn, flags);
> if (ret < 0)
> return ret;
> if (ret == 0)
> goto done;
>
>- if (PageHWPoison(hpage)) {
>- put_page(hpage);
>- pr_info("soft offline: %#lx hugepage already poisoned\n", pfn);
>- return -EBUSY;
>- }
>-
> /* Keep page count to indicate a given hugepage is isolated. */
> ret = migrate_huge_page(hpage, new_page, MPOL_MF_MOVE_ALL, false,
> MIGRATE_SYNC);
>@@ -1441,12 +1440,14 @@ static int soft_offline_huge_page(struct page *page, int flags)
> return ret;
> }
> done:
>- if (!PageHWPoison(hpage))
>- atomic_long_add(1 << compound_trans_order(hpage),
>- &mce_bad_pages);
>+ /* keep elevated page count for bad page */
>+ lock_page(hpage);
>+ atomic_long_add(1 << compound_trans_order(hpage), &mce_bad_pages);
> set_page_hwpoison_huge_page(hpage);
>+ unlock_page(hpage);
>+
> dequeue_hwpoisoned_huge_page(hpage);
>- /* keep elevated page count for bad page */
>+
> return ret;
> }
>
>@@ -1488,6 +1489,11 @@ int soft_offline_page(struct page *page, int flags)
> }
> }
>
>+ if (PageHWPoison(page)) {
>+ pr_info("soft offline: %#lx page already poisoned\n", pfn);
>+ return -EBUSY;
>+ }
>+
> ret = get_any_page(page, pfn, flags);
> if (ret < 0)
> return ret;
>@@ -1519,19 +1525,11 @@ int soft_offline_page(struct page *page, int flags)
> return -EIO;
> }
>
>- lock_page(page);
>- wait_on_page_writeback(page);
>-
> /*
> * Synchronized using the page lock with memory_failure()
> */
>- if (PageHWPoison(page)) {
>- unlock_page(page);
>- put_page(page);
>- pr_info("soft offline: %#lx page already poisoned\n", pfn);
>- return -EBUSY;
>- }
>-
>+ lock_page(page);
>+ wait_on_page_writeback(page);
> /*
> * Try to invalidate first. This should work for
> * non dirty unmapped page cache pages.
>@@ -1582,8 +1580,11 @@ int soft_offline_page(struct page *page, int flags)
> return ret;
>
> done:
>+ /* keep elevated page count for bad page */
>+ lock_page(page);
> atomic_long_add(1, &mce_bad_pages);
> SetPageHWPoison(page);
>- /* keep elevated page count for bad page */
>+ unlock_page(page);
>+
Hi Xishi,
Why add lock_page here, the comment in function unpoison_memory tell us
we don't need it.
/*
* 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.
*/
Futhermore, Andrew didn't like a variable called "mce_bad_pages".
- Why do we have a variable called "mce_bad_pages"? MCE is an x86
concept, and this code is in mm/. Lights are flashing, bells are
ringing and a loudspeaker is blaring "layering violation" at us!
Regards,
Wanpeng Li
> return ret;
> }
>--
>1.7.1
>
>
>--
>To unsubscribe, send a message with 'unsubscribe linux-mm' in
>the body to majordomo@xxxxxxxxx. For more info on Linux MM,
>see: http://www.linux-mm.org/ .
>Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>