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 4367FC48260 for ; Thu, 8 Feb 2024 07:16:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6A7ED6B0071; Thu, 8 Feb 2024 02:16:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 657E76B0074; Thu, 8 Feb 2024 02:16:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 51FCC6B0075; Thu, 8 Feb 2024 02:16:34 -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 404FC6B0071 for ; Thu, 8 Feb 2024 02:16:34 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id BD25E1C1287 for ; Thu, 8 Feb 2024 07:16:33 +0000 (UTC) X-FDA: 81767778666.28.72CF71B Received: from mail-ua1-f51.google.com (mail-ua1-f51.google.com [209.85.222.51]) by imf09.hostedemail.com (Postfix) with ESMTP id 12848140015 for ; Thu, 8 Feb 2024 07:16:31 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=UGQviFcY; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf09.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.51 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=1707376592; 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=GEDru1uOefvH1gyRqHIq3eFRXiLRGNPemxxlD4tRTZE=; b=RXymNXUagF76/lAvoZIHBnyP5NE8EJAnRaujBTCHqdMxgM1uSd2958Ehws/OF4O90+oD1O 4SNcTPcLvoj0sLrWdN/GVUQ9xjc+5p/jEdOnkhJTnhhoKIkuT4P43v5fQ+er0pqOk9SvpW FsuXg4IVoWOl54Djq8lkSFYPThRa3iY= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=UGQviFcY; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf09.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.51 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707376592; a=rsa-sha256; cv=none; b=8QT7wwGOiBLGRLOuHnNRCEHYlYMiy7UO3CTr3/Wh5aBmHsfQiQZP0IooWhFS0H+GOgDHHI uQLeDZ2Q+4audoxB7x8pyO+GxkkcyfDY0rN1jIQRj2eGr1ULQkKbxlQoSr9esD3Kn85/Z3 1pnioXHWIUMPfdt2HGoJFgqGpE8kXgc= Received: by mail-ua1-f51.google.com with SMTP id a1e0cc1a2514c-7d698a8d93cso628350241.3 for ; Wed, 07 Feb 2024 23:16:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707376591; x=1707981391; 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=GEDru1uOefvH1gyRqHIq3eFRXiLRGNPemxxlD4tRTZE=; b=UGQviFcYm5y1/y9hk+jL9DAzzqkxgQEhBox/xwQrNS9Fqg6PgP/aUTlh676dslvN4m VYDIhMzTeBFuxg31pjJNXsGCJ8xmeBKQdqoJKYlwSohkYh17oKQZT0mg3nRpIknOPIKy BI25vD/58IvG9+SewR/USo/Z5BDX5D06vaLqQ2tOX/YaqxCBRbtcNJOG3WJYRoHTso+8 15P56S3nALHFavmZOaenXLIfaMf+VcmEfz+bDLeO1tGy+Y437UozQFLS10VzXjp4Jsec yXde7wRTVeV9TyWJ3IZJJUp86M9XZ6cNwFaTRv5N2xUA0+QCLPB2dnex9phXRsW6JcKP fJUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707376591; x=1707981391; 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=GEDru1uOefvH1gyRqHIq3eFRXiLRGNPemxxlD4tRTZE=; b=sejMMR+Qlky1qdEToJE+z4ympcxyrcuFv5IL5n+FbBgIqVu4DtUwsr9GhBdSTKOGJG 1TSVbSEI8HzgLfEgfS7asd+8svfzVAO/Tiv/tqzgrC4H7UiZhcOl/23e8Uv90CV2MFiV s3e1biUnU6l1CzEJ1cfxj9q+v1uSkLTfGDm0d3bKZeJUDloAESBPuNIT9UzgDImKJlvv P3SkcBn6KcboIiroQ9lXkrXSdZpJs/pkBCJoTH61rFvX5dxRk719rBkRCVKTBBS9zTTd 0fH5MIdFXBaT5n5mInAwHe7lpzWb/a5U2DlXztkGPIg0dn4R47K+haO8Th0kBj2YGlwc Bk4A== X-Forwarded-Encrypted: i=1; AJvYcCV2iG7ZcDZBN8qb5y7/hSUPV617Jm1ny+YXcXbdxbIpvCGJWxXY9PcRh9WPbG+HM3G/TUSmhtaF9/TPYld86YNX4X8= X-Gm-Message-State: AOJu0YwxaEDfPWnWYDIPJ1BaQfkFgu2b+f4Y0bOEbVYa+UB0606Ii3au p86ELIGQW7NnoLo1gdFfpcCdknCrxOJyIkAYSaPOZgTNaGR+vWA41lyqJNp0M6FV1ZHyBLbU5mU jE+8lrC7lA8qWPoVgXC0KU/UBCZxMgF9Ub0w= X-Google-Smtp-Source: AGHT+IFhPiVy/9A4ulaNe0IQiQEZrs6a2zPrvNIgCrPdi19kqhDrmWbgQgOOv6JsWNl61QKN6Jmxi5bXpIpc3D/am5g= X-Received: by 2002:a05:6122:72d:b0:4c0:3b05:75cb with SMTP id 45-20020a056122072d00b004c03b0575cbmr4895381vki.10.1707376590957; Wed, 07 Feb 2024 23:16:30 -0800 (PST) MIME-Version: 1.0 References: <20240206182559.32264-1-ryncsn@gmail.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Thu, 8 Feb 2024 15:16:18 +0800 Message-ID: Subject: Re: [PATCH v2] mm/swap: fix race when skipping swapcache To: Kairui Song Cc: Minchan Kim , Chris Li , 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-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 12848140015 X-Stat-Signature: dqnpxc3sbrz3hqa1hob3budiqmuwjb7w X-Rspam-User: X-HE-Tag: 1707376591-423412 X-HE-Meta: U2FsdGVkX19GJM2/As0CpJCEgwwgtAOGIyBqoxtQipmhFASYPOPDLYN0g6IiL8py0ZNX3AlQksEB95DykkOLwkulnOXLQJgvYh6fZCdi8ZaN6E0wVhRlsRfG8KUoRYjmoqmewHOa0U14hpO3M/3wnJIVHKB+lLWnH+6/1sgpHaafucU6ccjrugQNHnD3uHe+SwCPTa8UhIp6vSCZNushMyDSoFfg59CIn78CRHyrFYp+dMXO7Dw/1ej1wIJ7968I2TEFkAOoC75Uv88QJjvVz15SXrmrZcEU8bOtowefNU+FFd8wn0nSdVH83R0h/Eqit1r3MdgL+lmWYLKMV1mh4BKSFO0QhyJUwbxpVRUU0D/S+uKWMZnbHb2xepBzYvC7rKbHzs1Pzf6nIXbwuQS5crIB4cmGwdmiA0X7fivPjDpmz1aRu1TF+ysqnElZJVr62y8m/xSixxbhySCvTaN7TAAik8zObC+pqS2+Vl30rJ0Ad3d5SnyN+j4ZbJpZwVNKjQbkL8WkZAapMU9OQIF3OdZbCZibgsSHDw5s2SjOYtdffVW06q0RG8QG4tMc+cOMlfZ+N3SLGdoMQ5djL++cF617M38OXgmsw8upaddz1LbhdBiAeGi7ez97n1AArRztz9i7b5MwKH0ZdvSPveaOIOLWjqu2ZisNC81am48e0P4NELHP2skMenILa/A7AnpWT+RXY0ezSYyU8XfTvVkvhOlPKIPP0/ezQSIHriSxpIxeZ6ey2NCISdq3UMv6+Zd+LrNrqghKkMAtzQPkJeaR37y0oGSuyAS9P17urzj4keWe87RY87xh3htEoOYuOb+1+TKFFdzvqQFJC/PmndK3eQBY32h31nL5XvE54UGN433ZttmqNBBuEOHYIhvx92rLLxfSN2/L6hWINjeQDJZ21ZFIsvaLf+wfkx1DpKbR76huiRhEd//Fd1PxNKsu2ZVSNmfWuGPbXxOAQVpiWGu N2A9bQAL p9o/COs5kGrztk9HuCDibeUx87DjjoK7leMGs 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:05=E2=80=AFPM Kairui Song wrote= : > > On Thu, Feb 8, 2024 at 2:31=E2=80=AFAM Minchan Kim w= rote: > > > > 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 = wrote: > > > > > > > > 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@gmai= l.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 V= 2 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 threads > > > > > > > > > swapin the same entry at the same time, they get differen= t pages (A, B). > > > > > > > > > Before one thread (T0) finishes the swapin and installs p= age (A) > > > > > > > > > to the PTE, another thread (T1) could finish swapin of pa= ge (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 co= rruption. > > > > > > > > > > > > > > > > > > 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, i= f > > > > > > > > > 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 s= wap entry using > > > > > > > > > the cache flag, and allow only one thread to pin it. Rele= ase 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 t= o be a good > > > > > > > > > idea after some tests, that will cause racers to fall bac= k 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 cons= tructed > > > > > > > > > reproducer and patched brd (with a delay in read path) [1= ]: > > > > > > > > > > > > > > > > > > With latest 6.8 mainline, race caused data loss can be ob= served 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 mapp= ed pages one by > > > > > > > > > one in opposite direction trying to create a race, with o= ne dedicated > > > > > > > > > thread keep swapping out the data out using madvise. > > > > > > > > > > > > > > > > > > The reproducer created a reproduce rate of about once eve= ry 5 minutes, > > > > > > > > > so the race should be totally possible in production. > > > > > > > > > > > > > > > > > > After this patch, I ran the reproducer for over a few hun= dred rounds > > > > > > > > > and no data loss observed. > > > > > > > > > > > > > > > > > > Performance overhead is minimal, microbenchmark swapin 10= G 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 synchronous device") > > > > > > > > > Reported-by: "Huang, Ying" > > > > > > > > > Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yh= uang6-desk2.ccr.corp.intel.com/ > > > > > > > > > Link: https://github.com/ryncsn/emm-test-project/tree/mas= ter/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 conte= nt 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 buil= t [SeongJae Park] > > > > > > > > > - Update the commit message and summary, refer to SWP_SYN= CHRONOUS_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 *vmf) > > > > > > > > > if (!folio) { > > > > > > > > > if (data_race(si->flags & SWP_SYNCHRONOUS= _IO) && > > > > > > > > > __swap_count(entry) =3D=3D 1) { > > > > > > > > > + /* > > > > > > > > > + * Prevent parallel swapin from p= roceeding with > > > > > > > > > + * the cache flag. Otherwise, ano= ther thread may > > > > > > > > > + * finish swapin first, free the = entry, and swapout > > > > > > > > > + * reusing the same entry. It's u= ndetectable 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 corre= ctly, 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 understand= ing 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 w= ill resume > > > > > > > > from the page fault instruction, possibly generate another = page fault > > > > > > > > at the exact same location. That page fault loop will repea= t until > > > > > > > > CPU1 install the new pte on that faulting virtual address a= nd 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 th= e above > > > > > > > > CPU2 page fault looping situation. I consider the page faul= t looping > > > > > > > > on CPU2 not acceptable. For one it will mess up the page fa= ult > > > > > > > > statistics. > > > > > > > > In my mind, having an explicit loop for CPU2 waiting for th= e 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 l= oop can > > > > > > > miss B. Thus we will trap into an infinite loop. this is even= worse. > > > > > > > > > > > > Yes. You are right, it is worse. Thanks for catching that. That= is why > > > > > > I say this needs more discussion, I haven't fully thought it th= rough > > > > > > :-) > > > > > > > > > > Hi Chris and Barry, > > > > > > > > > > Thanks for the comments! > > > > > > > > > > The worst thing I know of returning in do_swap_page without handl= ing > > > > > the swap, is an increase of some statistic counters, note it will= not > > > > > cause major page fault counters to grow, only things like perf co= unter > > > > > 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 ma= ke > > > > 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, a= nd > > > > > they could all be invalidated if something raced, simply looping > > > > > inside here could miss a lot of corner cases, so we have to go th= rough > > > > > that again. > > > > > > > > Actually, I think about it. Looping it here seems worse in the sen= se > > > > that it is already holding some locks. Return and retry the page fa= ult > > > > 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 = some > > > > > counters, maybe something like returning a VM_FAULT_RETRY could m= ake > > > > > it better, but code is more complex and will cause other counters= to > > > > > 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 effecti= ve > > in your tests. The approach risk forcing racers to rely on the swap cac= he > > 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). Just like a refcount, releasing to 0 should be ok. the last one who sets refcount to 0 will finally release the swap entry. Increasing swap_count from 1 to 2 might cause concurrent threads go into non-sync-io path(full swapcache things), which was Minchan's original patch wanted to avoid. i am not sure if this will negatively affec= t performance. If not, it seems a feasible way. > 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? Thanks Barry