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 A7F0CC4828F for ; Wed, 7 Feb 2024 03:46:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3CB656B0085; Tue, 6 Feb 2024 22:46:04 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 354446B0087; Tue, 6 Feb 2024 22:46:04 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 21BF36B0088; Tue, 6 Feb 2024 22:46:04 -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 0CF276B0085 for ; Tue, 6 Feb 2024 22:46:04 -0500 (EST) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id ABFA180BB4 for ; Wed, 7 Feb 2024 03:46:03 +0000 (UTC) X-FDA: 81763619406.30.2A15D60 Received: from mail-vs1-f53.google.com (mail-vs1-f53.google.com [209.85.217.53]) by imf20.hostedemail.com (Postfix) with ESMTP id DC9CD1C000C for ; Wed, 7 Feb 2024 03:46:01 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IpqZuSSL; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf20.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.53 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=1707277561; 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=kbX5qtQr/EjqD92mfGUpwxjcql20u4gYJdGiDPFLd4s=; b=VW2oh1NRv5Mzr9lvX9rlW4JmTxkqdVPGBLFd+pqTf/bZZbWM2xEAxj/u/KTafOzKdpKurw Smfq2pR4o9Bkxg2h+oKsiu+rz1jeZ7U301UuNE25SL1ENuLLpdqqcrfF0KikQZ0BcPrnSY IE9L4c/Vsy2xJZK5r1z0QE7E9RHk18M= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IpqZuSSL; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf20.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.53 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707277561; a=rsa-sha256; cv=none; b=gTpuhAY4Nrcctbu6GIHylN+nMMWR7YlrDldCA7K8ctVP/xs1HwK/pXjlAZPVu9tY9bCBDK MX2fizFlaPEKG1Fj0fzXD+B9Ml0BQ+9wceboch0n5zXmPqW/Rg2ZCFa9pszGMN+Jbh9F28 2T3qgZ3440Kb/EYZqpNfMqxsw5nw/cU= Received: by mail-vs1-f53.google.com with SMTP id ada2fe7eead31-46d30f6789fso21319137.3 for ; Tue, 06 Feb 2024 19:46:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707277561; x=1707882361; 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=kbX5qtQr/EjqD92mfGUpwxjcql20u4gYJdGiDPFLd4s=; b=IpqZuSSL537Uf9lZdqydUqHPn32MI/2JzWeIJEQyshAi6W/10+0FW6IeQnhaikNbfN DBKPavzJ62FdyIATT9dY0q9KnhtVHulWBkL/k9XTYS/llF2rM33cy97O5p4sT9Zti+0h 8tzK3pcDttUM86mv0F0ZdlyTMpaCeubBLXFb0SIrG3eGMo6iAQ2oXfLXLnp5v0HFsQVZ xz8r5HCgP7y5d87fDTAwkALW1xkAyFwuTXuX5c4NfqYghFpg6r5vECc2FpEtopJADdkk zOSSBx47VvAD9Jqaj8C1V1Gv3XPWDB51E6xfB4K/sh8jJf4k08UdlKchDh3xdJMB9JI0 fuYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707277561; x=1707882361; 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=kbX5qtQr/EjqD92mfGUpwxjcql20u4gYJdGiDPFLd4s=; b=hfyKvlPNiKrHoFdqOgZ1Ap5dZmX1v1a4eK7ZJowUN7Z2zREFC2uoMxs+hXEp4EBXIP 8dN3vu4tSt59ilTTJ7WyNFPBoyj0xvclxM+pMuuVYLZq4WUscAC3VjIK2f5MJ9pJkxks udqeypjX0sTekR+tJVFsJiqS96rSALTQR/7BxybBSIxgUtEUzbdBt6dZrbpnmhabrRYz AvfBu0pa4tgKTW37h0mK+KiHt2uiX8H4p+JSt5nvplWAHA93Ktr4kXYm+pN4UBKxe5jS Zop1TUZ4Xp29LMJDbbpwOsDCDdlMg3ze3bJ9kMsQVa7L0tKkODqiHKI46HP6sFd3/L4N M5LQ== X-Gm-Message-State: AOJu0YwYSXKLnQ/0IimWRK791kqLmYda8d/X/3A0VKtTiJ6zF6eLfRvy hWEWT9WAOkffqfpJRH9V3EQzvqp1XBW0fLHUVn4S6DZS6uZhMc9rzoOLciJUImpJL52xnBSTlA2 oJHu2EENZLc21VYuTG+cQN7cxRoU= X-Google-Smtp-Source: AGHT+IESVNmkqTxmfnQfVqNJZli2dbk2J7qalQUKTZp8u/z+qqQmF4ZwHQUED/Pyx8Ao6A/y0POX0cI/ejK6M0Ot7rk= X-Received: by 2002:a67:e348:0:b0:46d:20a1:e884 with SMTP id s8-20020a67e348000000b0046d20a1e884mr1803438vsm.19.1707277560862; Tue, 06 Feb 2024 19:46:00 -0800 (PST) MIME-Version: 1.0 References: <20240206182559.32264-1-ryncsn@gmail.com> <87sf25yqj2.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Wed, 7 Feb 2024 16:45:49 +1300 Message-ID: Subject: Re: [PATCH v2] mm/swap: fix race when skipping swapcache To: Kairui Song Cc: "Huang, Ying" , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: DC9CD1C000C X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: z7cqnaiozbznjxt4jim9zx9z8hi1xmf8 X-HE-Tag: 1707277561-631567 X-HE-Meta: U2FsdGVkX18ye82Xh4fogVlIv1iU28IdLmyJUWBlI+n0Jx9jB6HEzHfeYAjs4IQhQ5TEuCqGHK+dvorC1ylhar6PGaETJLoCMQIGVcXcULTPjSbgIHwgml8nrUFRjuEChc9GW4kMZvpqBDULGHb1FMXb0eQOr5kfMVuUy5jOVawK6AKNNBmIvTgBdczpfUZfjAArq5xrLsMuj1v9IaFEGZogJV/NOdievwME3PxoycaoOFbB+4gm53Rn919IS9MdhCyzMxomdXjh6YAlB5KcTfl+T6QDlo9UzAokuBzWQswkUwruOJwzfZmBqD/dRPF94ieqxeHm+OqAgW7NSsdGwIeuGdmrc9bbDrQDZDtUPufH+aiVMzy/O9tCC1/WgevFgFMhK1b/fgkc462go/DsCnlfPKbuVtMPWJhlhdJjVivW02mNDzdFpUoZa5jvkUi5F0g2ZQ6oTs5Pi399CIe4dTU4HDdwVNHrO1eXYrFL5l5pvWRx1JLcO+iNBpRbQ5JHID37gcLEZ/sQN3evtb9nTe0LAf+jSI5rFL7JXzwIT8ZurTAOHN7X71zyEQwKNJvYdmMQwBWvBPBSQTVCPKlZcz2MrU1O/nxoIwx7C8hUYQ67wIr/2HdV8xjYBpj1GBNVhxeCjxfLqPpzTgMan5gJVBwpTZZJ6OtKYqrJvfCe4aPKFziLRCgzuLg9NZ0nmXOAfFEI5yLpGO1xNNc6XrBR9FgH14MLZYPjo2veRdpJzYbe1BIBDCyUiIWiP3WvoTZ3xhUSco+hPnYSNrx16LrL+f1hLg4g/vv/He0jTVRte3SfuNaEc2JPRupyn4LFtYX9P8+jCNA4JK4TOSuIrov8y6BC1OoLEzzVbFWGMUPPQMLbQJyUE5aCY7EssDx9kKxmg37JUSDIKHDe+4U1/vxyZikLyltMzkESJjhdwgSuc0ajmt+GOaohZagRE04v5XQAsZGmyGezb3tW/AaYFgu wHqGdsgC geCCumRvxOJHBfPtmdtgRQzgtPMZzbqy261v5XoTjPjkWH5Pbi9gIwxVWUOz70J/oOha/SnX2PAVtL1IjRfKowsVqT8EEi3T5weuHiFHpjFTvOrH/IeKfwQ0ssGTtX5ZZy2gj7QRjZ+h+XQXIB5Xe+hNLjvu/ZckzmY0nTYJTaFaubKwtEeJJNt92rcaJLaNuIqR7Ga9+ZSh5ICI8rDQzgJwDl6ruUcuS/myOcxUo3Hwi49nUfApWAn3K9rj+DewHDULbmd24TsZuW58aJ8k8kTZfHffobHWcVDXhXywwT1U6JNvK1s1Whf/hAqNN2/AX2GwJ5+fyyPWcwGpPYUYt7sFgIrm/qHSZ5A/8g/u5gFiP6PODynfQOdpi+Qc581yD/qbCemo+HCax6itCTCErDa9m5V3DU2faiI1aAHd8mC88XOQGSroXkjuwp1+FjJBMGu+vsGZBzGSMTgWAolqPsPi+JWl5blcVWScCNOo8wU0GbnpZXxLT+rq6Trtg/DdU+SBdDzaJf3a/GmaZE72Ffj4ZSjEJdzEKsG2tCyNDTh/sRJ7dHQaKNSvw7w== 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 3:29=E2=80=AFPM Kairui Song wrote= : > > 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 w= rote: > > >> > > >> 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 thr= eads > > >> > 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) becau= se > > >> > 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 entr= y > > >> > > > >> > > > >> > 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 disca= rd > > >> > 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 p= in > > >> > 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 go= od > > >> > 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 ea= sily: > > >> > $ 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 los= s! > > >> > Round 0: Error on 0x395200, expected 32746, got 32743, 3 data lo= ss! > > >> > Round 0: Error on 0x3fd000, expected 32746, got 32737, 9 data lo= ss! > > >> > Round 0 Failed, 15 data loss! > > >> > > > >> > This reproducer spawns multiple threads sharing the same memory re= gion > > >> > using a small swap device. Every two threads updates mapped pages = one by > > >> > one in opposite direction trying to create a race, with one dedica= ted > > >> > thread keep swapping out the data out using madvise. > > >> > > > >> > The reproducer created a reproduce rate of about once every 5 minu= tes, > > >> > so the race should be totally possible in production. > > >> > > > >> > After this patch, I ran the reproducer for over a few hundred roun= ds > > >> > and no data loss observed. > > >> > > > >> > Performance overhead is minimal, microbenchmark swapin 10G from 32= G > > >> > 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 synch= ronous device") > > >> > Reported-by: "Huang, Ying" > > >> > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yhuang6-des= k2.ccr.corp.intel.com/ > > >> > Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-= stress-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 swa= p_free so the race window is a bit different but cure is the same. [Barry S= ong] > > >> > - Update comments make it cleaner [Huang, Ying] > > >> > - Add a function place holder to fix CONFIG_SWAP=3Dn built [SeongJ= ae 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 *vm= f) > > >> > 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 thre= ad may > > >> > + * finish swapin first, free the entry, an= d swapout > > >> > + * reusing the same entry. It's undetectab= le as > > >> > + * pte_same() returns true due to entry re= use. > > >> > + */ > > >> > + 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 wi= ll > > >> return *unhandled*. Even worse, the "-EEXIST" error is not preserved= , > > >> CPU2 does not even know the page fault is not handled, it will resum= e > > >> from the page fault instruction, possibly generate another page faul= t > > >> at the exact same location. That page fault loop will repeat until > > >> CPU1 install the new pte on that faulting virtual address and pick u= p > > >> 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 i= n > > swap cache for long time. > > > > Another possibility is to move swapcache_prepare() after > > vma_alloc_folio() to reduce the race window. what about we make everything go as it is. I mean, we only need to record we have failed on swapcache_prepare, but we don't goto out. bool swapcache_prepare_failed =3D swapcache_prepare(); .... // don't change any code but we only change the last code to set pte from the below ptl if(pte_same) set_pte to ptl if(pte_same && !swapcache_prepare_failed) set_pte as the chance is close to 0%, the increased count should be very minor. > > 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? Thanks Barry