On 2012/12/11 20:42, Wanpeng Li wrote:
> 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 <qiuxi...@huawei.com>
>> Signed-off-by: Jiang Liu <jiang....@huawei.com>
>> ---
>> 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.
>
Hi Wangpeng,
Since unpoison is only for debugging, so it's unnecessary to add the lock
here, right?
> /*
> * 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!
>
Just change the name?
Thanks
Xishi Qiu
> Regards,
> Wanpeng Li
>
>> return ret;
>> }
>> --
>> 1.7.1
>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majord...@kvack.org. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"d...@kvack.org";> em...@kvack.org </a>
>
>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
