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 713A3C4345F for ; Fri, 12 Apr 2024 20:21:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E0E556B0092; Fri, 12 Apr 2024 16:21:32 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DBD116B0093; Fri, 12 Apr 2024 16:21:32 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C5F096B0095; Fri, 12 Apr 2024 16:21:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id A4DCD6B0092 for ; Fri, 12 Apr 2024 16:21:32 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 5C8AE140DB7 for ; Fri, 12 Apr 2024 20:21:32 +0000 (UTC) X-FDA: 82002000024.29.A39D637 Received: from mail-ej1-f43.google.com (mail-ej1-f43.google.com [209.85.218.43]) by imf23.hostedemail.com (Postfix) with ESMTP id 72AB6140005 for ; Fri, 12 Apr 2024 20:21:30 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=mev0vrVk; spf=pass (imf23.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.43 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712953290; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=EGFW6CnMszNqSWAsUr3yiNIPdWw7VUWE85giIQlsZvc=; b=TAKifTupbSUoFOd7VffCMSglSJMitymAgJ5Bp4ygkfHlda3CcXoUXujQh4xAqgSKaNW41z lfxUg3ojzHLr39cCQNpOFzoqqWaU846S1UUv6Oy8cr4m8gv0pX2MTb+MGV6DniE3RDxBKq gUX3u+kUWm55m/zc+DeITQTTNxC8Qgo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712953290; a=rsa-sha256; cv=none; b=Avv9HGolEw2SkxqsK+KSGj8emhCqhnZGHGUDyu/TZSyRHYQQiC04OHuuRSTB2leFhNq/TJ CCP/SdW0iKk1NUTtIgq2ZYfWsIQHJCzsNiU5drMNMqk4cadV7r7ptlEncGMyvr7iJotBwi QO9WLZRBsAMcQvHYbRflOu2OzrYitKs= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=mev0vrVk; spf=pass (imf23.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.43 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ej1-f43.google.com with SMTP id a640c23a62f3a-a44f2d894b7so141782066b.1 for ; Fri, 12 Apr 2024 13:21:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1712953289; x=1713558089; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=EGFW6CnMszNqSWAsUr3yiNIPdWw7VUWE85giIQlsZvc=; b=mev0vrVkaDOlJewKEmxDIK6/wDC5FznNemHYi5vUVHvCHx0XKU9hGj638fubcpf3Hu GyFvTzUiGIH1heLZZtkxzPUtgiekH7Pr2Gyn0U1yBa2p9LrBlbe4ttvoKtkshThs1riW aKEQSD/kmvpZGuZAJNuIksZjS8cZkplnu1WHZlOdjbYhqcOQVPCrFvU0/U3mkL1wrKaZ 6K4o/h+DI90KrDPSOfPMMbn7mUzyr19JiFotxrJ5/aE06cFzDZbcrNmgqCdUuSyzqUoh JiplbZUhczAwfLl+USIz1nPogcZHumezAkpnUq8FrC+ICygz+sWGGVvU17izWOms3v93 WsqA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712953289; x=1713558089; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=EGFW6CnMszNqSWAsUr3yiNIPdWw7VUWE85giIQlsZvc=; b=VqnvtgotQnyCaw367sp3fU/UshTB9ST79Ro18EWucqwpSzcHp7LhTmV0MG2HXj/BM7 C0siasrcjPpsXCv7ZdMiH7swqMLPEoZo/qvGjpZ9WLLtlMkqMG025GlFs/aTakfcFkwR 6oh+18YkAhKO1D4bdQXBYZW3JEPFGgvFFgNkQw+/0OF4CazcAybA5z7QnPVla+kG6pzW lIp38m1Qb2jio5ZoBvOOjUsnlw/P8C3sYko3pjEm8PSSyaCSTwTWliOwzVGdgNFtpRQ3 jNiPDHJbiptuMcyXp9rKx3sHoXnGb2iPs+XvS6rroF48DAe1elWHc6kYIxymUwwKh+AL NHGQ== X-Forwarded-Encrypted: i=1; AJvYcCVp0IQonkFQGra6lP7uRhfqg8Qyo2rBSpnGJdKMfzkWtmaBWLUQmCcMffESxzolAyqZhJGWHTJFhztO3VFFXJv0B4o= X-Gm-Message-State: AOJu0YwF6+Yiqq7uOgmKO2s++vAfsNRuSp00AKgxwbDPA0G4qoBU5RIB qh5LL0Ue/u3p7vuEd0aTMYj0XlJ3M6B3oklJ49uO83JpH4XORTCvFbK2liuOpyvGRpPpdqf5VnE rbBCOJIPaa1QD/SGlWt2jA6fdcoY= X-Google-Smtp-Source: AGHT+IGRIEDvMAPDilSJiq3VU+yXpUrLZRTHOVb1pA2EQjbD3GZ71V+gTDELD+eURh0H50rE8g5cX4Zi5PXxM/1rEEc= X-Received: by 2002:a17:906:dc8f:b0:a51:af7d:4652 with SMTP id cs15-20020a170906dc8f00b00a51af7d4652mr3603557ejc.32.1712953288499; Fri, 12 Apr 2024 13:21:28 -0700 (PDT) MIME-Version: 1.0 References: <20240411153232.169560-1-zi.yan@sent.com> <86722546-1d54-4224-9f31-da4f368cd47e@redhat.com> <0A4D2CA1-E156-43E9-A1C9-E09E62E760A3@nvidia.com> <9e4516d9-b861-4501-84d3-31f5e8e5dfef@redhat.com> In-Reply-To: <9e4516d9-b861-4501-84d3-31f5e8e5dfef@redhat.com> From: Yang Shi Date: Fri, 12 Apr 2024 13:21:17 -0700 Message-ID: Subject: Re: [PATCH] mm/rmap: do not add fully unmapped large folio to deferred split list To: David Hildenbrand Cc: Zi Yan , linux-mm@kvack.org, Andrew Morton , Matthew Wilcox , Ryan Roberts , Barry Song <21cnbao@gmail.com>, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 72AB6140005 X-Rspam-User: X-Stat-Signature: kk7dr4z9e36q93zcwmywwkuxm66wsh9i X-Rspamd-Server: rspam03 X-HE-Tag: 1712953290-756847 X-HE-Meta: U2FsdGVkX19pYx5ceVtJcTv/R/fmB2BtiyTTEZcKrVs1MbMWqXpUtMYwqDYxEckNXX5jwiJ7dAtdCFrwEMxYX15ckhT1VEWsldkLro5u8PpMTT4JmPsWyET389mVHIPJkWj93eKkIoZQKtMsHAIpRC5ASwSTd2LnpMnFYd4+t4M5CF/7CGxad/lYO8MVkS1d2QlpLBw2AkdQhu+rnfuHyIEHLa3KB/hW2Gv3+pcNBakWqFSfTTC/liHuM5x4gMqrIIEG6SwYZC4P/E7iAUchRZK7PMVswqWKzGnMUlZWiTlWgvouy+z3RA724k6Rjp0lgEojqyys2TZH6xETL30lmaF38BCC+520o+WS/jnmW5O27B+UUuaeRP7tRpVx9HxlS6exPO/tVF/NyaTWRzg6J9kGtBecJ9msZi+Ug9kUgjNTSp5aLuLRmQWK9ymCmOu68+rkUJ+puaPU+ZC6wgNnBWYwkOqxEA4+XxB/YmDH/kayDpmMjWPCkuwLm7t/Acn2xistBVxTovSnGKPLGRKYV2Hx8fip5UgKXZHGwarVkx1xj0/zHr+nkcWuBP18QGl31wSepnxCIz3uQFLwu/Nj1r9JSMq8SSKKdI5WhJNty9PT/mntlPclQNbwihhfKtAdSkcBCy9LwM5asFeknr9pTpLGYSZH4l8SmsIPcNxUEZbxhJMNADmtJW5QKJFC2k3GqXsyeanf1LDi1mnb+mJQmEOcF438Go+r4XjScqiQAsLZbvpcV2CTpSjmH+4h/dw37h+Ofl6M/QFddraG0oi2FbIjT2zxxuvsiI4FE1ypQZUinnm46BZhF0i2AO5Mo/C+IUyHlVmNwx4ecPUb0dhDtp2jUq6MXzue5VadDSILHrjHvQtfF9b/SANK7RkMsPiaxzox9ceU/68nrUISH5CHZD9mjrrcE11hRAsjtwQnDoMlBZpHhzamvBA6Pcx+jjhT9QRjOLt7m7up4RLxJkl Ybu35gEB TJa7DS1fyJ3aMxtSeKDt8/0be5NBoh4B2YxdnWbFDzGYeacXejohi+5b6SLLXF5/xZQIlZ5t4eknUmmJNfZKKObknu4/gNyiLgnznB8WBH1Uyu9j+BxW8lA8XrL7dL1PPqi/sX2J5hZLMQ79M3qDr29qUah3dabo34R56ETb+v7aXhuWhLdUUumWv7IgzveYU0KOQ7k7tt6VbcqvtArpIA+CGZFoPaFGUCzIRubMtJm7OSVheBFBwc88J3KCKVaLN217ALwHYpTBzvUuqt1WU+AqPsGxGPL80G1R8/A2ah69T3yzeK6jeEzqBBaBcHxWHi6r3lDTeoAUS82/Nc9hmloIUr9PWEj2Y7ZsZsLV7cIdeNEZ7eXvWIiG26QMenRXdCNqcHYCoWrbBuqcpGYY47302YyHa9BAhF0MyMzpVeHw3UoSjKK3FQAEG6OPPtAzK2TfSvSadB4bXkqXzaB4vDlv3Q6lvFBdaQPQEwFTazi28zg42v34tsgxzS4TEGzO/ICCcj0cUc68PB1XvsYhx9qzFaPrGWJEpSSdP 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: List-Subscribe: List-Unsubscribe: On Fri, Apr 12, 2024 at 12:36=E2=80=AFPM David Hildenbrand wrote: > > On 12.04.24 20:29, Yang Shi wrote: > > On Fri, Apr 12, 2024 at 7:31=E2=80=AFAM Zi Yan wrote: > >> > >> On 12 Apr 2024, at 10:21, Zi Yan wrote: > >> > >>> On 11 Apr 2024, at 17:59, Yang Shi wrote: > >>> > >>>> On Thu, Apr 11, 2024 at 2:15=E2=80=AFPM David Hildenbrand wrote: > >>>>> > >>>>> On 11.04.24 21:01, Yang Shi wrote: > >>>>>> On Thu, Apr 11, 2024 at 8:46=E2=80=AFAM David Hildenbrand wrote: > >>>>>>> > >>>>>>> On 11.04.24 17:32, Zi Yan wrote: > >>>>>>>> From: Zi Yan > >>>>>>>> > >>>>>>>> In __folio_remove_rmap(), a large folio is added to deferred spl= it list > >>>>>>>> if any page in a folio loses its final mapping. It is possible t= hat > >>>>>>>> the folio is unmapped fully, but it is unnecessary to add the fo= lio > >>>>>>>> to deferred split list at all. Fix it by checking folio mapcount= before > >>>>>>>> adding a folio to deferred split list. > >>>>>>>> > >>>>>>>> Signed-off-by: Zi Yan > >>>>>>>> --- > >>>>>>>> mm/rmap.c | 9 ++++++--- > >>>>>>>> 1 file changed, 6 insertions(+), 3 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/mm/rmap.c b/mm/rmap.c > >>>>>>>> index 2608c40dffad..d599a772e282 100644 > >>>>>>>> --- a/mm/rmap.c > >>>>>>>> +++ b/mm/rmap.c > >>>>>>>> @@ -1494,7 +1494,7 @@ static __always_inline void __folio_remove= _rmap(struct folio *folio, > >>>>>>>> enum rmap_level level) > >>>>>>>> { > >>>>>>>> atomic_t *mapped =3D &folio->_nr_pages_mapped; > >>>>>>>> - int last, nr =3D 0, nr_pmdmapped =3D 0; > >>>>>>>> + int last, nr =3D 0, nr_pmdmapped =3D 0, mapcount =3D 0; > >>>>>>>> enum node_stat_item idx; > >>>>>>>> > >>>>>>>> __folio_rmap_sanity_checks(folio, page, nr_pages, level)= ; > >>>>>>>> @@ -1506,7 +1506,8 @@ static __always_inline void __folio_remove= _rmap(struct folio *folio, > >>>>>>>> break; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - atomic_sub(nr_pages, &folio->_large_mapcount); > >>>>>>>> + mapcount =3D atomic_sub_return(nr_pages, > >>>>>>>> + &folio->_large_mapcou= nt) + 1; > >>>>>>> > >>>>>>> That becomes a new memory barrier on some archs. Rather just re-r= ead it > >>>>>>> below. Re-reading should be fine here. > >>>>>>> > >>>>>>>> do { > >>>>>>>> last =3D atomic_add_negative(-1, &page->= _mapcount); > >>>>>>>> if (last) { > >>>>>>>> @@ -1554,7 +1555,9 @@ static __always_inline void __folio_remove= _rmap(struct folio *folio, > >>>>>>>> * is still mapped. > >>>>>>>> */ > >>>>>>>> if (folio_test_large(folio) && folio_test_anon(f= olio)) > >>>>>>>> - if (level =3D=3D RMAP_LEVEL_PTE || nr < nr= _pmdmapped) > >>>>>>>> + if ((level =3D=3D RMAP_LEVEL_PTE && > >>>>>>>> + mapcount !=3D 0) || > >>>>>>>> + (level =3D=3D RMAP_LEVEL_PMD && nr < n= r_pmdmapped)) > >>>>>>>> deferred_split_folio(folio); > >>>>>>>> } > >>>>>>> > >>>>>>> But I do wonder if we really care? Usually the folio will simply = get > >>>>>>> freed afterwards, where we simply remove it from the list. > >>>>>>> > >>>>>>> If it's pinned, we won't be able to free or reclaim, but it's rat= her a > >>>>>>> corner case ... > >>>>>>> > >>>>>>> Is it really worth the added code? Not convinced. > >>>>>> > >>>>>> It is actually not only an optimization, but also fixed the broken > >>>>>> thp_deferred_split_page counter in /proc/vmstat. > >>>>>> > >>>>>> The counter actually counted the partially unmapped huge pages (so > >>>>>> they are on deferred split queue), but it counts the fully unmappe= d > >>>>>> mTHP as well now. For example, when a 64K THP is fully unmapped, t= he > >>>>>> thp_deferred_split_page is not supposed to get inc'ed, but it does > >>>>>> now. > >>>>>> > >>>>>> The counter is also useful for performance analysis, for example, > >>>>>> whether a workload did a lot of partial unmap or not. So fixing th= e > >>>>>> counter seems worthy. Zi Yan should have mentioned this in the com= mit > >>>>>> log. > >>>>> > >>>>> Yes, all that is information that is missing from the patch descrip= tion. > >>>>> If it's a fix, there should be a "Fixes:". > >>>>> > >>>>> Likely we want to have a folio_large_mapcount() check in the code b= elow. > >>>>> (I yet have to digest the condition where this happens -- can we ha= ve an > >>>>> example where we'd use to do the wrong thing and now would do the r= ight > >>>>> thing as well?) > >>>> > >>>> For example, map 1G memory with 64K mTHP, then unmap the whole 1G or > >>>> some full 64K areas, you will see thp_deferred_split_page increased, > >>>> but it shouldn't. > >>>> > >>>> It looks __folio_remove_rmap() incorrectly detected whether the mTHP > >>>> is fully unmapped or partially unmapped by comparing the number of > >>>> still-mapped subpages to ENTIRELY_MAPPED, which should just work for > >>>> PMD-mappable THP. > >>>> > >>>> However I just realized this problem was kind of workaround'ed by co= mmit: > >>>> > >>>> commit 98046944a1597f3a02b792dbe9665e9943b77f28 > >>>> Author: Baolin Wang > >>>> Date: Fri Mar 29 14:59:33 2024 +0800 > >>>> > >>>> mm: huge_memory: add the missing folio_test_pmd_mappable() for = THP > >>>> split statistics > >>>> > >>>> Now the mTHP can also be split or added into the deferred list,= so add > >>>> folio_test_pmd_mappable() validation for PMD mapped THP, to avo= id > >>>> confusion with PMD mapped THP related statistics. > >>>> > >>>> Link: https://lkml.kernel.org/r/a5341defeef27c9ac7b85c97f030f93= e4368bbc1.1711694852.git.baolin.wang@linux.alibaba.com > >>>> Signed-off-by: Baolin Wang > >>>> Acked-by: David Hildenbrand > >>>> Cc: Muchun Song > >>>> Signed-off-by: Andrew Morton > >>>> > >>>> This commit made thp_deferred_split_page didn't count mTHP anymore, = it > >>>> also made thp_split_page didn't count mTHP anymore. > >>>> > >>>> However Zi Yan's patch does make the code more robust and we don't > >>>> need to worry about the miscounting issue anymore if we will add > >>>> deferred_split_page and split_page counters for mTHP in the future. > >>> > >>> Actually, the patch above does not fix everything. A fully unmapped > >>> PTE-mapped order-9 THP is also added to deferred split list and > >>> counted as THP_DEFERRED_SPLIT_PAGE without my patch, since nr is 512 > >>> (non zero), level is RMAP_LEVEL_PTE, and inside deferred_split_folio(= ) > >>> the order-9 folio is folio_test_pmd_mappable(). > >>> > >>> I will add this information in the next version. > >> > >> It might > >> Fixes: b06dc281aa99 ("mm/rmap: introduce folio_remove_rmap_[pte|ptes|p= md]()"), > >> but before this commit fully unmapping a PTE-mapped order-9 THP still = increased > >> THP_DEFERRED_SPLIT_PAGE, because PTEs are unmapped individually and fi= rst PTE > >> unmapping adds the THP into the deferred split list. This means commit= b06dc281aa99 > >> did not change anything and before that THP_DEFERRED_SPLIT_PAGE increa= se is > >> due to implementation. I will add this to the commit log as well witho= ut Fixes > >> tag. > > > > Thanks for digging deeper. The problem may be not that obvious before > > mTHP because PMD-mappable THP is converted to PTE-mapped due to > > partial unmap in most cases. But mTHP is always PTE-mapped in the > > first place. The other reason is batched rmap remove was not supported > > before David's optimization. > > Yes. > > > > > Now we do have reasonable motivation to make it precise and it is also > > easier to do so than before. > > If by "precise" you mean "less unreliable in some cases", yes. See my > other mail. Yes, definitely. Make the unreliability somehow acceptable. > > -- > Cheers, > > David / dhildenb >