Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751334AbdHXHg3 convert rfc822-to-8bit (ORCPT ); Thu, 24 Aug 2017 03:36:29 -0400 Received: from tyo162.gate.nec.co.jp ([114.179.232.162]:56164 "EHLO tyo162.gate.nec.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751216AbdHXHg2 (ORCPT ); Thu, 24 Aug 2017 03:36:28 -0400 From: Naoya Horiguchi To: Zi Yan CC: Greg Kroah-Hartman , "Kirill A . Shutemov" , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "Zi Yan" Subject: Re: [RFC PATCH 3/4] mm: soft-offline: retry to split and soft-offline the raw error if the original THP offlining fails. Thread-Topic: [RFC PATCH 3/4] mm: soft-offline: retry to split and soft-offline the raw error if the original THP offlining fails. Thread-Index: AQHTFWkxoR8jwUDvu0ut2SvkozVvVKKSlGiA Date: Thu, 24 Aug 2017 07:31:52 +0000 Message-ID: <4c271e6e-c25a-8ce6-0765-20f93c21eb44@ah.jp.nec.com> References: <20170815015216.31827-1-zi.yan@sent.com> <20170815015216.31827-4-zi.yan@sent.com> In-Reply-To: <20170815015216.31827-4-zi.yan@sent.com> Accept-Language: en-US, ja-JP Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.128.101.9] Content-Type: text/plain; charset="iso-2022-jp" Content-ID: <477FE2A80605E8439919A83D44C3C048@gisp.nec.co.jp> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5204 Lines: 156 On Mon, Aug 14, 2017 at 09:52:15PM -0400, Zi Yan wrote: > From: Zi Yan > > For THP soft-offline support, we first try to migrate a THP without > splitting. If the migration fails, we split the THP and migrate the > raw error page. > > migrate_pages() does not split a THP if the migration reason is > MR_MEMORY_FAILURE. > > Signed-off-by: Zi Yan > --- > mm/memory-failure.c | 77 +++++++++++++++++++++++++++++++++++++---------------- > mm/migrate.c | 16 +++++++++++ > 2 files changed, 70 insertions(+), 23 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 8a9ac6f9e1b0..c05107548d72 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c ... > @@ -1657,23 +1658,53 @@ static int __soft_offline_page(struct page *page, int flags) > * cannot have PAGE_MAPPING_MOVABLE. > */ > if (!__PageMovable(page)) > - inc_node_page_state(page, NR_ISOLATED_ANON + > - page_is_file_cache(page)); > - list_add(&page->lru, &pagelist); > + mod_node_page_state(page_pgdat(hpage), NR_ISOLATED_ANON + > + page_is_file_cache(hpage), hpage_nr_pages(hpage)); > +retry_subpage: > + list_add(&hpage->lru, &pagelist); > ret = migrate_pages(&pagelist, new_page, NULL, MPOL_MF_MOVE_ALL, > MIGRATE_SYNC, MR_MEMORY_FAILURE); > if (ret) { > - if (!list_empty(&pagelist)) > - putback_movable_pages(&pagelist); > - > + if (!list_empty(&pagelist)) { > + if (!PageTransHuge(hpage)) > + putback_movable_pages(&pagelist); > + else { > + lock_page(hpage); > + if (split_huge_page_to_list(hpage, &pagelist)) { > + unlock_page(hpage); > + goto failed; Don't you need to call putback_movable_pages() before returning? > + } > + unlock_page(hpage); > + > + if (split) > + *split = 1; > + /* > + * Pull the raw error page out and put back other subpages. > + * Then retry the raw error page. > + */ > + list_del(&page->lru); > + putback_movable_pages(&pagelist); If putback_movable_pages() is not called for the raw error page, NR_ISOLATED_ANON or NR_ISOLATED_FILE stat remains incremented by 1 after return? > + hpage = page; > + goto retry_subpage; > + } > + } > +failed: > pr_info("soft offline: %#lx: migration failed %d, type %lx (%pGp)\n", > - pfn, ret, page->flags, &page->flags); > + pfn, ret, hpage->flags, &hpage->flags); > if (ret > 0) > ret = -EIO; > } > + /* > + * Set PageHWPoison on the raw error page. > + * > + * If the page is a THP, PageHWPoison is set then cleared > + * in its head page in migrate_pages(). So we need to set the raw error > + * page here. Otherwise, setting PageHWPoison again is fine. > + */ > + SetPageHWPoison(page); When we failed to migrate the page, we don't have to set PageHWPoison because we can still continue to use the page (which tends to cause corrected error but is still usable) and we have a chance to retry soft-offline later. So please set the flag only when page/thp migration succeeds. ... > diff --git a/mm/migrate.c b/mm/migrate.c > index f7b69282d216..b44df9cf72fd 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1118,6 +1118,15 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, > } > > if (unlikely(PageTransHuge(page) && !PageTransHuge(newpage))) { > + /* > + * soft-offline wants to retry the raw error subpage, if the THP > + * migration fails. So we do not split the THP here and exit directly. > + */ > + if (reason == MR_MEMORY_FAILURE) { > + rc = -ENOMEM; > + goto put_new; > + } > + This might imply that existing thp migration code have a room for improvment. > lock_page(page); > rc = split_huge_page(page); > unlock_page(page); This code maybe doesn't work. Even when split_huge_page() succeeds (rc = 0), __unmap_and_move() migrates only the head page and all tail pages are ignored (note that split_huge_page_to_list() sends tail pages to LRU list when parameter 'list' is NULL.) IOW, the retry logic in migrate_pages() doesn't work for thp migration. What I think is OK is that - when thp migration from soft offline fails, 1. split the thp 2. send only raw error page to migration page list and send the other healthy subpages to LRU list 3. retry loop in migrate_pages() - when thp migration from other callers fails, 1. split the thp 2. send all subpages to migration page list 3. retry loop in migrate_pages() > @@ -1164,6 +1173,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, > */ > if (!test_set_page_hwpoison(page)) > num_poisoned_pages_inc(); > + > + /* > + * Clear PageHWPoison in the head page. The caller > + * is responsible for setting the raw error page. > + */ > + if (PageTransHuge(page)) > + ClearPageHWPoison(page); I think that you can write more simply with passing the pointer to the raw error page from caller. It is also helpful for above-mentioned split-retry issue. unmap_and_move() has a parameter 'private' which is now only used for get/put_new_page(), so I think we can extend it for our purpose. Thanks, Naoya Horiguchi