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 BCB51C48260 for ; Thu, 8 Feb 2024 06:05:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 427E26B0075; Thu, 8 Feb 2024 01:05:17 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3D7346B0078; Thu, 8 Feb 2024 01:05:17 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 29F826B007D; Thu, 8 Feb 2024 01:05:17 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 182116B0075 for ; Thu, 8 Feb 2024 01:05:17 -0500 (EST) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id B1B5B4025A for ; Thu, 8 Feb 2024 06:05:16 +0000 (UTC) X-FDA: 81767599032.13.A0F31CC Received: from mail-lj1-f179.google.com (mail-lj1-f179.google.com [209.85.208.179]) by imf19.hostedemail.com (Postfix) with ESMTP id D2D381A0005 for ; Thu, 8 Feb 2024 06:05:14 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BWAGH6Io; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf19.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.208.179 as permitted sender) smtp.mailfrom=ryncsn@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707372315; a=rsa-sha256; cv=none; b=hXicN5ya28nVhdTmoHLoUAXrVb3raey/H9pqRUq7PyxfdScGAuTytsxeIGq5rJtWl4S03g EJqu/CmymEd3O8XO+GE5nNOwrAO7UnskAbwx5c/f4yrluLKmS/JRWkUdRmacqUiWSuhnYl sdVCcavlLIQ+U//k6mmq9CztSR9hX2U= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BWAGH6Io; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf19.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.208.179 as permitted sender) smtp.mailfrom=ryncsn@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707372315; 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=bz4+FfHyCP6BBKdQ7cOOlMvE98ccARnG+JyeIo2PFwg=; b=W849ORW6I5HKI3Nc7jht6htaPbtm0T16rgXIcvV44HEKJRyTdVi+HCNZ7OMxyLEfV2jH8A GuH9St02DDHVTKKwUYPbSwSMPlzk9Syi+wM6WQC7jOZZ28dJM1Urr9A8Tvp5mg0y+BIi+d 7H51MOBF2xeE7qTNDzBjYrFrYi08vVI= Received: by mail-lj1-f179.google.com with SMTP id 38308e7fff4ca-2d09fefabc1so15816271fa.1 for ; Wed, 07 Feb 2024 22:05:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707372313; x=1707977113; 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=bz4+FfHyCP6BBKdQ7cOOlMvE98ccARnG+JyeIo2PFwg=; b=BWAGH6Io6+0cyVyGuaBScxXoNRCpUz0IptBe6FZxY5lVFYA/EOP+HJIpb5O7KbFPkG vl9AHHDtrhrCLdFgC9+a52p70f6Et2Mb2Nl2meLP18RM0nL45EwWwaqNHa5Tm6ZvJ02K 0VIVN/R9dNHXVvl08OXNKTn3enhMpVOnbT0iwvsOXcICcLvNzA2zHXb2SGPshSBip26Q O/c9B1UM9jQmxdL9eZa0MfODN/MG2ved8iqzEKmx+P80bho5vHefXIJocxcvkuw8iIKP qVHb1CisiMbMjMB5MnAxabReJ8+J6iQMgGSQkLj2rY95i0lSX7xfSlQURXiYvctHRbCd 50tg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707372313; x=1707977113; 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=bz4+FfHyCP6BBKdQ7cOOlMvE98ccARnG+JyeIo2PFwg=; b=GbPQJ8yH4DXfK74cn9Tky8lQTDQUcIF1GrXhRxXH3N9wHA/UIKorpqG3HZXdvQaMux JvRtG/80m/3i78A+f03Bi9k9brCRV7CZ6RHRn56rXgCpeC36JfGzw09B9e3Ryk/uHA1w +OP/WVvhXK1Sm1mAmxJv441DiGzjhzF00IQfhlPuLUWaTkhE7vu6xaIinDcjEpBX8YQS AN5MZtKcxB03XgIFbmu95uu2ktTtLntDu6ggNtwhZEFgWyFmqmYNjGYqnuP5t1N7FFkQ Dy2yHsmdB9tnZySmgDVinK6qPY1lepeBpHdP5Cf57YfxzUCEbFV88bNHzCOoQp4GpwyZ Tr1g== X-Gm-Message-State: AOJu0YyrN6CY1I34oXx9/cbXZUEvYh0/Gv2avc4iZiSAdaTz29M3fdXM II6YUyRAYZgjY6Fn7A5EvXY4N6sihODLjIAyYzktl/s7qcb0zmGLTmv9bV3OMcj9LGqbPf+NqtD db2RvZKITTQpDwnO8ReHjPvuBYHM= X-Google-Smtp-Source: AGHT+IFZu8gPirxAzRWk7AVavJh11h6U+IK5vZrMhokGFrAaJleUdQiMZFoBvJANYdry6eZGw5DE7gxkQEQ281NpUJc= X-Received: by 2002:a2e:b0f0:0:b0:2d0:aa89:a34d with SMTP id h16-20020a2eb0f0000000b002d0aa89a34dmr5157530ljl.11.1707372312698; Wed, 07 Feb 2024 22:05:12 -0800 (PST) MIME-Version: 1.0 References: <20240206182559.32264-1-ryncsn@gmail.com> In-Reply-To: From: Kairui Song Date: Thu, 8 Feb 2024 14:04:54 +0800 Message-ID: Subject: Re: [PATCH v2] mm/swap: fix race when skipping swapcache To: Minchan Kim Cc: Chris Li , Barry Song <21cnbao@gmail.com>, linux-mm@kvack.org, Andrew Morton , "Huang, Ying" , 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-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: D2D381A0005 X-Stat-Signature: 3bb6ie6wpbt3msgxnffpb6ennx8gxy3n X-HE-Tag: 1707372314-942791 X-HE-Meta: U2FsdGVkX19DbKFphW+XtTlwNbpTJOisfM0vZpt0r7tiptl2z2lNUhgGDHrp7jDxX3dMoljnGlOVPb4F+TbVZtLtNHJzBXSFsI+pkWrfQf51+whKxaaCnMbeHSUzqNRvRI2gxojYkqIXVewbGiaVcYkbojpUZzoRLUuCsQ53/djIGBP9dkuMP9NnKIm4sajzgvbVw2sJP+DVczARP+6C1ZIb/reFoszB4L02/ClFbxx860415ImFktv2qHK2fgAQIP6pUTmiPVAUeMnOz1P6QHLcpSurjjOnejDwyrSsS71Fbk4zYn6Lhmv+JSJ3bKkzvjCvwqH2LcIwXiRC8DWl/VfEHKC42+Tpb14zaumj//kjDsaP5pcHCp3VlcnLXOdFc9QJBVbXa/iiCtWEfzKvS85kWFcglYSoAbg7RJYE56fJ2Fe2FkchIvyrZLu7Xgm0+Bvq1xWWnuLsHDWNlv72rcS4Agj/a0jw2f4D9rSiWvuBeiDUFS0Mm6AAwVLH9JMdDTdpBgOS7annmjP30IpGdeMmgkjal9yns76MoEGvXLKULmLV9xydV+VEekND2ag5ZhM2N35YIn+bJ8K7UUA31Rjk+t9cSZ02hhR9NEEKuiw72nOZSYJAGh+Au6w7eLRzWoBkLnLWxwAEqnHJp7NUqqOfAAn6RkaXBgfJKjngsIwKY80DyzscG4I4XSCIWjf7g2PftgF9xhnKxsXycGYeE4hMt8Nj3qlqkCSvSEHNc8YhIxnTYQb8D0vgGfPv7SMskhbmR0Hpl22RiKCfbLlM6sV591Bbg7exvXOWQ8FwwU7cDsf8ALisEP3vXtojVZrS24NruDR/NgWGM9XTvRLApvcuh+wlUbqQrpNfjV0mno0PF81FMBKzfRMlUkxIk4B24so7HV72e6ZGbnIeGESwNyvhCTXa51PYQi4MLJfNbAt8G+xuuSwNAY2kDtV2scKp08m4cYMQ23O7DDMZxTK lvSc57lG EfI+QKtdOD61rIch6Qxsy+GgcawRzy1yDFjsnTKp6qKL0KjQ4vlZDc6SseX/le/1ldXy4MO4oOLPkzhovIc1uy9yOxD2zCZPVVold53tgp3bxHjQzYRplHmzYygI2RGLHct+Q/QH01mpK2vcwTCbc6gKrgJnRtesOwegEEew3g2SieigPgnm+Ukx1Hq7EH23i9zEuZCuvKotsH8OixN1pNXf5w43kcchIsp42tBEJ1P/S6UaFE00efr/uMhR0NudINw7EI/CotzY1OUx9lLX2oCqVicpDVLXjYNbEiL3svy4leHgb/VWXXeoAdnyMc/9/Pgcpz83hUHBBAF5BGwM87TLRmeIZuAXIwZCzcgAoQRqf5QvedZS4LWV/DabVz0qhvVXCP/jDD49cVBKiyiDGNb4IyaJNZ/7fgTkqRzK0BMWpSAcGus+ADNl3oq3REi8t959qXe0E3TDEoWMvYjVfRlxp/1aSSWrfgDd2ZARbbSapdX78Ks6pn7HHVT4sJgz8DIMrIWZ9lyMTJUxwGSSz9lrtFUopK+lH3OebBgrylKJi4Bypo6zHJUBwSg== 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 Thu, Feb 8, 2024 at 2:31=E2=80=AFAM Minchan Kim wro= te: > > On Wed, Feb 07, 2024 at 12:06:15PM +0800, Kairui Song wrote: > > On Wed, Feb 7, 2024 at 12:02=E2=80=AFPM Chris Li wr= ote: > > > > > > On Tue, Feb 6, 2024 at 6:21=E2=80=AFPM Kairui Song = wrote: > > > > > > > > On Wed, Feb 7, 2024 at 10:03=E2=80=AFAM Chris Li wrote: > > > > > > > > > > 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 m= ore threads > > > > > > > > swapin the same entry at the same time, they get different = pages (A, B). > > > > > > > > Before one thread (T0) finishes the swapin and installs pag= e (A) > > > > > > > > to the PTE, another thread (T1) could finish swapin of page= (B), > > > > > > > > swap_free the entry, then swap out the possibly modified pa= ge > > > > > > > > reusing the same entry. It breaks the pte_same check in (T0= ) because > > > > > > > > PTE value is unchanged, causing ABA problem. Thread (T0) wi= ll > > > > > > > > install a stalled page (A) into the PTE and cause data corr= uption. > > > > > > > > > > > > > > > > One possible callstack is like this: > > > > > > > > > > > > > > > > CPU0 CPU1 > > > > > > > > ---- ---- > > > > > > > > do_swap_page() do_swap_page() with sa= me entry > > > > > > > > > > > > > > > > > > > > > > > > swap_read_folio() <- read to page A swap_read_folio() <- r= ead to page B > > > > > > > > > > > > > > > > ... set_pte_at() > > > > > > > > swap_free() <- entry i= s 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 t= o 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 swa= p entry using > > > > > > > > the cache flag, and allow only one thread to pin it. Releas= e 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 l= eads to > > > > > > > > a much more complex scenario. > > > > > > > > > > > > > > > > Reproducer: > > > > > > > > > > > > > > > > This race issue can be triggered easily using a well constr= ucted > > > > > > > > reproducer and patched brd (with a delay in read path) [1]: > > > > > > > > > > > > > > > > With latest 6.8 mainline, race caused data loss can be obse= rved 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 d= ata 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 me= mory 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 hundr= ed 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 o= f synchronous device") > > > > > > > > Reported-by: "Huang, Ying" > > > > > > > > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yhua= ng6-desk2.ccr.corp.intel.com/ > > > > > > > > Link: https://github.com/ryncsn/emm-test-project/tree/maste= r/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 swap_free 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_SYNCH= RONOUS_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_e= ntry_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_fa= ult *vmf) > > > > > > > > if (!folio) { > > > > > > > > if (data_race(si->flags & SWP_SYNCHRONOUS_I= O) && > > > > > > > > __swap_count(entry) =3D=3D 1) { > > > > > > > > + /* > > > > > > > > + * Prevent parallel swapin from pro= ceeding with > > > > > > > > + * the cache flag. Otherwise, anoth= er thread may > > > > > > > > + * finish swapin first, free the en= try, and swapout > > > > > > > > + * reusing the same entry. It's und= etectable as > > > > > > > > + * pte_same() returns true due to e= ntry reuse. > > > > > > > > + */ > > > > > > > > + if (swapcache_prepare(entry)) > > > > > > > > + goto out; > > > > > > > > + > > > > > > > > > > > > > > I am puzzled by this "goto out". If I understand this correct= ly, you > > > > > > > have two threads CPU1 and CPU2 racing to set the flag SWAP_HA= S_CACHE. > > > > > > > The CPU1 will succeed in adding the flag and the CPU2 will g= et > > > > > > > "-EEXIST" from "swapcache_prepare(entry)". Am I understandin= g it > > > > > > > correctly so far? > > > > > > > > > > > > > > Then the goto out seems wrong to me. For the CPU2, the page f= ault will > > > > > > > return *unhandled*. Even worse, the "-EEXIST" error is not pr= eserved, > > > > > > > CPU2 does not even know the page fault is not handled, it wil= l resume > > > > > > > from the page fault instruction, possibly generate another pa= ge 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 pt= e > > > > > > will increase the count of page faults for a couple of times no= w. > > > > > > > > > > > > > > > > > > > > I just re-read your comment: "Racers will simply busy wait si= nce 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 faul= t > > > > > > > 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 ha= ve 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 loo= p can > > > > > > miss B. Thus we will trap into an infinite loop. this is even w= orse. > > > > > > > > > > Yes. You are right, it is worse. Thanks for catching that. That i= s why > > > > > I say this needs more discussion, I haven't fully thought it thro= ugh > > > > > :-) > > > > > > > > Hi Chris and Barry, > > > > > > > > Thanks for the comments! > > > > > > > > The worst thing I know of returning in do_swap_page without handlin= g > > > > the swap, is an increase of some statistic counters, note it will n= ot > > > > cause major page fault counters to grow, only things like perf coun= ter > > > > and vma lock statistic are affected. > > > > > > > > And actually there are multiple already existing return points in > > > > do_swap_page that will return without handling it, which may > > > > re-trigger the page fault. > > > > > > Thanks for pointing that out. I take a look at those, which seems > > > different than the case here. In those cases, it truely can not make > > > forward progress. > > > Here we actually have all the data it needs to complete the page > > > fault. Just a data synchronization issue preventing making forward > > > progress. > > > Ideally we can have some clever data structure to solve the > > > synchronization issue and make forward progress. > > > > > > > When do_swap_page is called, many pre-checks have been applied, and > > > > they could all be invalidated if something raced, simply looping > > > > inside here could miss a lot of corner cases, so we have to go thro= ugh > > > > that again. > > > > > > Actually, I think about it. Looping it here seems worse in the sense > > > that it is already holding some locks. Return and retry the page faul= t > > > at least release those locks and let others have a chance to make > > > progress. > > > > > > > > > > > This patch did increase the chance of false positive increase of so= me > > > > counters, maybe something like returning a VM_FAULT_RETRY could mak= e > > > > it better, but code is more complex and will cause other counters t= o > > > > grow. > > > > > > This is certainly not ideal. It might upset the feedback loop that > > > uses the swap fault statistic as input to adjust the swapping > > > behavior. > > > > > > Chris > > > > Hi Chris, > > > > Thanks for the reply. > > > > So I think the thing is, it's getting complex because this patch > > wanted to make it simple and just reuse the swap cache flags. > > I agree that a simple fix would be the important at this point. > > Considering your description, here's my understanding of the other idea: > Other method, such as increasing the swap count, haven't proven effective > in your tests. The approach risk forcing racers to rely on the swap cache > again and the potential performance loss in race scenario. > > While I understand that simplicity is important, and performance loss > in this case may be infrequent, I believe swap_count approach could be a > suitable solution. What do you think? Hi Minchan Yes, my main concern was about simplicity and performance. Increasing swap_count here will also race with another process from releasing swap_count to 0 (swapcache was able to sync callers in other call paths but we skipped swapcache here). So the right step is: 1. Lock the cluster/swap lock; 2. Check if still have swap_count =3D=3D 1, bail out if not; 3. Set it to 2; __swap_duplicate can be modified to support this, it's similar to existing logics for SWAP_HAS_CACHE. And swap freeing path will do more things, swapcache clean up needs to be handled even in the bypassing path since the racer may add it to swapcache. Reusing SWAP_HAS_CACHE seems to make it much simpler and avoided many overhead, so I used that way in this patch, the only issue is potentially repeated page faults now. I'm currently trying to add a SWAP_MAP_LOCK (or SWAP_MAP_SYNC, I'm bad at naming it) special value, so any racer can just spin on it to avoid all the problems, how do you think about this?