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 4FE56C48297 for ; Wed, 7 Feb 2024 00:43:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 790A66B0072; Tue, 6 Feb 2024 19:43:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 73FA56B0074; Tue, 6 Feb 2024 19:43:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5E0D26B0075; Tue, 6 Feb 2024 19:43:18 -0500 (EST) 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 48D206B0072 for ; Tue, 6 Feb 2024 19:43:18 -0500 (EST) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id E62611A054D for ; Wed, 7 Feb 2024 00:43:17 +0000 (UTC) X-FDA: 81763158834.04.B010080 Received: from mail-vs1-f52.google.com (mail-vs1-f52.google.com [209.85.217.52]) by imf13.hostedemail.com (Postfix) with ESMTP id 2E8E620010 for ; Wed, 7 Feb 2024 00:43:15 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=I0GDdi+i; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.52 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707266595; 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=Q7BlF+GgpfGfiq8n3mWp1hSdGSr/E+pWqazv5LlW0CM=; b=tgJRBRDPPoCZQtiiKh/LJEw4QEI/kOYFDsJCBLcFnbxNaqvFzOBis0shBnjdrCZOVM5Q+j KnAGIiWAyEGsmrTQsYn3iVtzSL4j7rVnoZLAVztcynXsoMzRajxV95iWnij5PreVLxoYeJ D7f3MYc5jWTQnPVEw7pS85JjB3pzWAA= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=I0GDdi+i; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.52 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707266595; a=rsa-sha256; cv=none; b=S4zCJZxb7+rqJWdBoQE1ibar72xo6Y7cHxwN6uJVgFaheBzdrvWwiz29vDPc+oA/vh94y4 FsB032FrE2hibylC6QSILUg8OlvI9dEREFb4prWKhgXOGLOwPavmWKleucOpvqq0m6Rhd7 ytsf7/UCeMQ73H/71snlUBADCtQ4ywU= Received: by mail-vs1-f52.google.com with SMTP id ada2fe7eead31-467ed334c40so18480137.0 for ; Tue, 06 Feb 2024 16:43:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707266594; x=1707871394; 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=Q7BlF+GgpfGfiq8n3mWp1hSdGSr/E+pWqazv5LlW0CM=; b=I0GDdi+iywvNlBN83pDfxksB/OLD0QwzhDbeyi0T7fI86z5JibX04D0ivFEXmqAMEA eHhIlg4gw0AmsbOiHGXcqRb78VFTxY7EMpwwlmcbPOGyyX1VlL+Y+vkZAMoQs8Clh9mb Vw53LO9qm+6JTxGGowGAiBfWu2XAXAK64wuUJdcphwf5AZQF7J9o4rju6ZP8wDO6otE5 2fSvxftZdUU2aN3OrxG2qh4xjOmIh2fJPAv8aQfaczI6Khislh+mrTlnNKzQ6LBzCf3P /yE2UvlQgTVT2qbx7RAT/53oPcCWBRTN8qvWUaaaf+ScjHqvsF4GoXddrAE5VsVWcbDh 0kQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707266594; x=1707871394; 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=Q7BlF+GgpfGfiq8n3mWp1hSdGSr/E+pWqazv5LlW0CM=; b=TuwYcSgyY/Xu1FuYtvz6hOOwI2XVGRxWxN4dj7FKqw4+RPMZXFdcVqKVmr1oqlezIq cI1pCIUfGGa3oU+kbdbcAvYeRpJlZ7kSlzXXORDRxSKJK8Y3AgbOK3pM0RJw1cSTDf58 OB2o6X75aDNwbvqXY0JbHFtwKeTQnqeyQCn4GUbzILGTq/a9CpOIwk5h/9aCO0zJjvYZ 04xbOgSpT3+lYaLgVKn/2WAVdawOiL0n2ad4uRFyLoCLos13mQxESX+uWvgSxLsBUD+y 7HbBkfHRHfEAP2HdGeAH3uodliN81Dm01bh1AAtAJWkmnDEo4JW0pxukoYeQh3RkDsCd /nsQ== X-Gm-Message-State: AOJu0Yz9y9fCvnSS1iZlEHTUY95FFUfU9TeUDF6r5A0PeQAuQ8VSazZm rfFvzqUXEBeMxJG97umyDaOA6t6Tm6HUUfUcAsLDfuyy9IHuhkV3DnmJe77Y+AuXC/9tlarJ54t uPB+203tuP6lW6AGI3RdhTW6MrzE= X-Google-Smtp-Source: AGHT+IGJkE627H4rO2JNjClu/fwx4Ssfk4RqtF9rAJRQKAVFROsEkgrINIirAKjLLstXR6y02rqNsV+OK0YASBkiUp0= X-Received: by 2002:a05:6102:4425:b0:46d:2c97:71cf with SMTP id df37-20020a056102442500b0046d2c9771cfmr1642896vsb.10.1707266594145; Tue, 06 Feb 2024 16:43:14 -0800 (PST) MIME-Version: 1.0 References: <20240206182559.32264-1-ryncsn@gmail.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Wed, 7 Feb 2024 07:40:12 +0800 Message-ID: Subject: Re: [PATCH v2] mm/swap: fix race when skipping swapcache To: Chris Li 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-Rspamd-Queue-Id: 2E8E620010 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: p6m5fnhmkhpiym86kyahffwsofxn4msq X-HE-Tag: 1707266595-237482 X-HE-Meta: U2FsdGVkX1/0OxcDFzynVgdcGJmspxcgeC5iPBVGOlreiCunzzHyYwfNT6u8Ot+4hv45OD2+4CRWOSgreDkHEMRP1o+hSjfyldTxr+PlZcHGQGoqaFpMRq+1TWOZ0aoRkIIvRdk455yEJtQDgXTO2hPeBuJfsQatDz+0spc/nXFsHAna45fZliCFu/9APgLHgL70Cwctf3Z5iKL9EPMpLOqyNa4K+U5NDtbRTAvZR3ygbr/rcULxUHLgvhdGFZdxVNVKjJ+lS7I9LhUb1ckOHGJBtI5iJ25R88APbmwx9XrwGd7ushRyeW4vxZEI+Y6uBzFVo+oUu1Mp8cXP2B63Pj9CgoWJia8ik53jvf1YHUcVP53/ayHg/Ylkrzx8wzE8vxEkA/5GpxPSz7UQXPRewrRdxRFPTAaklMAClClfGuEuTmCg/oGmbiWFiKMayzg1Cpx2Gyq44fidNd2t07OQ0bOK1UXsnJ67q7FkzrPNMLlDf21kFBN0aYEe7zInX/mv5hKn/LdULZKAbMthQrb3oJVJDkxZ2L+jY3a7kp+fF/xTrY6vGGN6iasaXptYKMvG11omMAgWONta77vxvwVFhhgk+JyifzLOKwh6iqn4dWTbqpWrrgBkYs5Os/IzDiFbRpSIGEIf6BusAOEa2zWiA4WaFpHuEIJzKwgH+dal1wJ42johpI7jodpsj293XZYiGQZcJab5KbNdgvPxQxk0O0Ym9GhLyRXaYeoKagmd900zGHoDWFfOSBjOBEyEcvXrlkp0yHSsf6QZcIBgW8qwyPfV0eFRqFERDZ1RzJk09yIloED1Cvvw6HqQ1QqCU9zPQpVwhWYD2AjJs/iRUVu16GodTGyhCZo4HLTYoUTQhf/KP/gO+9xXlAWB++WoCgIbGx608jv2DT0t2QUBBreuhqEgUPimXSOrDjrF9BG4KN6bNaLpw6djyQG8ttYDW5++t+qAiiiZ+yujIrsPOXD QMBkA/f2 Mbqo6MMRtdHupiT8x6fdBfirm4ctRhU4Mk/M1gwPUnV0A58MtpsslqQs6A/9jJWWNq8G1gcDfMMwJzyPYN7beRjyTq55xzp4Y4vqjgUS26SyLIQq1Ob5pWzb5POPVzga1PFtshAtTkiMRDRjz6V7IoRTFmjgjTv34YDT/bJ6BQkpTUyWdbhBTo1pQcww95YuriIOjVAIZOGmZYUoL9iVuPIk+cNTJJptLAzEscGw/jnTP8BtdNjxXkmc66j8pnTmi0HyF9OoSTgR9XLx/90zcQKcW4lPGhHL4IthAFS9nzWgnswXyzbfcgtotCKuVZuuRpyGCNGCylXBclsT92xLLEbQJKi/XsP+YUGnGwIoR9LiSmtlGRPgiPXZ1sRfcYF6SMQAXVFncJQYxCBIoUYVnHQnFe1ROMVZAJxWgYzk76pnqqbjBznfKJEk2jeuVUE0MSkK/qR9D6a0qGlOGEsjAcBGXv6Z5VWNvQfSfUx2vGe6VK85xvrdcbqif7pKXfUsO0M1pC0E2Bbau3DepmtPJ0ORfJ5XCkg52sPcYjUzy+M1T7hdYbUyHw1ZspK+l5O0NAF9yE4qaYCimXiRuFi2jYM3k+A== 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 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 wr= ote: > > > > From: Kairui Song > > > > When skipping swapcache for SWP_SYNCHRONOUS_IO, if two or more threads > > 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 page = 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 usin= g > > 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 easily: > > $ 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 region > > using a small swap device. Every two threads updates mapped pages one b= y > > 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 synchronou= s device") > > Reported-by: "Huang, Ying" > > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yhuang6-desk2.cc= r.corp.intel.com/ > > Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-stres= s-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_fre= e 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 Pa= rk] > > - Update the commit message and summary, refer to SWP_SYNCHRONOUS_IO in= stead 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 with > > + * the cache flag. Otherwise, another thread ma= y > > + * finish swapin first, free the entry, and swa= pout > > + * 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. is it possible to loop for the success of swapcache_prepare(entry) instead? > > This behavior needs more discussion. > > Chris > > > Chris > > > > /* skip swapcache */ > > folio =3D vma_alloc_folio(GFP_HIGHUSER_MOVABLE,= 0, > > vma, vmf->address, fals= e); > > @@ -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 unlock = */ > > + 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, str= uct writeback_control *wbc) > > return 0; > > } > > > > +static inline void swapcache_clear(struct swap_info_struct *si, swp_en= try_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