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=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=no 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 50F53C43603 for ; Thu, 5 Dec 2019 17:39:47 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 230B021835 for ; Thu, 5 Dec 2019 17:39:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 230B021835 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A94DF6B115E; Thu, 5 Dec 2019 12:39:46 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A458A6B115F; Thu, 5 Dec 2019 12:39:46 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 95B776B1160; Thu, 5 Dec 2019 12:39:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0144.hostedemail.com [216.40.44.144]) by kanga.kvack.org (Postfix) with ESMTP id 7D93D6B115E for ; Thu, 5 Dec 2019 12:39:46 -0500 (EST) Received: from smtpin27.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 4208D824CA0B for ; Thu, 5 Dec 2019 17:39:46 +0000 (UTC) X-FDA: 76231800372.27.bean41_553548f1b7f0c X-HE-Tag: bean41_553548f1b7f0c X-Filterd-Recvd-Size: 4697 Received: from out4436.biz.mail.alibaba.com (out4436.biz.mail.alibaba.com [47.88.44.36]) by imf18.hostedemail.com (Postfix) with ESMTP for ; Thu, 5 Dec 2019 17:39:45 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R181e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04420;MF=yang.shi@linux.alibaba.com;NM=1;PH=DS;RN=11;SR=0;TI=SMTPD_---0Tk3EpCO_1575567568; Received: from US-143344MP.local(mailfrom:yang.shi@linux.alibaba.com fp:SMTPD_---0Tk3EpCO_1575567568) by smtp.aliyun-inc.com(127.0.0.1); Fri, 06 Dec 2019 01:39:31 +0800 Subject: Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node To: Qian Cai Cc: fabecassis@nvidia.com, jhubbard@nvidia.com, mhocko@suse.com, cl@linux.com, vbabka@suse.cz, mgorman@techsingularity.net, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <1575519678-86510-1-git-send-email-yang.shi@linux.alibaba.com> <9E51ECF6-E9E8-4772-B7D8-7E528DD56A89@lca.pw> From: Yang Shi Message-ID: Date: Thu, 5 Dec 2019 09:39:25 -0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <9E51ECF6-E9E8-4772-B7D8-7E528DD56A89@lca.pw> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 12/5/19 1:42 AM, Qian Cai wrote: > >> On Dec 4, 2019, at 11:21 PM, Yang Shi wro= te: >> >> Felix Abecassis reports move_pages() would return random status if the >> pages are already on the target node by the below test program: >> >> ---8<--- >> >> int main(void) >> { >> const long node_id =3D 1; >> const long page_size =3D sysconf(_SC_PAGESIZE); >> const int64_t num_pages =3D 8; >> >> unsigned long nodemask =3D 1 << node_id; >> long ret =3D set_mempolicy(MPOL_BIND, &nodemask, sizeof(nodemask))= ; >> if (ret < 0) >> return (EXIT_FAILURE); >> >> void **pages =3D malloc(sizeof(void*) * num_pages); >> for (int i =3D 0; i < num_pages; ++i) { >> pages[i] =3D mmap(NULL, page_size, PROT_WRITE | PROT_READ, >> MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS, >> -1, 0); >> if (pages[i] =3D=3D MAP_FAILED) >> return (EXIT_FAILURE); >> } >> >> ret =3D set_mempolicy(MPOL_DEFAULT, NULL, 0); >> if (ret < 0) >> return (EXIT_FAILURE); >> >> int *nodes =3D malloc(sizeof(int) * num_pages); >> int *status =3D malloc(sizeof(int) * num_pages); >> for (int i =3D 0; i < num_pages; ++i) { >> nodes[i] =3D node_id; >> status[i] =3D 0xd0; /* simulate garbage values */ >> } >> >> ret =3D move_pages(0, num_pages, pages, nodes, status, MPOL_MF_MOV= E); >> printf("move_pages: %ld\n", ret); >> for (int i =3D 0; i < num_pages; ++i) >> printf("status[%d] =3D %d\n", i, status[i]); >> } >> ---8<--- >> >> Then running the program would return nonsense status values: >> $ ./move_pages_bug >> move_pages: 0 >> status[0] =3D 208 >> status[1] =3D 208 >> status[2] =3D 208 >> status[3] =3D 208 >> status[4] =3D 208 >> status[5] =3D 208 >> status[6] =3D 208 >> status[7] =3D 208 >> >> This is because the status is not set if the page is already on the >> target node, but move_pages() should return valid status as long as it >> succeeds. The valid status may be errno or node id. >> >> We can't simply initialize status array to zero since the pages may be >> not on node 0. Fix it by updating status with node id which the page = is >> already on. And, it looks we have to update the status inside >> add_page_for_migration() since the page struct is not available outsid= e >> it. >> >> Make add_page_for_migration() return 1 if store_status() is failed in >> order to not mix up the status value since -EFAULT is also a valid >> status. > Don=E2=80=99t really feel it is a bug after all. As you mentioned, the = manpage was rather poorly written. Why it is not a good idea just update = the manpage or/and code comments instead to document the current behavior= ? There are definitely a few inconsistencies, but I think the manpage is=20 quite clear for this specific case, which says "status is an array of=20 integers that return the status of each page. The array contains valid=20 values only if move_pages() did not return an error." And, it looks=20 kernel just misbehaved since 4.17 due to the fixes commit, so it sounds=20 like a regression too.