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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90D43C433F5 for ; Wed, 3 Nov 2021 08:06:22 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 1271161186 for ; Wed, 3 Nov 2021 08:06:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1271161186 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=kvack.org Received: by kanga.kvack.org (Postfix) id 800DA6B006C; Wed, 3 Nov 2021 04:06:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 78A21940007; Wed, 3 Nov 2021 04:06:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 650A86B0072; Wed, 3 Nov 2021 04:06:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0168.hostedemail.com [216.40.44.168]) by kanga.kvack.org (Postfix) with ESMTP id 5322F6B006C for ; Wed, 3 Nov 2021 04:06:21 -0400 (EDT) Received: from smtpin06.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 0FFEE5ACE2 for ; Wed, 3 Nov 2021 08:06:21 +0000 (UTC) X-FDA: 78766886562.06.C61A6AF Received: from out30-132.freemail.mail.aliyun.com (out30-132.freemail.mail.aliyun.com [115.124.30.132]) by imf30.hostedemail.com (Postfix) with ESMTP id 381EFE0016BE for ; Wed, 3 Nov 2021 08:06:03 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R151e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04426;MF=baolin.wang@linux.alibaba.com;NM=1;PH=DS;RN=5;SR=0;TI=SMTPD_---0Uus2Jrk_1635926774; Received: from 30.21.176.137(mailfrom:baolin.wang@linux.alibaba.com fp:SMTPD_---0Uus2Jrk_1635926774) by smtp.aliyun-inc.com(127.0.0.1); Wed, 03 Nov 2021 16:06:15 +0800 Subject: Re: [PATCH] mm: migrate: Correct the hugetlb migration stats To: Zi Yan Cc: akpm@linux-foundation.org, shy828301@gmail.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <677EF981-F33E-4002-AA38-DD669C319284@nvidia.com> <29aa9c6e-7191-71bb-d8a3-e2695b18fa3e@linux.alibaba.com> <7f45b2c8-fd2c-345a-ec6c-43b8b1c06de1@linux.alibaba.com> <7E44019D-2A5D-4BA7-B4D5-00D4712F1687@nvidia.com> From: Baolin Wang Message-ID: <98eccb43-a8d1-fde1-65ab-6f81fed1faa3@linux.alibaba.com> Date: Wed, 3 Nov 2021 16:07:00 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <7E44019D-2A5D-4BA7-B4D5-00D4712F1687@nvidia.com> Content-Type: text/plain; charset=utf-8; format=flowed X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 381EFE0016BE X-Stat-Signature: s8rd1y3iwjpgdhyicu6yjo86yge36knw Authentication-Results: imf30.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=alibaba.com; spf=pass (imf30.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.132 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com X-HE-Tag: 1635926763-689153 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 2021/11/3 3:30, Zi Yan wrote: > On 2 Nov 2021, at 2:08, Baolin Wang wrote: >=20 >> On 2021/11/1 23:12, Zi Yan wrote: >>> On 1 Nov 2021, at 2:54, Baolin Wang wrote: >>> >>>> On 2021/10/29 23:43, Zi Yan wrote: >>>>> On 29 Oct 2021, at 3:42, Baolin Wang wrote: >>>>> >>>>>> Now hugetlb migration is also available for some scenarios, such a= s >>>>>> soft offling or memory compaction. So we should correct the migrat= ion >>>>> >>>>> hugetlb migration is available at the time if (PageHuge(page)) bran= ch >>>>> is added. I am not sure what is new here. >>>> >>>> No new things actually, sorry for confusing and will update the comm= it message in next version. >>>> >>>>> >>>>>> stats for hugetlb with using compound_nr() instead of thp_nr_pages= () >>>>>> to get the number of pages. >>>>> >>>>> nr_failed records the number of pages, not subpages. It is returned= to >>>> >>>> I also think nr_failed should record the number of pages, not the nu= mber of hugetlb, if I understand you correctly. >>>> >>>>> user space when move_pages() syscall is used. After your change, >>>>> if users try to migrate a list of pages including THPs and/or huget= lb >>>>> pages and some of THPs and/or hugetlb fail to migrate, move_pages() >>>>> will return a number larger than the number of pages the users trie= d >>>> >>>> OK, thanks for pointing out the issue. >>>> >>>> But before my patch, we've already returned the number of pages succ= essed or failed for THP migration, instead of the number of THP. That mea= ns if we just move only 1 page by >>> >>> Ah, you are right. >>> >>>> move_pages() and if this page is 2M THP, so move_pages() will return= 512 if failed to migrate, which is larger than the page count specified = from user. >>>> >>>> if (err > 0) >>>> err +=3D nr_pages - i - 1; >>> >>> I am not sure this is right for user-space. >>> >>>> >>>> On the other hand, the stats of PGMIGRATE_SUCCESS/PGMIGRATE_FAIL sho= uld stand for the number of pages, instead of the number of hugetlb. Also= for hugetlb migration when memory compaction, we've already counted the = number of pages for a hugetlb into cc->nr_migratepages, if the hugetlb mi= gration failed, the trace stat of compaction will be confusing if we retu= rn the number of hugetlb. >>>> >>>> trace_mm_compaction_migratepages(cc->nr_migratepages, err, = &cc->migratepages); >>>> >>>> So I think the stats of hugetlb migration should be consistent with = THP. >>> >>> It makes sense to me. >>> >>>> >>>>> to migrate. I am not sure this is the change we want. Or at least, >>>>> the comment of migrate_pages() and the manpage of move_pages() need >>>>> to be changed and linux-api mailing list should be cc=E2=80=99d. >>>> >>>> I don't think we should update the comments of migrate_pages(), "Ret= urns the number of pages that were not migrated" makes sense to me if I u= nderstand correctly. >>>> >>>> For the manpage of move_pages(), as you said, the the returned non-m= igrate page numbers can be larger than the numbers specified from user if= failed to migrate a THP or a hugetlb. I am not sure if we should change = the manpage, since the THP already did, but I can send a patch to update = the manpage if you think this is still necessary. Thanks. >>> >>> I am not sure changing manpage would help the users of move_pages() a= fter >>> think about it again, since users might not know all the THP and/or h= ugetlb >>> information when they call move_pages() and they just pass a list of = N pages. > >>> I just wonder if we could fix the rc value of migrate_pages to return >>> the number of {base page, THP, hugetlb} instead, so that move_pages() >>> can get its return value right. >> >> IMO it will break the usage in other places if we change the rc value = of migrate_pages(), for example, the page migration when doing memory com= paction as I said before, which will expect the number of normal pages. M= eanwhile the THP page can be split into normal pages during migration, so= it will not be consistent if we return the number of THP. >=20 > You mean the above trace_mm_compaction_migratepages()? I checked all mi= grate_pages() Right. > callers and none of them cares about the actual number of non-migrated = pages, except > do_move_pages_to_node() and trace_mm_compaction_migratepages(). The for= mer expects > the number of before-split-and-not-subpage pages, whereas the latter ex= pects, like > you said, the number of base pages. Yes. >> >> Changing the return value of migrate_pages() will make things more com= plicated, and I am not sure whether it is worth doing. Any suggestion? Th= anks. >=20 > How about 1) fixing migrate_pages() to return the number of before-spli= t-and-not-subpage > pages, and 2) replace err with nr_succeeded (you can get it via *ret_su= cceeded in > migrate_pages()) in trace_mm_compaction_migratepages()? As a result, us= er-space move_pages() > will be fixed and trace_mm_compaction_migratepages() gives a > different but correct number as you want (you can still get nr_nonmigra= ted =3D > nr_migratepages - nr_succeeded). OK, sounds reasonable to me, I can try it. Thanks for your input.