From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.9 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C7C69C3F2C6 for ; Tue, 3 Mar 2020 06:12:05 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 75B3620870 for ; Tue, 3 Mar 2020 06:12:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="G2GjwNv9" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 75B3620870 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 137596B0006; Tue, 3 Mar 2020 01:12:05 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0E75B6B0007; Tue, 3 Mar 2020 01:12:05 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F3F106B0008; Tue, 3 Mar 2020 01:12:04 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0051.hostedemail.com [216.40.44.51]) by kanga.kvack.org (Postfix) with ESMTP id DCCA66B0006 for ; Tue, 3 Mar 2020 01:12:04 -0500 (EST) Received: from smtpin11.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id CD7B9180AD801 for ; Tue, 3 Mar 2020 06:12:04 +0000 (UTC) X-FDA: 76553030568.11.lunch05_86881e8af031d X-HE-Tag: lunch05_86881e8af031d X-Filterd-Recvd-Size: 6916 Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) by imf34.hostedemail.com (Postfix) with ESMTP for ; Tue, 3 Mar 2020 06:12:04 +0000 (UTC) Received: by mail-ot1-f65.google.com with SMTP id a20so1923190otl.0 for ; Mon, 02 Mar 2020 22:12:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=Gq/3yKSGvZrh++9IegruTZ10brzhg9cnheydTKKfza0=; b=G2GjwNv9lN32Lho4nclFA19SKiJf2vjcB7YJjjntip7ZmL5NB/zTb/C/qNY5lF6ohw xZj1d9Zi7FmqdQZP39pTh9yYMxTUvzFIcHglxvFcGwF2O7ni1gGwXEMzXiOnov+FjUso nUchSYqjnJAklZ5PZRIgMGmxymmWzexAS+H10mnAsEqYk3yVUvnucz40TBPtAih4G6wx hXCY/2/ylat0wl7M3+Rb+47FdzfxkjQh5XYvf078iLuRVx8M4xgHSSA/dwzEzDiY3RV2 uFFB/3ZGqHlE64cWHlQxtSnz45fbgCfMDMDCQoif5QBxjpoqK8iO7s0AzfqBttW6ntZn ascw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=Gq/3yKSGvZrh++9IegruTZ10brzhg9cnheydTKKfza0=; b=T68r0jTgpuHBZ1MjWeyP/wUfkIuUh0EZ9TTTjYb1l9pPbZuGt66OWJNni1DK1Mqe40 GP1JgkkFIdOdMJqAlbIOqkugHHqUmjg8uBXBTRoxhXHhZyGO9Ae9NvNx3WV98g+hticU OMwRRb0rhokEXYI0fTnZoUM9IwYQvgGNjmsEO2iJ4Sh+TRi6VOgoyc55fJ6d1PBs/T4L 8TEBVz2fLl+1HUMdChbfOPvWptfFKSrZPX88BmM1JhzeTPqojhME7SD7y5Vkx3DZnzUQ xxuuUwcZXFkgsBeNaH0Zo/V5wGTv5VvWmr54pXDsI99VJJGRNc/licUlqpQDtQEgybyl CaEA== X-Gm-Message-State: ANhLgQ3Rbl61CJzAXbhxXJSwsCkcrnQR2EjnvfIJdeKVSiPnMpnUkcqX /aDfg3wiSISsnFs2ZHk4qFaO+Q== X-Google-Smtp-Source: ADFU+vv95CXgWq8Xutk77esPJtjt3xQD8GBhtSJlxQcGmNWM+9/c8qSQT4iINbrY94/MJlh9qRtClw== X-Received: by 2002:a05:6830:114:: with SMTP id i20mr2230238otp.320.1583215922975; Mon, 02 Mar 2020 22:12:02 -0800 (PST) Received: from eggly.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id x69sm3512317oix.50.2020.03.02.22.12.01 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 02 Mar 2020 22:12:02 -0800 (PST) Date: Mon, 2 Mar 2020 22:11:45 -0800 (PST) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: "Huang, Ying" cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Dave Hansen , David Hildenbrand , Mel Gorman , Vlastimil Babka , Zi Yan , Michal Hocko , Minchan Kim , Johannes Weiner , Hugh Dickins Subject: Re: [PATCH] mm, migrate: Check return value of try_to_unmap() In-Reply-To: <20200303033645.280694-1-ying.huang@intel.com> Message-ID: References: <20200303033645.280694-1-ying.huang@intel.com> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, 3 Mar 2020, Huang, Ying wrote: > From: Huang Ying > > During the page migration, try_to_unmap() is called to replace the > page table entries with the migration entries. Now its return value > is ignored, that is generally OK in most cases. But in theory, it's > possible that try_to_unmap() return false (failure) for the page > migration after arch_unmap_one() is called in unmap code. Even if > without arch_unmap_one(), it makes code more robust for the future > code changing to check the return value. No. This patch serves no purpose, and introduces a bug. More robust? Robustness is provided by the later expected_count checks, with freezing where necessary. Saving time by not going further if try_to_unmap() failed? That's done by the page_mapped() checks immediately following the try_to_unmap() calls (just out of sight in two cases). > > Known issue: I don't know what is the appropriate error code for > try_to_unmap() failure. Whether EIO is OK? -EAGAIN is the rc already being used for this case, and which indicates that retries may be appropriate (but they're often scary overkill). > > Signed-off-by: "Huang, Ying" > Cc: Dave Hansen > Cc: David Hildenbrand > Cc: Mel Gorman > Cc: Vlastimil Babka > Cc: Zi Yan > Cc: Michal Hocko > Cc: Minchan Kim > Cc: Johannes Weiner > Cc: Hugh Dickins > --- > mm/migrate.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 3900044cfaa6..981f8374a6ef 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1116,8 +1116,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage, > /* Establish migration ptes */ > VM_BUG_ON_PAGE(PageAnon(page) && !PageKsm(page) && !anon_vma, > page); > - try_to_unmap(page, > - TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS); > + if (!try_to_unmap(page, > + TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS)) { > + rc = -EIO; > + goto out_unlock_both; No: even if try_to_unmap() says that it did not entirely succeed, it may have unmapped some ptes, inserting migration entries in their place. Those need to be replaced by proper ptes before the page is unlocked, which page_was_mapped 1 and remove_migration_ptes() do; but this goto skips those. > + } > page_was_mapped = 1; > } > > @@ -1337,8 +1340,11 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, > goto put_anon; > > if (page_mapped(hpage)) { > - try_to_unmap(hpage, > - TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS); > + if (!try_to_unmap(hpage, > + TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS)) { > + rc = -EIO; > + goto unlock_both; Same as above. > + } > page_was_mapped = 1; > } > > @@ -1349,6 +1355,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, > remove_migration_ptes(hpage, > rc == MIGRATEPAGE_SUCCESS ? new_hpage : hpage, false); > > +unlock_both: > unlock_page(new_hpage); > > put_anon: > @@ -2558,8 +2565,7 @@ static void migrate_vma_unmap(struct migrate_vma *migrate) > continue; > > if (page_mapped(page)) { > - try_to_unmap(page, flags); > - if (page_mapped(page)) > + if (!try_to_unmap(page, flags)) > goto restore; > } > > -- > 2.25.0