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 8D583C4828D for ; Wed, 7 Feb 2024 02:03:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1DA1E6B007E; Tue, 6 Feb 2024 21:03:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 194E86B0080; Tue, 6 Feb 2024 21:03:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 02AA06B0081; Tue, 6 Feb 2024 21:03:43 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id E388E6B007E for ; Tue, 6 Feb 2024 21:03:43 -0500 (EST) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 8EA22408F9 for ; Wed, 7 Feb 2024 02:03:43 +0000 (UTC) X-FDA: 81763361526.30.D9D8EEF Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf27.hostedemail.com (Postfix) with ESMTP id DD0D34000E for ; Wed, 7 Feb 2024 02:03:40 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=ZA0kN7D1; spf=pass (imf27.hostedemail.com: domain of chrisl@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=chrisl@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707271421; 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=kXID3ve8BFUr8fqoS4pM/7WTb9UHB6gC/sWPPF57dfs=; b=5sychZ0X5p1kNNB5wqEB8Dx9+IgG/tFhaqnJtm+gS0nT4KUw3EKKQSzUP4KNeHGHLBzFV6 GwG40FJjr/QYsENqSL+fGTRtrWA0GYeaYXdcSt6LLd0v890iYtkiQzInIsb/EfHhepxxY4 SbnE9jRcEhgJ9FpOegCubX7viajTBl4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707271421; a=rsa-sha256; cv=none; b=ZWK8om37w9QRDMaO0nuySqb3Rp2IPh+cb1olszw+42gScofILRstRJtXwQwVg3O/hjtqbt PezdMefVgIezEmcjlfo0NgmHvfnG76+bNEsH8SNCX0LwtoqWzer5bDwB0fgEAqPzatBs1c d3akOvO48WtnnzFOOmatoYTeHYKsbJc= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=ZA0kN7D1; spf=pass (imf27.hostedemail.com: domain of chrisl@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=chrisl@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 68FC4CE1779 for ; Wed, 7 Feb 2024 02:03:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A2E4CC43390 for ; Wed, 7 Feb 2024 02:03:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1707271414; bh=h8ij4//8CC7WCstiqgRzmIA+sJ6imtACc24F2GyZVk8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ZA0kN7D1l7QhLiaebSujCK4W+MN65pdgxN2lyqPf2cYLw0bYONjZgDWim3KfCg+3n 0wrDID+JzBSVFgW2SK0OAyhrydG0LzlN7RNNeC7higoAHPXCoN4dP1QvPZyJa/Y7lK 1yydcvnxIgHaD32GJhgEVBy10ZTL5VTIybEyFaHgQ1sbn20tNM0agbldZjeyZw8OPy cx2aHi6+0Kh0EbzCGIB5ZJ/FA9yDhZdeXDPFQ9ibFJJ9gHhnO4ZIVQlEGXdedPM2KO U/jdts/N1Q504biP1PMPJUWUmZs0wHusg7g7E/b2ypEyPf/hbxtogozvdlYd5cQyLU 0KY4z/rlOOmZA== Received: by mail-il1-f182.google.com with SMTP id e9e14a558f8ab-363ce3a220aso497705ab.0 for ; Tue, 06 Feb 2024 18:03:34 -0800 (PST) X-Gm-Message-State: AOJu0YxwvFTAcvDsVsFi59PEn9JerEAowI4gfFvRtX0XHcTUkDMb1BmF BLEUvC1b4IQSj/uzLDtZVRWb7+kvYf9KTfDN/SFFaNTLW8h34WFLF3WfbBkILYeT23Isg6v9fST DSdUj5g9+rpLh3ZjoTA9CJMidiSw4L0K5J6fn X-Google-Smtp-Source: AGHT+IEcw+4DUz1aPhHo0gS4H2IWxJ8EnwjZr+ZkPh1FeHY5dIy0Msfxz71NYm13vimFvMlDCBFBVkrYDrd9XhlsLdA= X-Received: by 2002:a05:6e02:1541:b0:363:d96f:6850 with SMTP id j1-20020a056e02154100b00363d96f6850mr1987387ilu.12.1707271413958; Tue, 06 Feb 2024 18:03:33 -0800 (PST) MIME-Version: 1.0 References: <20240206182559.32264-1-ryncsn@gmail.com> In-Reply-To: From: Chris Li Date: Tue, 6 Feb 2024 18:03:22 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] mm/swap: fix race when skipping swapcache To: Barry Song <21cnbao@gmail.com> Cc: Kairui Song , linux-mm@kvack.org, Andrew Morton , "Huang, Ying" , Minchan Kim , Yu Zhao , Barry Song , SeongJae Park , Hugh Dickins , Johannes Weiner , Matthew Wilcox , Michal Hocko , Yosry Ahmed , David Hildenbrand , stable@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: ss4kkhwxd36nn7poxgkf1z45hzf5fj8u X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: DD0D34000E X-Rspam-User: X-HE-Tag: 1707271420-916567 X-HE-Meta: U2FsdGVkX18YlGmUgKCBn+GdMixA4YW0LdNiwDMkVAt1Pdvr6+C537yR7aCOfMD8Vo8jR1p6oU0wGuMCaHxLzWhjm/F4Dr5kxfPo4A8gQZ6/s2sBIJ4xr/nD0K2Cyw+Un6WZuPKEKgX0T5csr32v9b7RWWrovYITP+4gFFKCtDcp7eFM77BtGbcBB/XCpMs6NikKk2fDEBP2xwEST2O7ToyAf5tvUQhel2n0pW1RZhcp9uV//+KCYbitYSXr3WAQ3ZwektH2M9aT/JPt8grSasDRaTGyMmbKXMEsqDPOqaBjx6znNIPWQbC636LUbt8bPIK2CmmztQPSqQTXYRrgZTZB7EKTcBtB1Fm3LB4pbnnKjnFx76jGqOjUoByeIytGxLLyWJnBSIwoBVB4JiuwSw7gaJkb85fFRXYlxLTDcOuRm83fnlwhnD9liEWHqbHMfgU6DrBbqkRO3K9ojh3zkIAjnYMgZeZQwPGT/Xx6q6UULCGxQVGYKlBgiK8DsQrDhcaVQo2VZTYal2T8pqq+Gc8vlFeqmQMbcdpabD/V99KMad35aaKZ8vN5LVKy4fBmVp9glwU7PMmgPTkmf3ZFxmdgxaFz4LCDzJYR++wYaEJfSwwHei3j1yXeqTjrZcivAsPf1y7TMTa/CSN7AWTVaFnlXrsS9TWZyvDfgn+s2bcie+h0tdlFXPUcqPnwmOaXwRkIE/8IKlS6DO/dzaPzQvPdB0S9sKgsYsC3EMWT4/3d4L46bENJPFQiak8R+go403KzOGcJTkgmvdj7mzPukZ7ZnZGIKeziNZd6wP08vLRprMMSmGRJqL3DcQgNRb+L25TcHoLSUKOzOfbgGgWeV7E4yn7j5te4FHkG44zVRMsByNec7abAG55gngMQl/Sr7XUMEG/61unyMPEHoJPyGQQQd3cLTlp9pkLzFQS2L8nNvG0PIHZrYL+rg9ic8nwC3xvPqonsSKuJrKUZnoj XB7PjS+m wJQ6pSXEHRrFepUXoXb9XlnCpACRRhjfdbpxap1XvEOGE4ag1IqSgUdtpb1Jvbe8kiEHCCyUtpFi0exIuYOXkKILkTqZa0ncd+EpFY79VyHgWlPXzGiw34W9L0Cm9yFfwCdjMTl78YRtyP8CSVBQRyBJsZ2nAJUjpY9TR6nUUDgSEeWFOiEvy5MO0M970MoV7es8E5jO9dhAMhaO3TegqibV94B5ybIU8SDshr8fUS+2q1oyKHjVyBHcq3t9Yc2bVb+o/2tbPorhPg0q4CR4TmbcvP7+LJoG7b8D0FIQcwWvn3nzGxV2ZoUoo3Qj0JaPwwBcWAMRVmlJYgfG08VhEG0f2WjfUxYYq61JElgLCQssjvj/dCd32XcZD6V6zS9xurRNETg/fYESy+pEMO1R92Mixy99KHwXB86BuYMPDSclz3tQ= 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 Tue, Feb 6, 2024 at 4:43=E2=80=AFPM Barry Song <21cnbao@gmail.com> wrote= : > > On Wed, Feb 7, 2024 at 7:18=E2=80=AFAM Chris Li wrote= : > > > > Hi Kairui, > > > > Sorry replying to your patch V1 late, I will reply on the V2 thread. > > > > On Tue, Feb 6, 2024 at 10:28=E2=80=AFAM Kairui Song = wrote: > > > > > > From: Kairui Song > > > > > > When skipping swapcache for SWP_SYNCHRONOUS_IO, if two or more thread= s > > > swapin the same entry at the same time, they get different pages (A, = B). > > > Before one thread (T0) finishes the swapin and installs page (A) > > > to the PTE, another thread (T1) could finish swapin of page (B), > > > swap_free the entry, then swap out the possibly modified page > > > reusing the same entry. It breaks the pte_same check in (T0) because > > > PTE value is unchanged, causing ABA problem. Thread (T0) will > > > install a stalled page (A) into the PTE and cause data corruption. > > > > > > One possible callstack is like this: > > > > > > CPU0 CPU1 > > > ---- ---- > > > do_swap_page() do_swap_page() with same entry > > > > > > > > > swap_read_folio() <- read to page A swap_read_folio() <- read to pag= e B > > > > > > ... set_pte_at() > > > swap_free() <- entry is free > > > > > > > > > pte_same() <- Check pass, PTE seems > > > unchanged, but page A > > > is stalled! > > > swap_free() <- page B content lost! > > > set_pte_at() <- staled page A installed! > > > > > > And besides, for ZRAM, swap_free() allows the swap device to discard > > > the entry content, so even if page (B) is not modified, if > > > swap_read_folio() on CPU0 happens later than swap_free() on CPU1, > > > it may also cause data loss. > > > > > > To fix this, reuse swapcache_prepare which will pin the swap entry us= ing > > > the cache flag, and allow only one thread to pin it. Release the pin > > > after PT unlocked. Racers will simply busy wait since it's a rare > > > and very short event. > > > > > > Other methods like increasing the swap count don't seem to be a good > > > idea after some tests, that will cause racers to fall back to use the > > > swap cache again. Parallel swapin using different methods leads to > > > a much more complex scenario. > > > > > > Reproducer: > > > > > > This race issue can be triggered easily using a well constructed > > > reproducer and patched brd (with a delay in read path) [1]: > > > > > > With latest 6.8 mainline, race caused data loss can be observed easil= y: > > > $ gcc -g -lpthread test-thread-swap-race.c && ./a.out > > > Polulating 32MB of memory region... > > > Keep swapping out... > > > Starting round 0... > > > Spawning 65536 workers... > > > 32746 workers spawned, wait for done... > > > Round 0: Error on 0x5aa00, expected 32746, got 32743, 3 data loss! > > > Round 0: Error on 0x395200, expected 32746, got 32743, 3 data loss! > > > Round 0: Error on 0x3fd000, expected 32746, got 32737, 9 data loss! > > > Round 0 Failed, 15 data loss! > > > > > > This reproducer spawns multiple threads sharing the same memory regio= n > > > using a small swap device. Every two threads updates mapped pages one= by > > > one in opposite direction trying to create a race, with one dedicated > > > thread keep swapping out the data out using madvise. > > > > > > The reproducer created a reproduce rate of about once every 5 minutes= , > > > so the race should be totally possible in production. > > > > > > After this patch, I ran the reproducer for over a few hundred rounds > > > and no data loss observed. > > > > > > Performance overhead is minimal, microbenchmark swapin 10G from 32G > > > zram: > > > > > > Before: 10934698 us > > > After: 11157121 us > > > Non-direct: 13155355 us (Dropping SWP_SYNCHRONOUS_IO flag) > > > > > > Fixes: 0bcac06f27d7 ("mm, swap: skip swapcache for swapin of synchron= ous device") > > > Reported-by: "Huang, Ying" > > > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yhuang6-desk2.= ccr.corp.intel.com/ > > > Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-str= ess-race [1] > > > Signed-off-by: Kairui Song > > > Reviewed-by: "Huang, Ying" > > > Acked-by: Yu Zhao > > > > > > --- > > > Update from V1: > > > - Add some words on ZRAM case, it will discard swap content on swap_f= ree so the race window is a bit different but cure is the same. [Barry Song= ] > > > - Update comments make it cleaner [Huang, Ying] > > > - Add a function place holder to fix CONFIG_SWAP=3Dn built [SeongJae = Park] > > > - Update the commit message and summary, refer to SWP_SYNCHRONOUS_IO = instead of "direct swapin path" [Yu Zhao] > > > - Update commit message. > > > - Collect Review and Acks. > > > > > > include/linux/swap.h | 5 +++++ > > > mm/memory.c | 15 +++++++++++++++ > > > mm/swap.h | 5 +++++ > > > mm/swapfile.c | 13 +++++++++++++ > > > 4 files changed, 38 insertions(+) > > > > > > diff --git a/include/linux/swap.h b/include/linux/swap.h > > > index 4db00ddad261..8d28f6091a32 100644 > > > --- a/include/linux/swap.h > > > +++ b/include/linux/swap.h > > > @@ -549,6 +549,11 @@ static inline int swap_duplicate(swp_entry_t swp= ) > > > return 0; > > > } > > > > > > +static inline int swapcache_prepare(swp_entry_t swp) > > > +{ > > > + return 0; > > > +} > > > + > > > static inline void swap_free(swp_entry_t swp) > > > { > > > } > > > diff --git a/mm/memory.c b/mm/memory.c > > > index 7e1f4849463a..1749c700823d 100644 > > > --- a/mm/memory.c > > > +++ b/mm/memory.c > > > @@ -3867,6 +3867,16 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > > if (!folio) { > > > if (data_race(si->flags & SWP_SYNCHRONOUS_IO) && > > > __swap_count(entry) =3D=3D 1) { > > > + /* > > > + * Prevent parallel swapin from proceeding wi= th > > > + * the cache flag. Otherwise, another thread = may > > > + * finish swapin first, free the entry, and s= wapout > > > + * reusing the same entry. It's undetectable = as > > > + * pte_same() returns true due to entry reuse= . > > > + */ > > > + if (swapcache_prepare(entry)) > > > + goto out; > > > + > > > > I am puzzled by this "goto out". If I understand this correctly, you > > have two threads CPU1 and CPU2 racing to set the flag SWAP_HAS_CACHE. > > The CPU1 will succeed in adding the flag and the CPU2 will get > > "-EEXIST" from "swapcache_prepare(entry)". Am I understanding it > > correctly so far? > > > > Then the goto out seems wrong to me. For the CPU2, the page fault will > > return *unhandled*. Even worse, the "-EEXIST" error is not preserved, > > CPU2 does not even know the page fault is not handled, it will resume > > from the page fault instruction, possibly generate another page fault > > at the exact same location. That page fault loop will repeat until > > CPU1 install the new pte on that faulting virtual address and pick up > > by CPU2. > > > > Am I missing something obvious there? > > I feel you are right. any concurrent page faults at the same pte > will increase the count of page faults for a couple of times now. > > > > > I just re-read your comment: "Racers will simply busy wait since it's > > a rare and very short event." That might be referring to the above > > CPU2 page fault looping situation. I consider the page fault looping > > on CPU2 not acceptable. For one it will mess up the page fault > > statistics. > > In my mind, having an explicit loop for CPU2 waiting for the PTE to > > show up is still better than this page fault loop. You can have more > > CPU power friendly loops. > > I assume you mean something like > > while(!pte_same()) > cpu_relax(); > > then we still have a chance to miss the change of B. > > For example, another thread is changing pte to A->B->A, our loop can > miss B. Thus we will trap into an infinite loop. this is even worse. Yes. You are right, it is worse. Thanks for catching that. That is why I say this needs more discussion, I haven't fully thought it through :-) > > is it possible to loop for the success of swapcache_prepare(entry) > instead? It looks like it can work. It is using the SWAP_HAS_CACHE like a per swap entry spin lock. Chris > > > > > This behavior needs more discussion. > > > > Chris > > > > > > Chris > > > > > > > /* skip swapcache */ > > > folio =3D vma_alloc_folio(GFP_HIGHUSER_MOVABL= E, 0, > > > vma, vmf->address, fa= lse); > > > @@ -4116,6 +4126,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > > unlock: > > > if (vmf->pte) > > > pte_unmap_unlock(vmf->pte, vmf->ptl); > > > + /* Clear the swap cache pin for direct swapin after PTL unloc= k */ > > > + if (folio && !swapcache) > > > + swapcache_clear(si, entry); > > > out: > > > if (si) > > > put_swap_device(si); > > > @@ -4124,6 +4137,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > > if (vmf->pte) > > > pte_unmap_unlock(vmf->pte, vmf->ptl); > > > out_page: > > > + if (!swapcache) > > > + swapcache_clear(si, entry); > > > folio_unlock(folio); > > > out_release: > > > folio_put(folio); > > > diff --git a/mm/swap.h b/mm/swap.h > > > index 758c46ca671e..fc2f6ade7f80 100644 > > > --- a/mm/swap.h > > > +++ b/mm/swap.h > > > @@ -41,6 +41,7 @@ void __delete_from_swap_cache(struct folio *folio, > > > void delete_from_swap_cache(struct folio *folio); > > > void clear_shadow_from_swap_cache(int type, unsigned long begin, > > > unsigned long end); > > > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry)= ; > > > struct folio *swap_cache_get_folio(swp_entry_t entry, > > > struct vm_area_struct *vma, unsigned long addr); > > > struct folio *filemap_get_incore_folio(struct address_space *mapping= , > > > @@ -97,6 +98,10 @@ static inline int swap_writepage(struct page *p, s= truct writeback_control *wbc) > > > return 0; > > > } > > > > > > +static inline void swapcache_clear(struct swap_info_struct *si, swp_= entry_t entry) > > > +{ > > > +} > > > + > > > static inline struct folio *swap_cache_get_folio(swp_entry_t entry, > > > struct vm_area_struct *vma, unsigned long addr) > > > { > > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > > index 556ff7347d5f..746aa9da5302 100644 > > > --- a/mm/swapfile.c > > > +++ b/mm/swapfile.c > > > @@ -3365,6 +3365,19 @@ int swapcache_prepare(swp_entry_t entry) > > > return __swap_duplicate(entry, SWAP_HAS_CACHE); > > > } > > > > > > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry) > > > +{ > > > + struct swap_cluster_info *ci; > > > + unsigned long offset =3D swp_offset(entry); > > > + unsigned char usage; > > > + > > > + ci =3D lock_cluster_or_swap_info(si, offset); > > > + usage =3D __swap_entry_free_locked(si, offset, SWAP_HAS_CACHE= ); > > > + unlock_cluster_or_swap_info(si, ci); > > > + if (!usage) > > > + free_swap_slot(entry); > > > +} > > > + > > > struct swap_info_struct *swp_swap_info(swp_entry_t entry) > > > { > > > return swap_type_to_swap_info(swp_type(entry)); > > > -- > > > 2.43.0 > > Thanks > Barry >