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 D19E0C5478C for ; Wed, 28 Feb 2024 01:23:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1A3546B0251; Tue, 27 Feb 2024 20:23:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 153C46B0254; Tue, 27 Feb 2024 20:23:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F360B6B0255; Tue, 27 Feb 2024 20:23:39 -0500 (EST) 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 E4CE96B0251 for ; Tue, 27 Feb 2024 20:23:39 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 90B261C10C0 for ; Wed, 28 Feb 2024 01:23:39 +0000 (UTC) X-FDA: 81839465358.14.7EA461C Received: from mail-ua1-f52.google.com (mail-ua1-f52.google.com [209.85.222.52]) by imf08.hostedemail.com (Postfix) with ESMTP id D15A416000F for ; Wed, 28 Feb 2024 01:23:37 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=QKvnwmzF; spf=pass (imf08.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.52 as permitted sender) smtp.mailfrom=21cnbao@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=1709083417; 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=QY52hhOkWrTeKEaam0U/0mVI5iX9IF4eoJj+VESfF20=; b=WfOLmnC6GV0mMmN6YzpDxGRhoRgptpbgYGTZIxY9ZCpwdhnMnHqJ0Ra0uzL3QrZV8xOATY uVjYLJPZ4tjshUTprDwXZJHWWtgPjYQZ4d0YJyHL+mtAR9Ojtz06NB6tis2DxW04vLqLTY hlCpTRvn22okNdWN1Ktjg2b/dmCgW3U= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709083417; a=rsa-sha256; cv=none; b=KIAfw7KoL1+YJiV6rSffuqfri1TKWfcd1bRqLMvomFEJvp4f2UdOC8kU4Uj1kBfFYTBdV9 DWj4HiGVWJznjWtqc8lz9C6i5WVE9ou0TwGZTCq8FDR4rH/FXW9HHKy7BK0Va3sa6KNyQA tMufLocsv3WfzByvhxBJg3UP98zneao= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=QKvnwmzF; spf=pass (imf08.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.52 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ua1-f52.google.com with SMTP id a1e0cc1a2514c-7dac7cfbea0so291005241.3 for ; Tue, 27 Feb 2024 17:23:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1709083417; x=1709688217; 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=QY52hhOkWrTeKEaam0U/0mVI5iX9IF4eoJj+VESfF20=; b=QKvnwmzFSH7mgplc+eglqmajAzCKf9yPwuVpYRPCLT+L1O3qVfcxgk1Eh3fhcZ1rmG PSHWrpasqbKp0zp8uDRysSAImB9C6v8i/vYOAYkB2RUwg4hUJFn/UgEYzvQlN06D57nf 9Y0VRph7+0K5MEmUNDu5bD9axBKmM+RsyrkUbwH9EEvIgsc5EnGnN2ZJPEcjdNMKv10f pgfa1VtSdFMWgRKQ4rpPLNyDgsmtyv6BkjHvshU68mp9dNHTo0vGzWFSp5c7tW6iE9QV WPdMn0oF8UPS2t9qja9bwXLO4DyJQP6FPj6UD0AmAhy2MWlciapL05mE61ynIPhsMVmK DLEw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709083417; x=1709688217; 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=QY52hhOkWrTeKEaam0U/0mVI5iX9IF4eoJj+VESfF20=; b=kxexf06QqKfRwiPylzhZ5Y8w+0Fa/+f6ZXP2hk6Nu2u8ld28gEWpZMbhzEx5SDT69b MT4w9rk8XS69AsNBrdgHyvhTkapc/cpjB3UVjxOsCb0wVc7qt4ruKjijH6cUJc5beo6T vejzfnPlhwRImOpyf38Owr6M/2euw1SNx3FCzEySZL6R05Bb9wQ9M7s9/B9RebJgsCLo 1P81lGI5uHv8bdhhuruIyb1dspc4yTMU42N5q1vp9ZF90zGOFtZYS26ARdG2AqxuY/Gl crON7AleY5ydQEoIpmcl4R/soQwwRhxtAGxBb8dgGPPedKQAKR4Ryp0WGo3MUovXaiij 4FPg== X-Forwarded-Encrypted: i=1; AJvYcCU0dSwOqiIdNQfIayNUbYBoDxDv0s3X+3/MZcO7kuCEhKAmvvEKEnnZKdEMyvNxjioey7a2tJg9x+xn7w67Xtl03no= X-Gm-Message-State: AOJu0Yyzh2Ci/rSQrZgrnO9jGl9TTqCh7nxcylq/F9KEPhNAZDvTn4UT nyRAb2ROaxdMg9vun5Ik2K1IfroBlzVHC2S0SGLUBM6hhxWOR6YFM+u4oqwsKU9deuQr3iAfeyl r03OWXAyugj9NSZodyWlv97m20iI= X-Google-Smtp-Source: AGHT+IGkATYCBvpBNYs60EvUtc722Cjcoy3ozBYbvmNMiNwWJe1HdmNLmXY6krlBKINsOBpP/0QzyAScHEsLOChfGw0= X-Received: by 2002:a1f:c442:0:b0:4d3:3446:6bc7 with SMTP id u63-20020a1fc442000000b004d334466bc7mr2200639vkf.14.1709083416712; Tue, 27 Feb 2024 17:23:36 -0800 (PST) MIME-Version: 1.0 References: <20231025144546.577640-5-ryan.roberts@arm.com> <20240222070544.133673-1-21cnbao@gmail.com> <1a9fcdcd-c0dd-46dd-9c03-265a6988eeb2@redhat.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Wed, 28 Feb 2024 14:23:25 +1300 Message-ID: Subject: Re: [PATCH v3 4/4] mm: swap: Swap-out small-sized THP without splitting To: Ryan Roberts Cc: David Hildenbrand , akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, mhocko@suse.com, shy828301@gmail.com, wangkefeng.wang@huawei.com, willy@infradead.org, xiang@kernel.org, ying.huang@intel.com, yuzhao@google.com, chrisl@kernel.org, surenb@google.com, hanchuanhua@oppo.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: D15A416000F X-Rspam-User: X-Stat-Signature: cb57hy35huwsoyhfwpft17dbkjihkogr X-Rspamd-Server: rspam03 X-HE-Tag: 1709083417-76589 X-HE-Meta: U2FsdGVkX1+9Kzn+tTTjOlp9OYl73enJCYVQdawMv7nYgJl/udMrbu6CMUEVPlrQVH5no9NP9gP9u0wePV18zQZTNocXb35qtZd3urj8gG+nTp/43wyfo1DhVc1KAYnWA9TANkIxB3P7SRs6MJ+gdje7NYt5zFQ+l9DPxikm9zQuXmqB2iHDq+jexFp2hW/kXgNDRvDeAfDlfklXQrcKIYqfK9xTppk0DdQr2ofxAfWFmkr3WNVOkGqj3o/rqCw/1nZfePI5YNohimwezp8ZNgcyNaIguLWblfQKzYzTp49h1prlDI0p9qQN2ALg9icLWsHM1RwQyAJd7ZRP1g5OXtLunH79cK6lN7fD5J0jgPtikDI892EEeU78loARchg355A0Po3FhCv5+69sylopgD2ZXoMlYCoYzbB/ytbMs6NmBqwgy07RL7QAfbqQPruOjHw/I0EZmIBS2iwaq6M/WLlrYB5yF3PB0DR1dNE5BQId0ZjpVo5xz/+b0qnNS16iATWOadNZCI05yV3rEPeyRkglbEx65P2HdVIuyT4JuB1vTl0EDQJCVu2JKNXg3ZUKMzeDa/z0DpD1E9gRdIHUtuus4WHCDbTR4k4GmgJFSij1laGayghgKhhqF7EkTvY84rWA236b+y5qGnf2fXta/syR4Gg6W0xX06zQX4svNWGEY7i0SsJDdfKHldEJ7FXxGsHiyCAacfE+HEmS/b/iDkn2zwTgnhMC37s7TmUHQumndns7MuEw3HlIL3bw3hBOpPZv8zjToDPeqTUFVH5WbhmDRtHfu0vA0GtKo1wqqLECHAISqmlv9kgKwxOgfWgcim6a8iu+9t/77xIqoV9Vy3B810mMSC0xxcCbCpi/Jv34vOZkScQZng+dYPSCYXiRz4mI2nEdeZsiy6JDocqlOiUxH/AApdmWnUEzFebDnys= 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 Wed, Feb 28, 2024 at 1:06=E2=80=AFAM Ryan Roberts = wrote: > > On 23/02/2024 09:46, Barry Song wrote: > > On Thu, Feb 22, 2024 at 11:09=E2=80=AFPM David Hildenbrand wrote: > >> > >> On 22.02.24 08:05, Barry Song wrote: > >>> Hi Ryan, > >>> > >>>> diff --git a/mm/vmscan.c b/mm/vmscan.c > >>>> index 2cc0cb41fb32..ea19710aa4cd 100644 > >>>> --- a/mm/vmscan.c > >>>> +++ b/mm/vmscan.c > >>>> @@ -1212,11 +1212,13 @@ static unsigned int shrink_folio_list(struct= list_head *folio_list, > >>>> if (!can_split_folio(folio, NUL= L)) > >>>> goto activate_locked; > >>>> /* > >>>> - * Split folios without a PMD m= ap right > >>>> - * away. Chances are some or al= l of the > >>>> - * tail pages can be freed with= out IO. > >>>> + * Split PMD-mappable folios wi= thout a > >>>> + * PMD map right away. Chances = are some > >>>> + * or all of the tail pages can= be freed > >>>> + * without IO. > >>>> */ > >>>> - if (!folio_entire_mapcount(foli= o) && > >>>> + if (folio_test_pmd_mappable(fol= io) && > >>>> + !folio_entire_mapcount(foli= o) && > >>>> split_folio_to_list(folio, > >>>> folio_l= ist)) > >>>> goto activate_locked; > >>> > >>> I ran a test to investigate what would happen while reclaiming a part= ially > >>> unmapped large folio. for example, for 64KiB large folios, MADV_DONTN= EED > >>> 4KB~64KB, and keep the first subpage 0~4KiB. > >> > >> IOW, something that already happens with ordinary THP already IIRC. > >> > >>> > >>> My test wants to address my three concerns, > >>> a. whether we will have leak on swap slots > >>> b. whether we will have redundant I/O > >>> c. whether we will cause races on swapcache > >>> > >>> what i have done is printing folio->_nr_pages_mapped and dumping 16 s= wap_map[] > >>> at some specific stage > >>> 1. just after add_to_swap (swap slots are allocated) > >>> 2. before and after try_to_unmap (ptes are set to swap_entry) > >>> 3. before and after pageout (also add printk in zram driver to dump a= ll I/O write) > >>> 4. before and after remove_mapping > >>> > >>> The below is the dumped info for a particular large folio, > >>> > >>> 1. after add_to_swap > >>> [ 27.267357] vmscan: After add_to_swap shrink_folio_list 1947 mapnr= :1 > >>> [ 27.267650] vmscan: offset:101b0 swp_map 40-40-40-40-40-40-40-40-4= 0-40-40-40-40-40-40-40 > >>> > >>> as you can see, > >>> _nr_pages_mapped is 1 and all 16 swap_map are SWAP_HAS_CACHE (0x40) > >>> > >>> > >>> 2. before and after try_to_unmap > >>> [ 27.268067] vmscan: before try to unmap shrink_folio_list 1991 map= nr:1 > >>> [ 27.268372] try_to_unmap_one address:ffff731f0000 pte:e8000103cd0b= 43 pte_p:ffff0000c36a8f80 > >>> [ 27.268854] vmscan: after try to unmap shrink_folio_list 1997 mapn= r:0 > >>> [ 27.269180] vmscan: offset:101b0 swp_map 41-40-40-40-40-40-40-40-4= 0-40-40-40-40-40-40-40 > >>> > >>> as you can see, one pte is set to swp_entry, and _nr_pages_mapped bec= omes > >>> 0 from 1. The 1st swp_map becomes 0x41, SWAP_HAS_CACHE + 1 > >>> > >>> 3. before and after pageout > >>> [ 27.269602] vmscan: before pageout shrink_folio_list 2065 mapnr:0 > >>> [ 27.269880] vmscan: offset:101b0 swp_map 41-40-40-40-40-40-40-40-4= 0-40-40-40-40-40-40-40 > >>> [ 27.270691] zram: zram_write_page page:fffffc00030f3400 index:101b= 0 > >>> [ 27.271061] zram: zram_write_page page:fffffc00030f3440 index:101b= 1 > >>> [ 27.271416] zram: zram_write_page page:fffffc00030f3480 index:101b= 2 > >>> [ 27.271751] zram: zram_write_page page:fffffc00030f34c0 index:101b= 3 > >>> [ 27.272046] zram: zram_write_page page:fffffc00030f3500 index:101b= 4 > >>> [ 27.272384] zram: zram_write_page page:fffffc00030f3540 index:101b= 5 > >>> [ 27.272746] zram: zram_write_page page:fffffc00030f3580 index:101b= 6 > >>> [ 27.273042] zram: zram_write_page page:fffffc00030f35c0 index:101b= 7 > >>> [ 27.273339] zram: zram_write_page page:fffffc00030f3600 index:101b= 8 > >>> [ 27.273676] zram: zram_write_page page:fffffc00030f3640 index:101b= 9 > >>> [ 27.274044] zram: zram_write_page page:fffffc00030f3680 index:101b= a > >>> [ 27.274554] zram: zram_write_page page:fffffc00030f36c0 index:101b= b > >>> [ 27.274870] zram: zram_write_page page:fffffc00030f3700 index:101b= c > >>> [ 27.275166] zram: zram_write_page page:fffffc00030f3740 index:101b= d > >>> [ 27.275463] zram: zram_write_page page:fffffc00030f3780 index:101b= e > >>> [ 27.275760] zram: zram_write_page page:fffffc00030f37c0 index:101b= f > >>> [ 27.276102] vmscan: after pageout and before needs_release shrink_= folio_list 2124 mapnr:0 > >>> > >>> as you can see, obviously, we have done redundant I/O - 16 zram_write= _page though > >>> 4~64KiB has been zap_pte_range before, we still write them to zRAM. > >>> > >>> 4. before and after remove_mapping > >>> [ 27.276428] vmscan: offset:101b0 swp_map 41-40-40-40-40-40-40-40-4= 0-40-40-40-40-40-40-40 > >>> [ 27.277485] vmscan: after remove_mapping shrink_folio_list 2169 ma= pnr:0 offset:0 > >>> [ 27.277802] vmscan: offset:101b0 01-00-00-00-00-00-00-00-00-00-00-= 00-00-00-00-00 > >>> > >>> as you can see, swp_map 1-15 becomes 0 and only the first swp_map is = 1. > >>> all SWAP_HAS_CACHE has been removed. This is perfect and there is no = swap > >>> slot leak at all! > >>> > >>> Thus, only two concerns are left for me, > >>> 1. as we don't split anyway, we have done 15 unnecessary I/O if a lar= ge folio > >>> is partially unmapped. > > So the cost of this is increased IO and swap storage, correct? Is this a = big > problem in practice? i.e. do you see a lot of partially mapped large foli= os in > your workload? (I agree the proposed fix below is simple, so I think we s= hould > do it anyway - I'm just interested in the scale of the problem). > > >>> 2. large folio is added as a whole as a swapcache covering the range = whose > >>> part has been zapped. I am not quite sure if this will cause some pro= blems > >>> while some concurrent do_anon_page, swapin and swapout occurs between= 3 and > >>> 4 on zapped subpage1~subpage15. still struggling.. my brain is explod= ing... > > Yes mine too. I would only expect the ptes that map the folio will get re= placed > with swap entries? So I would expect it to be safe. Although I understand= the > concern with the extra swap consumption. yes. it should still be safe. just more I/O and more swap spaces. but they = will be removed while remove_mapping happens if try_to_unmap_one makes the folio unmapped. but with the potential possibility even mapped PTEs can be skipped by try_to_unmap_one (reported intermediate PTEs issue - PTL is held till a valid PTE, some PTEs might be skipped by try_to_unmap without being set to swap entries), we could have the possibility folio_mapped() is still= true after try_to_unmap_one. so we can't get to __remove_mapping() for a long time. but it still doesn't cause a crash. > > [...] > >>> > >>> To me, it seems safer to split or do some other similar optimization = if we find a > >>> large folio has partial map and unmap. > >> > >> I'm hoping that we can avoid any new direct users of _nr_pages_mapped = if > >> possible. > >> > > > > Is _nr_pages_mapped < nr_pages a reasonable case to split as we > > have known the folio has at least some subpages zapped? > > I'm not sure we need this - the folio's presence on the split list will t= ell us > everything we need to know I think? I agree, this is just one question to David, not my proposal. if deferred_list is sufficient, I prefer we use deferred_list. I actually don't quite understand why David dislikes _nr_pages_mapped being= used though I do think _nr_pages_mapped cannot precisely reflect how a folio is mapped by multi-processes. but _nr_pages_mapped < nr_pages seems be safe to tell the folio is partially unmapped :-) > > > > >> If we find that the folio is on the deferred split list, we might as > >> well just split it right away, before swapping it out. That might be a > >> reasonable optimization for the case you describe. > > Yes, agreed. I think there is still chance of a race though; Some other t= hread > could be munmapping in parallel. But in that case, I think we just end up= with > the increased IO and swap storage? That's not the end of the world if its= a > corner case. I agree. btw, do we need a spinlock ds_queue->split_queue_lock for checking the list? deferred_split_folio(), for itself, has no spinlock while checkin= g if (!list_empty(&folio->_deferred_list)), but why? the read and write need to be exclusive..... void deferred_split_folio(struct folio *folio) { ... if (!list_empty(&folio->_deferred_list)) return; spin_lock_irqsave(&ds_queue->split_queue_lock, flags); if (list_empty(&folio->_deferred_list)) { count_vm_event(THP_DEFERRED_SPLIT_PAGE); list_add_tail(&folio->_deferred_list, &ds_queue->split_queu= e); ds_queue->split_queue_len++; #ifdef CONFIG_MEMCG if (memcg) set_shrinker_bit(memcg, folio_nid(folio), deferred_split_shrinker->id); #endif } spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); } > > > > > i tried to change Ryan's code as below > > > > @@ -1905,11 +1922,12 @@ static unsigned int shrink_folio_list(struct > > list_head *folio_list, > > * PMD map right away. Chances = are some > > * or all of the tail pages can= be freed > > * without IO. > > + * Similarly, split PTE-mapped = folios if > > + * they have been already > > deferred_split. > > */ > > - if (folio_test_pmd_mappable(fol= io) && > > - !folio_entire_mapcount(foli= o) && > > - split_folio_to_list(folio, > > - folio_l= ist)) > > + if > > (((folio_test_pmd_mappable(folio) && !folio_entire_mapcount(folio)) || > > + > > (!folio_test_pmd_mappable(folio) && > > !list_empty(&folio->_deferred_list))) > > I'm not sure we need the different tests for pmd_mappable vs !pmd_mappabl= e. I > think presence on the deferred list is a sufficient indicator that there = are > unmapped subpages? I don't think there are fundamental differences for pmd and pte. i was testing pte-mapped folio at that time, so kept the behavior of pmd as is. > > I'll incorporate this into my next version. Great! > > > + && > > split_folio_to_list(folio, folio_list)) > > goto activate_locked; > > } > > if (!add_to_swap(folio)) { > > > > It seems to work as expected. only one I/O is left for a large folio > > with 16 PTEs > > but 15 of them have been zapped before. > > > >> > >> -- > >> Cheers, > >> > >> David / dhildenb > >> > > Thanks Barry