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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF78EEB64DD for ; Fri, 11 Aug 2023 07:38:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E57AD6B0071; Fri, 11 Aug 2023 03:38:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E07876B0072; Fri, 11 Aug 2023 03:38:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CCF516B0074; Fri, 11 Aug 2023 03:38:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id B91B16B0071 for ; Fri, 11 Aug 2023 03:38:58 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 7CBFEA03FD for ; Fri, 11 Aug 2023 07:38:58 +0000 (UTC) X-FDA: 81111022356.16.2DF6A29 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.126]) by imf27.hostedemail.com (Postfix) with ESMTP id E47A24001B for ; Fri, 11 Aug 2023 07:38:55 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=RMCpQ7PW; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf27.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.126 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1691739536; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ZcfIBlZhg77EX3iZrAOjSh0vqqwCnBIiN063+TAAfbc=; b=T+z08Uy07VK7putgdEzVIY2XbvpEFS3OLZqSlM1AxWVdklZXyQk3RelD65/gM4y17i8upF J/I/cnqGj4hCtchk0G3iAoRNv3w7F0bCB3dnMLlcwR51f1Ilmu3mqAipj8AnXvjGicTdkX goll6ZVoEDCcdiMwMsNYS1VAaU/5f04= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=RMCpQ7PW; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf27.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.126 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1691739536; a=rsa-sha256; cv=none; b=s3pOtvNjtDEKdc89wWL5ZNgEAmO5qdl7cSG7OHsDftBgEIYM/WH1WIR6y6GihmAI1xfM7u MvlyK/6Ckarb631fRc6o2uO3EhVhHMGWCYmpzNUAmWKr0KXDW/0fftIWO7cgN6HkWsQQ/F n2G3OvvsU+beQ9HVXamxCY04UCzC66s= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1691739535; x=1723275535; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=hcKBt1M23hgo9IEP5r0ZmylMxt/+5kONxb2gvMrT0z4=; b=RMCpQ7PWCvlzPMpPXDpxEhj2IapNRGo2pRq/e67X+jE7c7WHGDu3tOLM i1SX3uF4S/b6B894s8PFk0Ye91KgCymuzFUo4a5QioZ/XWFsXSRn48TxR e75EiiZM38SeSBl6WzFGO7SU/zVUdthIvWkYwofJ7afS3Jp8O5lmjdPX/ GUJei355i2CK1+tVFWUMtjF9RDwJOb3+mXpyU3q0XxNVja6uXO0iIih9W HtGYt1COemfhQe1IPM1lLjODq8/BYS/G3JXYWtoNer1XQqQsquS8GIrXX lskg0smiaXH+4lrAEjsqBZfcwMA0mtDEUGovqDOZJV4fDAdcpp49y/CbK Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10798"; a="356583017" X-IronPort-AV: E=Sophos;i="6.01,164,1684825200"; d="scan'208";a="356583017" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Aug 2023 00:38:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10798"; a="822577127" X-IronPort-AV: E=Sophos;i="6.01,164,1684825200"; d="scan'208";a="822577127" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Aug 2023 00:38:42 -0700 From: "Huang, Ying" To: Alistair Popple Cc: Michal Hocko , Andrew Morton , , , , , , , Subject: Re: [PATCH 1/2] mm/migrate.c: Fix return code when migration fails References: <20230807063945.911582-1-apopple@nvidia.com> <87pm3z2fer.fsf@nvdebian.thelocal> <87jzu4alz3.fsf@nvdebian.thelocal> Date: Fri, 11 Aug 2023 15:37:05 +0800 In-Reply-To: <87jzu4alz3.fsf@nvdebian.thelocal> (Alistair Popple's message of "Wed, 09 Aug 2023 14:10:08 +1000") Message-ID: <87bkfec9ni.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Rspamd-Queue-Id: E47A24001B X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: tf6m5k9a7auipe5pup9s7qgp7rd68y8c X-HE-Tag: 1691739535-76419 X-HE-Meta: U2FsdGVkX19fHt09nK/ld/VsXcEaLKaphbpvVjVGUD80I4m0ZyyvOXOjQpedWYV1nImNwaEPAeJpkfjmTGPn6BzBc1nlRWd/sYsYVfSNh/Uo81D8PFFsRLitWzrYwMoZMz2WcaEhrM7q8rosZxnC5G/7v6ZMnQ1AW/AcKkLOhPkuez3hsc2SU5MxOKz2/QzysyUp42CnxR0l6Qdh+0o7FNJyKbCyOoOq8r85gCU0lkVj3iEiPS8nramZ88R0r0xkaaEKwPHXPTiPcGyTaZ0hF3HcIFZRhysIA3x9Yl5UEGDOJ1BV6KdCQw9XE/x2fdMkm05OcYvQzRwsqvFGkQ88U1pu5h6kUXAnW7hyKtSuh7ZbcCSt/XhnvczgMTC/clXLCtiowDlNk+der4Wq0utM8caRyi37XQVLj5A+4EGnsy8kmAVeEbZqbTfpuICQ9cPiMJm1DTh6kCnzhwsNz2hcEGyQt3tho1bu3UH4c/MdPxRpzhs/RUHlv+aroy0nwpmU1iYVLnUznQVcabXJhswF6a1rWrHQyc5Nh6THvHGEiKPOj0/FaYzSFn5eCHI86Br67ZGQtirg6OpCwDPWUJ6BiwSylchzWZ/Ambl8GP7k4SHK5FyP3bRtv0ocDravdsBxV3JcTHGqhu0bmHfNyEZ0mVWcaykjhKabOcoroz33vcecN7DSJyFxzR87XHi9mMTDCr09Jgnb13lCzFWxP8ClOitnVjwtUp33H+R2lXqWcVWeu22HNAemXBSdWlBuECQwPY6z/WqQ9xY2LG2PkHu9+qCdV819LHoxQsMyUiymWhcqaR5cl5m3vRUKI7gaGK8dt63/Ty6aFTZ571jhe7y5Tgo1ZLud9WFaMt+JjnjR7eNNUc8bg+yn82olOzoOXreufcubPse8AtwVJa0OsVqRJsyiSkKZEqwANzlseTD1m6/k7UmABoLd290BbxYusWmq83I3VyuRtzxvu3SB5EX Ux012A9q HX8TZ8HunUZ2HPacQ7mqO3v81g8iIMjWvO6LNIYztV1kozZ7LVxGKF9IyUl6kBEQQsUiJT4uphZ/AzDUY+D9tyR0oaO7A0MTZcRU8v1ufVPRxbLIvNdOqNQDU1HmfiqKMOIEM+IB5HlxMzS2AgWCwdTMYDBRweLabwyVHSp8Zo7w2wHq9aYMicb7zaNaOMTSFjhT6JfasGCwmVEHpDwseHwxZo9qPSQT6uRES1JUbF1Q1xL8IbNXkvYsslP+fl7DNaRQa 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: Alistair Popple writes: > Michal Hocko writes: > >> On Mon 07-08-23 22:31:52, Alistair Popple wrote: >>> >>> Michal Hocko writes: >>> >>> > On Mon 07-08-23 16:39:44, Alistair Popple wrote: >>> >> When a page fails to migrate move_pages() returns the error code in a >>> >> per-page array of status values. The function call itself is also >>> >> supposed to return a summary error code indicating that a failure >>> >> occurred. >>> >> >>> >> This doesn't always happen. Instead success can be returned even >>> >> though some pages failed to migrate. This is due to incorrectly >>> >> returning the error code from store_status() rather than the code from >>> >> add_page_for_migration. Fix this by only returning an error from >>> >> store_status() if the store actually failed. >>> > >>> > Error reporting by this syscall has been really far from >>> > straightforward. Please read through a49bd4d71637 and the section "On a >>> > side note". >>> > Is there any specific reason you are trying to address this now or is >>> > this motivated by the code inspection? >>> >>> Thanks Michal. There was no specific reason to address this now other >>> than I came across this behaviour when updating the migration selftest >>> to inspect the status array and thought it was odd. I was seeing pages >>> had failed to migrate according to the status argument even though >>> move_pages() had returned 0 (ie. success) rather than a number of >>> non-migrated pages. >> >> It is good to mention such a motivation in the changelog to make it >> clear. Also do we have a specific test case which trigger this case? > > Not explicitly/reliably although I could write one. > >>> If I'm interpreting the side note correctly the behaviour you were >>> concerned about was the opposite - returning a fail return code from >>> move_pages() but not indicating failure in the status array. IIUC, we cannot avoid this even if leaving code untouched. In move_pages_and_store_status(), if some pages fails to be migrated, we will not store status. In fact, we don't know which pages failed to be migrated in kernel too. >>> That said I'm happy to leave the behaviour as is, although in that case >>> an update to the man page is in order to clarify a return value of 0 >>> from move_pages() doesn't actually mean all pages were successfully >>> migrated. IMHO, return value is more important than "status" as the reason I mentioned above. At least, we can make one thing correct. >> While I would say that it is better to let old dogs sleep I do not mind >> changing the behavior and see whether anything breaks. I suspect nobody >> except for couple of test cases hardcoded to the original behavior will >> notice. >> >>> >> Signed-off-by: Alistair Popple >>> >> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") >> >> The patch itself looks good. I am not sure the fixes tag is accurate. >> Has the reporting been correct before this change? I didn't have time to >> re-read the original code which was quite different. > > I dug deeper into the history and the fixes tag is wrong. The behaviour > was actually introduced way back in commit e78bbfa82624 ("mm: stop > returning -ENOENT from sys_move_pages() if nothing got migrated"). As > you may guess from the title it was intentional, so suspect it is better > to update documentation. Can we change the code but keep the -ENOENT behavior with some special case? -- Best Regards, Huang, Ying >> Anyway >> Acked-by: Michal Hocko > > Thanks for looking, but I will drop this and see if I can get the man > page updated. > >> Anyway rewriting this function to clarify the error handling would be a >> nice exercise if somebody is interested. > > Yeah, everytime I look at this function I want to do that but haven't > yet found the time. > >>> >> --- >>> >> mm/migrate.c | 4 +++- >>> >> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >> >>> >> diff --git a/mm/migrate.c b/mm/migrate.c >>> >> index 24baad2571e3..bb3a37245e13 100644 >>> >> --- a/mm/migrate.c >>> >> +++ b/mm/migrate.c >>> >> @@ -2222,7 +2222,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >>> >> * If the page is already on the target node (!err), store the >>> >> * node, otherwise, store the err. >>> >> */ >>> >> - err = store_status(status, i, err ? : current_node, 1); >>> >> + err1 = store_status(status, i, err ? : current_node, 1); >>> >> + if (err1) >>> >> + err = err1; >>> >> if (err) >>> >> goto out_flush; >>> >> >>> >> -- >>> >> 2.39.2