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 5DE71C4828D for ; Wed, 7 Feb 2024 03:46:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E08506B0088; Tue, 6 Feb 2024 22:46:52 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DB5D26B0089; Tue, 6 Feb 2024 22:46:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C2E8E6B008A; Tue, 6 Feb 2024 22:46:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id ACDF46B0088 for ; Tue, 6 Feb 2024 22:46:52 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 776BE120187 for ; Wed, 7 Feb 2024 03:46:52 +0000 (UTC) X-FDA: 81763621464.01.0EED2C6 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.7]) by imf14.hostedemail.com (Postfix) with ESMTP id C0713100016 for ; Wed, 7 Feb 2024 03:46:49 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b="j/N5IXlX"; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf14.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.7 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707277610; 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=h6k3zOd111VHDAASzf352UFgd6t3mh6ND8E5Z/+8puc=; b=xc8xNztGQwwfkmMLSJay6KepkF0JAIgL5ZQpf/Oa4LrLxaBpud4kx8B6KIF+R8CGbYLQhT sAeDbU1qqZskm8J0exFy39HkkLmgcdpN7V2S+kEqcalAb9+ZxThx4p8EKemkJRxAJW11ki 1AHsmSjsZq9S3kE49l+yysrCmB3BB+o= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b="j/N5IXlX"; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf14.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.7 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707277610; a=rsa-sha256; cv=none; b=1oPIhsg3xEet4XOBNZ6S9Hxx/7KO+8HcAcS15RkWd9g1pSFe8gU7nAzA1HQiOWojbN/bVN IrMrASYtyDTS2xs7/dQfKRuRCil6os1HVtseHAojtmm9NpnF5S4ARq6YrS3CP47GGVO/CC 9Ar2IbA9pT3rfWEkRqZvMlpKLCScQoA= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1707277610; x=1738813610; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=ObRmqVrEK+EhDm75qsgUM33Fyg8jeh2C+NocA5gkrrg=; b=j/N5IXlXu+U6XwRSK/jPh2YkpZBW9CO2IBKKvSLVaFSJSrICshyagMAI sR0WNhytb5uz/DTF8a75srTSyGVbs1syIFSJDEXLD60rDR6UmIN+aaFt2 dwDq9KnMmAR+A2nRnPG50XdiBVO8r8RXz61qKH3CiBAsGO7G3faso0URZ cjHEyvla6lEwAeS22mLIshM206s+gleWfQ+8Ahux//mTHqB96PIl2/wii 7kj8j91XbpKSP0M3klyjrWxvVOGE8R4sOsjX88t9I/WJ6XwwJW/EJJZYw NskBPlEHl5S/wfBQN6YuhVCwQ6dCIC9emfKskKHrqO+mzFVyqkZ0LNZWp A==; X-IronPort-AV: E=McAfee;i="6600,9927,10976"; a="26346820" X-IronPort-AV: E=Sophos;i="6.05,250,1701158400"; d="scan'208";a="26346820" Received: from orviesa001.jf.intel.com ([10.64.159.141]) by fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Feb 2024 19:46:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.05,250,1701158400"; d="scan'208";a="38633331" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Feb 2024 19:46:44 -0800 From: "Huang, Ying" To: Kairui Song Cc: Barry Song <21cnbao@gmail.com>, Chris Li , linux-mm@kvack.org, Andrew Morton , 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 Subject: Re: [PATCH v2] mm/swap: fix race when skipping swapcache In-Reply-To: (Kairui Song's message of "Wed, 7 Feb 2024 10:28:59 +0800") References: <20240206182559.32264-1-ryncsn@gmail.com> <87sf25yqj2.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Wed, 07 Feb 2024 11:44:47 +0800 Message-ID: <878r3xym34.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: C0713100016 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 13yc9s4w9mizafci1rdqrq6eg94ftyrs X-HE-Tag: 1707277609-944239 X-HE-Meta: U2FsdGVkX19fCHh3kwU29WxHUuVtj+WL+UN3zdGLlkBU115vPnbxVCTybEV+NMhtBC5kHvNNDYwucTLFevg6aWEP4f0QuO4oDngaRpvSPHgpKmsSYmD0yLeaNYSgGCmsESHqE/a5M0cMwSBOJREUtGNKIpr4d9JntlBpPO8LmC8Wvc9r/Oww6vEdZgZdBhPwXh5oZsejVGAmkXOKR65wUBkfSNg0saxIK4+FHpziODmvAwKm74e5kp6uxzdaRGUXhY3hVxcl/ynblwjVBlfMI5gEhcXNMD8dJozeYtk75rvgiSY63mvW5S7F/lXzbKdUVNWY8FkP4lUxuc3iB2oEk77J/y8FBRjMGhv0QpNPKIv0wV01K/ZQC6PmOr7nR8VT1EYJx+s6d3yVBDueVPiaPnEtD0TSGBwm8CF0p51EtQthkUO2cdxQqBEGk0i9nbLfbT8X+BvH4HyBc+o93/gvXC0rF9HPcknrIhCIX1OCrwQVfRTshr426ihCJkdNefz6dacbl2lbkOI+cnVo5oTFsy6+fJVbaQcuOF7K8FsvfIv3Iup8nSBDs92iQ3U5gsDIQVGtsjQ3rfFNxpFYmXmZfVVXn2sCGA8sc8JgoCjhs+t/hN40xocUfZzQcUd2tP/owy2Mf9ZEGTVP9aJW/vhWfn3NqFoVw3bmwAJBULv5Qf7q+YwDXVjBhCM5X6NfGXsGmPhSNrZcgX8UrrM8Sh8NUC0IsLncfpxGJotZcfO9nVFBrSDV10aDIR7q3FDTK15gC+iMmeOlunk1exq5alY9guKJ8Oy9pOUgx6urX1Do6moVgGPu/kg40gRVztCmoI/UvaZPC0oTIYGxz9wY7bolsJdSxF2PeVHlwL+tz87DTHC4hkRH4iYS2B9PiewlZ3EKXk5Oui00Nx42YtYJFzDR2gjTlcfMplUfYPDtB1vgSO+q8Sw02Hmssyuq4hP1mRNQ98FWlkyAgxe7LCyFXmu 63xQnaat 6seeZbZaHmLSGbEJ4duAbVa++y+YUoVkO5j7bEIbxiX62yQKDORdJsTcvza9y6XGsP57dJ6+VC9O+XNBW9L/Yg64kGIpPdIUuDpyMoXlvJ85kDpupzVQLhqAp9jcCcw6HsyZY+rFhDmtYS4E1kUsz/ASRDhYrZsdXszUEevsb1mDamha4V9knzpjlmJqKtKNz01wkrLSB1lG+bJElEuRncZtb9YvlJuQ8/chSLjlUYi68R8Yj0Fi+06/DaqIQWBUC6Q7siQN50S1+BmQDadq6T28QdEluOftKtoIR11eRROWiPb3MUVQHQm5z8kkO7cTDzUksZvGYlGKZhDK7Hz8G7QbyUhlQIsO42/U6M8XvU4d5g4s= 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: Kairui Song writes: > On Wed, Feb 7, 2024 at 10:10=E2=80=AFAM Huang, Ying wrote: >> >> Barry Song <21cnbao@gmail.com> writes: >> >> > On Wed, Feb 7, 2024 at 7:18=E2=80=AFAM Chris Li wr= ote: >> >> >> >> 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 thre= ads >> >> > 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 p= age 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 = using >> >> > 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 t= he >> >> > 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 eas= ily: >> >> > $ 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 los= s! >> >> > Round 0: Error on 0x3fd000, expected 32746, got 32737, 9 data los= s! >> >> > Round 0 Failed, 15 data loss! >> >> > >> >> > This reproducer spawns multiple threads sharing the same memory reg= ion >> >> > using a small swap device. Every two threads updates mapped pages o= ne by >> >> > one in opposite direction trying to create a race, with one dedicat= ed >> >> > thread keep swapping out the data out using madvise. >> >> > >> >> > The reproducer created a reproduce rate of about once every 5 minut= es, >> >> > 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 synchr= onous device") >> >> > Reported-by: "Huang, Ying" >> >> > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yhuang6-desk= 2.ccr.corp.intel.com/ >> >> > Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-s= tress-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= _free so the race window is a bit different but cure is the same. [Barry So= ng] >> >> > - Update comments make it cleaner [Huang, Ying] >> >> > - Add a function place holder to fix CONFIG_SWAP=3Dn built [SeongJa= e Park] >> >> > - Update the commit message and summary, refer to SWP_SYNCHRONOUS_I= O 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 s= wp) >> >> > 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 threa= d may >> >> > + * finish swapin first, free the entry, and= swapout >> >> > + * reusing the same entry. It's undetectabl= e as >> >> > + * pte_same() returns true due to entry reu= se. >> >> > + */ >> >> > + 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 doesn't work too. The swap count can increase to > 1 and be put in >> swap cache for long time. >> >> Another possibility is to move swapcache_prepare() after >> vma_alloc_folio() to reduce the race window. > > Reducing the race window seems like a good way. Or maybe we can just > add a cpu_relax() so raced swapins will just slow down, and won't loop > too much time and so the side effect (counter or power consumption) > should be much smaller? cpu_relax() only makes sense in tight loop. -- Best Regards, Huang, Ying