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=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 F329EC32771 for ; Fri, 24 Jan 2020 14:15:56 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BFAE02075D for ; Fri, 24 Jan 2020 14:15:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BFAE02075D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 57A466B0003; Fri, 24 Jan 2020 09:15:56 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5293B6B0005; Fri, 24 Jan 2020 09:15:56 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4188C6B0006; Fri, 24 Jan 2020 09:15:56 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 273196B0003 for ; Fri, 24 Jan 2020 09:15:56 -0500 (EST) Received: from smtpin23.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with SMTP id CD595181AEF0B for ; Fri, 24 Jan 2020 14:15:55 +0000 (UTC) X-FDA: 76412726670.23.sort30_33ae517adbf00 X-HE-Tag: sort30_33ae517adbf00 X-Filterd-Recvd-Size: 4300 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by imf34.hostedemail.com (Postfix) with ESMTP for ; Fri, 24 Jan 2020 14:15:54 +0000 (UTC) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Jan 2020 06:15:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,357,1574150400"; d="scan'208";a="292642836" Received: from richard.sh.intel.com (HELO localhost) ([10.239.159.54]) by fmsmga001.fm.intel.com with ESMTP; 24 Jan 2020 06:15:26 -0800 Date: Fri, 24 Jan 2020 22:15:38 +0800 From: Wei Yang To: Michal Hocko Cc: Wei Yang , akpm@linux-foundation.org, yang.shi@linux.alibaba.com, jhubbard@nvidia.com, vbabka@suse.cz, cl@linux.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero Message-ID: <20200124141538.GA12509@richard> Reply-To: Wei Yang References: <20200119065753.21694-1-richardw.yang@linux.intel.com> <20200124072127.GO29276@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200124072127.GO29276@dhcp22.suse.cz> User-Agent: Mutt/1.9.4 (2018-02-28) 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 Fri, Jan 24, 2020 at 08:21:27AM +0100, Michal Hocko wrote: >[Sorry I have missed this patch previously] > No problem, thanks for your comment. >On Sun 19-01-20 14:57:53, Wei Yang wrote: >> If we get here after successfully adding page to list, err would be >> 1 to indicate the page is queued in the list. >> >> Current code has two problems: >> >> * on success, 0 is not returned >> * on error, if add_page_for_migratioin() return 1, and the following err1 >> from do_move_pages_to_node() is set, the err1 is not returned since err >> is 1 > >This made my really scratch my head to grasp. So essentially err > 0 >will happen when we reach the end of the loop and rely on the >out_flush flushing to migrate the batch. Then err contains the >add_page_for_migratioin return value. And that would leak to the >userspace. > >What would you say about the following wording instead? >" >out_flush part of do_pages_move is responsible for migrating the last >batch that accumulated while processing the input in the loop. >do_move_pages_to_node return value is supposed to override any >preexisting error (e.g. when the user input is garbage) but the current I am afraid I have a different understanding here. If we jump to out_flush on the test of node_isset(), err is -EACCESS. Current logic would return this instead of the error from do_move_pages_to_node(). Seems we don't override -EACCESS. Is my understanding correct? >logic is wrong because add_page_for_migration returns 1 when >successfully adding a page into the batch and therefore this will be the >last err value after the loop is processed without any actual error. >We want to override that value of course because do_pages_move would >return 1 to the userspace even without any errors. >" > >> And these behaviors break the user interface. >> >> Fixes: e0153fc2c760 ("mm: move_pages: return valid node id in status if the >> page is already on the target node"). >> Signed-off-by: Wei Yang > >Acked-by: Michal Hocko > >> >> --- >> v2: >> * put more words to explain the error case >> --- >> mm/migrate.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 86873b6f38a7..430fdccc733e 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1676,7 +1676,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> err1 = do_move_pages_to_node(mm, &pagelist, current_node); >> if (!err1) >> err1 = store_status(status, start, current_node, i - start); >> - if (!err) >> + if (err >= 0) >> err = err1; >> out: >> return err; >> -- >> 2.17.1 >> > >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me