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 717C3C83F3D for ; Thu, 31 Aug 2023 19:58:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8D3FD8D0020; Thu, 31 Aug 2023 15:58:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 85D338D0002; Thu, 31 Aug 2023 15:58:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6FEB18D0020; Thu, 31 Aug 2023 15:58:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 5E3A88D0002 for ; Thu, 31 Aug 2023 15:58:22 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 2920B1603A9 for ; Thu, 31 Aug 2023 19:58:22 +0000 (UTC) X-FDA: 81185461644.29.FEF25DD Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf23.hostedemail.com (Postfix) with ESMTP id 558DA140026 for ; Thu, 31 Aug 2023 19:58:19 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf23.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1693511899; 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; bh=mB6d5BuiLX0vllY/12L2pMQ5+y5EXz4etRfhzU5fNrE=; b=xKRDlZ7jWCSer3cgy09e3QrQVejHU3JMlf9o3B2QcNXWm6AvE64A21wky58Ag/Qob7mtlU 8X+zs22eP98WS+B+A06aLl1t0PlZu5VNDGqCfhxZlN3eGB2kpT+Idmi7yz1vfx4nNWJv6X WDvbxc21sPyEaANOIiU7bwzDkWSpkGI= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf23.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1693511899; a=rsa-sha256; cv=none; b=41CaGVyoChjlzGlGeKRMaUqofPM5JrFiu6DFs6UkeE+6GLMPr6c+UT57jzR2xhlLWuragf 8NOE6KH2Z9iy7Lf5lK+tEbtgASarC5YKMr2HPSWQkagoHS5nJKqISqrRpqm/t/lsh/9zZa Ku9I/TBnBVRaPp1Ix4i2rWq382DAaXw= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 577E4C15; Thu, 31 Aug 2023 12:58:57 -0700 (PDT) Received: from [10.57.65.16] (unknown [10.57.65.16]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 837C53FBD2; Thu, 31 Aug 2023 12:57:57 -0700 (PDT) Message-ID: <31aa40aa-1471-44ca-9cc8-3f5476fbde7e@arm.com> Date: Thu, 31 Aug 2023 20:57:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 4/5] mm: Refector release_pages() Content-Language: en-GB To: Matthew Wilcox Cc: Will Deacon , "Aneesh Kumar K.V" , Andrew Morton , Nick Piggin , Peter Zijlstra , Christian Borntraeger , Sven Schnelle , Arnd Bergmann , David Hildenbrand , Yu Zhao , "Kirill A. Shutemov" , Yin Fengwei , Yang Shi , "Huang, Ying" , Zi Yan , linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20230830095011.1228673-1-ryan.roberts@arm.com> <20230830095011.1228673-5-ryan.roberts@arm.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 558DA140026 X-Stat-Signature: xbmzydkc3f9fz19i3yrcrdd9uithnjfz X-Rspam-User: X-HE-Tag: 1693511899-823472 X-HE-Meta: U2FsdGVkX1+lDksPSnhMvoTi6XbT1H+whUsETmVLJExvTgv/kRe7ljEsFMrGOdnr023FYunegiv9yebUT5n4W812lcU6ZleJcGKRnm5gedTigvbta6G3UwNdZEIjyqX/cyMOxS5lFoDhA6Yp2LLnhggjngWeNMzTAc3sE+CgKGp9ltGvq1FzXYIGEr6Zz7TOglWR7GDHgxRb54uAQip6den0vufLe11cm5ba80Fl0yTWE88BDLZXAiEuXmVuM2gF9jZ+bFlgZDIWfpIHqc7opXwQveGz4BY6z7I2KPl4GsJCr3J1zQZ65z2lMJt/vASCh3Mu53adymjDZLwEsm3R2aN5SX/0tOjhv9ogrvThXFd9cNAgL9c0W6XIsnFLLL0xZeQDCc44mkN5Ny0Thu50iik6+YL+ORbBi0eMUovkKG1qL7R1D9pd2/xwfSgb5lK3rfKNGwum6otGVdkUKflUS0T2gDD7H3SqXOivkTlwr3oDKCgjzTKqhIpmJudfX4dHoW/zKUWDBkNgnW9Bs2WDC0UQTqAAKOczLNqZocALT+WVuPZ4U3M1L2nP5aSzzVee2lJvpCptIoaQ3bTjtTpeAmHDLBfySLTocEP9pE/6av5+SlBOdqaHGbFP3+Veq8I2AlDwzHQoV/pfYJ686vLwRU55lo5LANimHRAShgMt6Zejng7HIjO8xoxx3P5h2U8VqdyZq22wh3L1DWbcyFhpBmliz5IplfClMAcY79MvAIV1FalAR5bypQXJpY8y5Ng+frHoL8K7GZR9WfOYS8iiuthT+Ef4YJgTAcQ+SLOOXMQP66K/IhpERPSUX8T1CnwLJ0QCaSS4+mvTUdHlXzWIkAtskWE1+Q4a2t9dJLiLzlMRwQvw7o7i6OHp7Gd4GfRkbGDpGtY3/eCl7ZHSgs9+JNDPV9GKPE7najBM4PMMzCqVHCcrqCJtHMnZZTx9xY1wZKsBVQi9/+flFj0HOiF SeNQpKqL VFT4dl/DCzEmbDeYia1iPNdTPQ20zzGrzaEH8EbJMKlYGrD1LaFtK89vuzI/svmmiioOWi88CJl3fHJlAIin0HMFz4rI32fK7qmBrPta+MGijxciWPyPgs9z0dmfgNXrTdAE96jEhsEqH0irIfiNBLhzl8O/2+0mx/OOFocLDSMcGZKBeQiJDAE+3T3gjVTF72fGr 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 30/08/2023 20:11, Matthew Wilcox wrote: > On Wed, Aug 30, 2023 at 10:50:10AM +0100, Ryan Roberts wrote: >> In preparation for implementing folios_put_refs() in the next patch, >> refactor release_pages() into a set of helper functions, which can be >> reused. The primary difference between release_pages() and >> folios_put_refs() is how they iterate over the set of folios. The >> per-folio actions are identical. > > As you noted, we have colliding patchsets. I'm not hugely happy with > how patch 4 turned out, Could you describe the issues as you see them? I'm keen not to repeat the same bad patterns in future. so I thought I'd send some addendum patches to > my RFC series that implement pfn_range_put() (maybe should have been > pfn_ranges_put()?) on top of my patch series. I think it's a bit nicer, > but not quite as nice as it could be. > > I'm thinking about doing ... > > void release_unref_folios(struct folio_batch *folios) > { > struct lruvec *lruvec = NULL; > unsigned long flags = 0; > int i; > > for (i = 0; i < folios->nr; i++) { > struct folio *folio = folios->folios[i]; > free_swap_cache(folio); Agree this can't go here - would put it in pfn_range_put(). But would not want it in folios_put() as you suggeted in the other email - that would surely change the behaviour of folios_put()? > __page_cache_release(folio, &lruvec, &flags); > } I think you would need to add: if (lruvec) unlock_page_lruvec_irqrestore(lruvec, flags); > mem_cgroup_uncharge_folios(folios); > free_unref_folios(folios); > } > > then this becomes: > > void folios_put(struct folio_batch *folios) > { > int i, j; > > for (i = 0, j = 0; i < folios->nr; i++) { > struct folio *folio = folios->folios[i]; > > if (is_huge_zero_page(&folio->page)) > continue; > if (folio_is_zone_device(folio)) { > if (put_devmap_managed_page(&folio->page)) > continue; > if (folio_put_testzero(folio)) > free_zone_device_page(&folio->page); > continue; > } > > if (!folio_put_testzero(folio)) > continue; > if (folio_test_hugetlb(folio)) { > free_huge_folio(folio); > continue; > } > > if (j != i) > folios->folios[j] = folio; > j++; > } > > folios->nr = j; > if (!j) > return; > release_unref_folios(folios); > } > > and pfn_range_put() also becomes shorter and loses all the lruvec work. something like this? void pfn_range_put(struct pfn_range *range, unsigned int nr) { struct folio_batch folios; unsigned int i; folio_batch_init(&folios); for (i = 0; i < nr; i++) { struct folio *folio = pfn_folio(range[i].start); unsigned int refs = range[i].end - range[i].start; free_swap_cache(folio); // <<<<< HERE? To be equivalent to // free_pages_and_swap_cache() if (is_huge_zero_page(&folio->page)) continue; if (folio_is_zone_device(folio)) { if (put_devmap_managed_page_refs(&folio->page, refs)) continue; if (folio_ref_sub_and_test(folio, refs)) free_zone_device_page(&folio->page); continue; } if (!folio_ref_sub_and_test(folio, refs)) continue; /* hugetlb has its own memcg */ if (folio_test_hugetlb(folio)) { free_huge_folio(folio); continue; } if (folio_batch_add(&folios, folio) == 0) release_unref_folios(&folios); } if (folios.nr) release_unref_folios(&folios); } > > Thoughts? Looks like its getting there to me. Although the bulk of the logic inside the loop is still common between folios_put() and pfn_range_put(); perhaps we can have a common helper for that, which would just need to take (folio, refs)? So by adding free_swap_cache() above, we can call pfn_range_put() direct and can remove free_pages_and_swap_cache() entirely. What's the best way to move forward here? Two options as I see it: - I drop patch 4 and create a version of pfn_range_put() that has the same semantic as above but essentially just copies the old release_pages() (similar to my v1). That keeps my series independent. Then you can replace with the new pfn_range_put() as part of your series. - We can just hook my patches up to the end of your series and do it all in one go. Opinion? Thanks, Ryan