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 12C1AC48BC4 for ; Mon, 19 Feb 2024 00:41:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 349866B0081; Sun, 18 Feb 2024 19:41:15 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2F9196B0082; Sun, 18 Feb 2024 19:41:15 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 199AA6B0083; Sun, 18 Feb 2024 19:41:15 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 09D176B0081 for ; Sun, 18 Feb 2024 19:41:15 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 52ABE801D4 for ; Mon, 19 Feb 2024 00:41:14 +0000 (UTC) X-FDA: 81806699268.23.75E4767 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.13]) by imf07.hostedemail.com (Postfix) with ESMTP id 9250F40011 for ; Mon, 19 Feb 2024 00:41:11 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=JuY3JC6l; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf07.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.13 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=1708303272; 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=rSoWNELmSC4lOKdeTFbf2NNJ8fgLmhBoF6TJjYZZmqA=; b=jMdD+NNrGQGd3vNLjy+8NCwa2+CaV28ixiF6XovHgkPSPTiQPOIUOpVyhaQqPtM8A+MzFY ndMABBzSnz+IDTRNYjcjquRqSL6yR2Rzv1d1WHotb+HsNWiWYbbPxFsIk0RzImYptHquA8 CGldMrpDe4tge5CgMQ+Lh+ly+XevutU= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=JuY3JC6l; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf07.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.13 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708303272; a=rsa-sha256; cv=none; b=ZL0UH37h1YoTETnCuq0S6ok5xEPikrfmXqP8rZG6MZYwN8ie9LPrGGgDNP+dKqFBz7gAqm dVlah4FrNAkAYZoCOYqhrAz6hIeY9T+PpZZtEWjhxMb1Z9l7QmqLKX7+hmnM5qVj348DCj a6xt/zgIlyIsmCanc/95AFvqwr5Ebaw= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1708303272; x=1739839272; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=9SelTA33djGiCgnQIz4P/Re605CPIWBhuKal9ZySlmM=; b=JuY3JC6lkddiLH+cAtjlNnhZeRN0PaZ0CWYZoXzOQOfx4o/EfimB1uuf IxdCLGiMZq8usZ5JrvXu3k+R5T0LPVQgq9XNicDsZ/XC5TkjYU5nLkPXX Ad6hrfuWijJ9x9X9fTmWLauKEajaP1wpYhkscNktGvWzStifEJWQ8ykYf aFv/44oIv+XmK/R/amVuYghnJXH2OIEDyMSHt90KFA6PFCzK366q0YUkp xks+t4R+7lTkxy8Daf6p8P30q5WlNsLcMpu5pLxxfeqFQpQtPgaQXfAFo IUQ/+OcvHDsle039AvyEfdqBaRAG0zj08j07MPafWTTtRvNCfAeKfC/tX w==; X-IronPort-AV: E=McAfee;i="6600,9927,10988"; a="5329859" X-IronPort-AV: E=Sophos;i="6.06,169,1705392000"; d="scan'208";a="5329859" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmvoesa107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Feb 2024 16:41:09 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10988"; a="912772617" X-IronPort-AV: E=Sophos;i="6.06,169,1705392000"; d="scan'208";a="912772617" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Feb 2024 16:41:05 -0800 From: "Huang, Ying" To: Kairui Song Cc: linux-mm@kvack.org, Andrew Morton , Chris Li , 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 v3] mm/swap: fix race when skipping swapcache In-Reply-To: (Kairui Song's message of "Sun, 18 Feb 2024 17:20:25 +0800") References: <20240216095105.14502-1-ryncsn@gmail.com> <87wmr2rx4a.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Mon, 19 Feb 2024 08:39:09 +0800 Message-ID: <87jzn1s2xe.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: 9250F40011 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: 75awrbcttwzukxsws46s6owmzrncqo7g X-HE-Tag: 1708303271-238099 X-HE-Meta: U2FsdGVkX18D9Fhwt6xe9siFCfI5Bz+RRUDJt73eo3XpArFM2GPbJav8N0PvSIvRtgh/yqFRTMstaZa5ax6chCq1UxKb69uv3AbLuSp6JrWaPof/9EBs4cwIApUzo9Oq5El+ytj8GfEaDjtRmM/gCFjL+Gi1wguzwxvtbXp1k9DKuJ/FDkc4QY9os+e0uy8wKlPR65UhrXb0F6oCRRoBf5bQbEozVTmNOTTlMwBiIMBPqCVZ/UQCKkqJUEQyXMB8bCXems0W1phqKaIWk92wMRoNGpyhbDEsd6xE+FreRiTRQHhg8Dve2pCMvza4VrUuW54TH2nyp+VtzCaKcz8E9I+p28Ow0FuXwqQUvag0RlT/FrlTefoe5wT3OlYJZIY0RELFT8V0EmIA+YS1waAGqRXHl6r/pg9WJ2QyzyKd/Oq7DVVPvkU0+VKSdKDn8C1UnhFO49Ao3x95swN34NAzku0OCWVR0Z6fQELNy6IRSq+DaRJFq2LkuGgeH5mY9HGw99J8AZaoBE0uu4/T89O56/MWoEAdvZw3TZX6dOWpJ9tP+25+GP1/7u5vyLBEr1Z+1d5ZpQgwSgUGiLw6/i9nlH26Ts8wqnm8fbTOk/lCgDFFAw5RjiCyoeHpjBFCcfxaArKn0tua399Q6siyxKrF7EWj7mK9eg9nC2OiIEjYtMxgdCybpk1J9sTSjxu7C9dmkhudjhAUWck4H/e6iEKZs57WEJ7EC8N4GAoOxxp/t2AL/6oKky6zrfLcb6lbcCKaM7i5IdmP0MKZ+b3xXU3ScgbFLLDRUhzMG1Fh4aA51gdUQyIZCEMQ/MGsBogORoB8Uug2+Pz3bDwuY4SvuNPbYuhOKKiCZXOm6fistusyGNJ3THxA2i5OvZqre+PyVRPqyL9Py8THLMYOGgr6WI6vDHGk5ITgwdXpU3dpQvjg4pj9G3Hi8Ju6MT3SH13qPMM0hGqVCp3BbDGhqkWfweZ dgOiY3QH R6l2QoByuAJCa+xTpp9kD4s+Fb+lqESkKahTjjn2zDI0Ob2wtifnJ+5qgHKiTP97pv50xe/JmVHqKkeV0nrdcdnQ2/d2G59ni2yuk/mujgS3hC/Uz2IsFTT/a8emCmxD4aLRTUCspHWbeAE39NDWNxcgTw7mF1AUsOPLZO3E/C1ah4LwBY93JA0KTvgA25xMagaOFWvFxAj7N7VrUcj5+LopcNtGCo9HwzhwzqvghgqKLEJo9GC8Oc3euNutyzBkCp77ma76S5uMG2ULoTlTpPvgFdlExIeEivrbfIWSiVMyo2DtILYUf02qhTRo4cbTsdE3u30Ic4fhizCv+3UiS4VvKgg8YsVlMiRIqx8DyjdGTqfk= 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 Sun, Feb 18, 2024 at 4:34=E2=80=AFPM Huang, Ying wrote: >> >> Kairui Song writes: >> >> > 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 usi= ng >> > the cache flag, and allow only one thread to pin it. Release the pin >> > after PT unlocked. Racers will simply wait since it's a rare and very >> > short event. A schedule() call is added to avoid wasting too much CPU >> > or adding too much noise to perf statistics >> > >> > 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. >> >> The swap entry may be put in swap cache by some parallel code path >> anyway. So, we always need to consider that when reasoning the code. >> >> > 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 = 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 synchrono= us device") >> > Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-stre= ss-race [1] >> > Reported-by: "Huang, Ying" >> > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yhuang6-desk2.c= cr.corp.intel.com/ >> > Signed-off-by: Kairui Song >> > Cc: stable@vger.kernel.org >> > >> > --- >> > Update from V2: >> > - Add a schedule() if raced to prevent repeated page faults wasting CPU >> > and add noise to perf statistics. >> > - Use a bool to state the special case instead of reusing existing >> > variables fixing error handling [Minchan Kim]. >> > >> > V2: https://lore.kernel.org/all/20240206182559.32264-1-ryncsn@gmail.co= m/ >> > >> > Update from V1: >> > - Add some words on ZRAM case, it will discard swap content on swap_fr= ee 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 P= ark] >> > - Update the commit message and summary, refer to SWP_SYNCHRONOUS_IO i= nstead of "direct swapin path" [Yu Zhao] >> > - Update commit message. >> > - Collect Review and Acks. >> > >> > V1: https://lore.kernel.org/all/20240205110959.4021-1-ryncsn@gmail.com/ >> > >> > include/linux/swap.h | 5 +++++ >> > mm/memory.c | 20 ++++++++++++++++++++ >> > mm/swap.h | 5 +++++ >> > mm/swapfile.c | 13 +++++++++++++ >> > 4 files changed, 43 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..7059230d0a54 100644 >> > --- a/mm/memory.c >> > +++ b/mm/memory.c >> > @@ -3799,6 +3799,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) >> > struct page *page; >> > struct swap_info_struct *si =3D NULL; >> > rmap_t rmap_flags =3D RMAP_NONE; >> > + bool need_clear_cache =3D false; >> > bool exclusive =3D false; >> > swp_entry_t entry; >> > pte_t pte; >> > @@ -3867,6 +3868,20 @@ 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 may >> > + * finish swapin first, free the entry, and swap= out >> > + * reusing the same entry. It's undetectable as >> > + * pte_same() returns true due to entry reuse. >> > + */ >> > + if (swapcache_prepare(entry)) { >> > + /* Relax a bit to prevent rapid repeated= page faults */ >> > + schedule(); >> >> The current task may be chosen in schedule(). So, I think that we >> should use cond_resched() here. >> > > I think if we are worried about current task got chosen again we can > use schedule_timeout_uninterruptible(1) here. Isn't cond_resched still > __schedule() and and it can even get omitted, so it should be "weaker" > IIUC. schedule_timeout_uninterruptible(1) will introduce 1ms latency for the second task. That may kill performance of some workloads. -- Best Regards, Huang, Ying