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 0127EC54E5D for ; Mon, 18 Mar 2024 22:35:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4FC678D0005; Mon, 18 Mar 2024 18:35:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4AB9A8D0002; Mon, 18 Mar 2024 18:35:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3250C8D0005; Mon, 18 Mar 2024 18:35:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 1D6A08D0002 for ; Mon, 18 Mar 2024 18:35:56 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id A8DDA40B5B for ; Mon, 18 Mar 2024 22:35:55 +0000 (UTC) X-FDA: 81911618670.12.2560CCF Received: from mail-ua1-f49.google.com (mail-ua1-f49.google.com [209.85.222.49]) by imf29.hostedemail.com (Postfix) with ESMTP id CB45312000F for ; Mon, 18 Mar 2024 22:35:52 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ehPZBnkh; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf29.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.49 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=1710801352; 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=kc3DhF9+ScmocSNbzNV6tif7jI3+A9+U0NWPTcBs5Hg=; b=lIaYfaFqOsGof/mz4h97KbMyDXIKh0/Kz/gpjRYkjl6iUNBIMsgkHhS5U1/FciNvWNn0TZ wO6u/cLxhhVeJ5GeJm7NTbZ9YQqjm4PUZgCcRUAy9OYXOrrAIaSfHpJj86Z1Ha9P7JWWIF xPrg6yLY5eWq2rfgJKaF8t0t4KP4qec= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ehPZBnkh; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf29.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.49 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710801352; a=rsa-sha256; cv=none; b=G+ZvQeiM8LdRKkZ5XayOD1Va1jCJWXOsfMOPLmMformfVuJ5rDfz2cOKYmZqOWFBOCYYJM kKaRDmZpwnXVRFCxl8acGbyIngGQQOokDfoea2TZ9crdaTSIoMJ23qRQ77U7Uwc9wg8MJr o9PzAP5tKoXYu11wESrMCWyhLxXsHV8= Received: by mail-ua1-f49.google.com with SMTP id a1e0cc1a2514c-7e083af7ceaso282741241.3 for ; Mon, 18 Mar 2024 15:35:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1710801352; x=1711406152; 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=kc3DhF9+ScmocSNbzNV6tif7jI3+A9+U0NWPTcBs5Hg=; b=ehPZBnkh01Y4JDiMxya2vRiNnvo7v9Nlb7dcquDJC2s+XJFB36CbprNK2YLpR3AeKD 5q9fts5M5IlMNu/uJGYZY372n4BdBGj2kpa+oTC99+HgRj7RNZQruaYPPi+RD7wh+6hP weHUoff0PFsqQbxJSaIdM3R11sSuj9yKOVm9rbkj+3RRadz+KvnOWkA0Uy+tqT/KkcLY IhTkWH3shvAaYzh7siX4ZOnbjw+LYeoZViQfpd6qcy9WAljtF8mwUaeThVofDawuwePy HG4+ryBBdvWBskfsw7zpUtdJlTEiS8+7w3/6GbAmyBdyrgduk/jfhxqJ9soDkBD8kyyy /Qlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710801352; x=1711406152; 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=kc3DhF9+ScmocSNbzNV6tif7jI3+A9+U0NWPTcBs5Hg=; b=wsU41mgD7uNS14ibjyHtIAOoa3J7cTVyL/p4VBcYxn9z1AJ2cOYB+bhjRlnUpYpNyZ zoiwsCBtEq/RG+ikLwaRWD3YEScXFnhz684hDEgAHT10hfNX85Xoz9ZdrxPs0w54Py52 RaJ4jA9vEuSEwNRITMkogVACm6VFsQjdHsEjVfLO40nzv2/BoCzcD4V++fydIyMykQ21 WPtCXhY+1Y6h4pFXsTy85PgfUV1iQ2+PAW8a2TrpQAMyMtmYB0TaM2PUxaypTH72m+xN mmHiZRo2AGq1xDYzJHaBVBO8SnxE+sD4HapnJx+Cb4uIzhW1laSGAoVzoHF597Q6UNTg lcOQ== X-Forwarded-Encrypted: i=1; AJvYcCXXVW4eBB4rYQ3jetpGBe4W7eDy71cUiMWkzJn2ssdGfgpF/nLYy4gz/qIEovahLcnBWUU1+nBHC3GgbTTiNctiTV8= X-Gm-Message-State: AOJu0Yy6pDjphZOA+fh2d6PprKG+5kiVAGYaXoGWkRWDbRDUxTL8AqSl EJCX/JsghcJ3f5zEAwKTr+XbUkpvH7O3QkytK+iqTCkNAh4T/pylO6CxudiuRlsDHAaAb0X/k9F 54DYBm+lp+74SCmvo7wTxrOQugyM= X-Google-Smtp-Source: AGHT+IFAb1WYZJ7j/iOgjuuvdm/Qa1J6hKrNxdrY+P4V22z4ep46iaE22aIXCXtFVdYOVH6opUYp7MrZ5q1ymht/INU= X-Received: by 2002:a67:fd0c:0:b0:472:a8e0:49fe with SMTP id f12-20020a67fd0c000000b00472a8e049femr495077vsr.35.1710801351690; Mon, 18 Mar 2024 15:35:51 -0700 (PDT) MIME-Version: 1.0 References: <20240304081348.197341-1-21cnbao@gmail.com> <20240304081348.197341-5-21cnbao@gmail.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Tue, 19 Mar 2024 11:35:40 +1300 Message-ID: Subject: Re: [RFC PATCH v3 4/5] mm: swap: introduce swapcache_prepare_nr and swapcache_clear_nr for large folios swap-in To: Ryan Roberts Cc: akpm@linux-foundation.org, linux-mm@kvack.org, chengming.zhou@linux.dev, chrisl@kernel.org, david@redhat.com, hannes@cmpxchg.org, kasong@tencent.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mhocko@suse.com, nphamcs@gmail.com, shy828301@gmail.com, steven.price@arm.com, surenb@google.com, wangkefeng.wang@huawei.com, willy@infradead.org, xiang@kernel.org, ying.huang@intel.com, yosryahmed@google.com, yuzhao@google.com, Barry Song , Hugh Dickins , Minchan Kim , SeongJae Park Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: CB45312000F X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: mfuikyjiju3p9jqbja7p5w36y4j3f7w8 X-HE-Tag: 1710801352-937097 X-HE-Meta: U2FsdGVkX1/dvRQRyEqAHTBXglEGdEQu3wYNbrStCXuPTglVVGVLZC/vOGKHQADCQnIzOCa/2/LzOcGAkvRgkqg6syouGmleYhot6n+lI/yNXr9F3YAiMKWGPglm8XeRCk7Ir1pKTNFsx+EMRllgyGfT7rVups5Q5idvoeL3QZaerq8amY9fFT+Hr6g200toz1BfNnnsxxpdFK/LUjOpCac8tXaW7eaQfJp/K+v7GrkasQX18T8/s8lqlgKH5KVgK5Tik5qsWD4XYwXZb4vqXd8B4d49Cn2oUsOs7XfIz3YIq5qc2PnBuKwOWl0ojsNLYiZJM19UHYqHkbLntBDO2P/CLLgRLaHO1EwIIBtzKnYZECFgaS2Phd9hC3QSI6MU7a06gzX5qFuI80bU+xMqkAWZq9elYrPAp9ar7OQK0sYwlk85yYcHfBVvZdJSzYIXJCIyn+ORByrhLt2aSwbnKSxRvOwDJTS6bEgaoamMMPWsu9u11tPK9NmzchMLSLi0K4slYnLBSMP2hpKrJsmVNKNtwFWxhfPGK08OdCv/y4k1alzfHtQyT/jiEbV5X0b5fMmoYIfrLDW5o5T2IxvGWGG6KRNj8Kt0IrArNRZ/fcfXr9mcfL20v9AhouVZlq6qDCtnQ/LTCBxJWSTSu6XWwBbvP3vGXEi+hIs293eaEhb/f1d6s/6a9bW56yFIxIGQESKJ+vq8NrHmTh44l0x44hSbSHglVpTZEJEZF6F+s97Mb/wdcdAHPbNPLdQKn/TXuO1SZHjGdkPQwTuACAdB6sSAakh9m/0TG1RUBpe647O4fBUihktSibn4p3afHCalgyUp0lSnzbdX9iT601RJxV4SN+2o4SfyyHEx9rAKn3GMt16Kp80Ad3ojYGUgDfG4lv/VjdamwcrwQQmey8eDvv3l7DmGZztzm+DHC4Fh3IywRm/WSGgdd8SNDeU27aipIpA1n37zhKmV8XU+kfc oFJONorb FJJteVZbB4xo4kwTBkdGch5AMrrv6psWrpOvmLNX9XklZe+UEb9+B4jmWJxVKAsJT6zXtWa37BiI3ped/ezZCYHc3c+byP3uiXDDBNo4bss8tNAUBGJvS7DPqZ7XpVU0/9alyuazZWEZQCsQaJPHp/PXuzwUIAdvJo576S5IJdF6ion0yhncMQ4cvN7KDlwuBuDb0tAEYBUr4DSBb8pOqW7XgfGJM80ykIjFYMFLVHBIHeNxMByZ1MCE1A+a7NRa3Ia25wPfSV94uQaXGnXL4LyT1xntoy3pUAfQTEydNpWbKdunqewqi4d9nRlUsytH0tfK3VdJfInTG4zHJ8BAG/YuSuvDYTJHQMbl8nmFC1PO47RK3CyKZigRMbqa2nbNujsEQS43pFivBcHM98/yNmTABIZsBnXeYjA8wjFAt3lqDPWmWmdgMjeMD2YFksdKxlel+466Dd/XcYFLhnZt8s/eIBGzTzZx7YlrH4AWdetlnrZSlZ2mVRzTJ34yOHm3LpDrsn4ZVdsT8/IemY/CT7r2br0kvFmJTwSPI3dmiAb9STpaIysy5y6kqanTR8MEd4fgFohg9cocuD8OdXbXZBH1gwLVkjjn0PcOc4eEFr5vXXGU3yW4IPCPj3/RjlZB0G/kRbyNL1uMg+1KjJIuqeUNKd+UXyZ4hbN8kqGGwnnaFZgOV2X1j/j+K3mw1BceL/rbwQrpJ+PRyMDnz2yhz6JuIHjLXrWGzyXyrE1X/x6kAdtirnsJS9wg7TsFf0+5l82mJLQz5j8ztK7dyWfY58B3DpvDOco/aKgwqbpGs8vd6Fj3O9yrpfPYWNIX6aK8E19mbs7SX2HuJPRkN57dXFdsABXErWivDEQoLSkUXRw0v6G5s9Vl/jlapw8vtq93ZkA8f 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, Mar 13, 2024 at 4:35=E2=80=AFAM Ryan Roberts = wrote: > > On 04/03/2024 08:13, Barry Song wrote: > > From: Barry Song > > > > Commit 13ddaf26be32 ("mm/swap: fix race when skipping swapcache") suppo= rts > > one entry only, to support large folio swap-in, we need to handle multi= ple > > swap entries. > > > > Cc: Kairui Song > > Cc: "Huang, Ying" > > Cc: David Hildenbrand > > Cc: Chris Li > > Cc: Hugh Dickins > > Cc: Johannes Weiner > > Cc: Matthew Wilcox (Oracle) > > Cc: Michal Hocko > > Cc: Minchan Kim > > Cc: Yosry Ahmed > > Cc: Yu Zhao > > Cc: SeongJae Park > > Signed-off-by: Barry Song > > --- > > include/linux/swap.h | 1 + > > mm/swap.h | 1 + > > mm/swapfile.c | 118 ++++++++++++++++++++++++++----------------- > > 3 files changed, 74 insertions(+), 46 deletions(-) > > > > diff --git a/include/linux/swap.h b/include/linux/swap.h > > index d6ab27929458..22105f0fe2d4 100644 > > --- a/include/linux/swap.h > > +++ b/include/linux/swap.h > > @@ -480,6 +480,7 @@ extern int add_swap_count_continuation(swp_entry_t,= gfp_t); > > extern void swap_shmem_alloc(swp_entry_t); > > extern int swap_duplicate(swp_entry_t); > > extern int swapcache_prepare(swp_entry_t); > > +extern int swapcache_prepare_nr(swp_entry_t entry, int nr); > > extern void swap_free(swp_entry_t); > > extern void swap_nr_free(swp_entry_t entry, int nr_pages); > > extern void swapcache_free_entries(swp_entry_t *entries, int n); > > diff --git a/mm/swap.h b/mm/swap.h > > index fc2f6ade7f80..1cec991efcda 100644 > > --- a/mm/swap.h > > +++ b/mm/swap.h > > @@ -42,6 +42,7 @@ void delete_from_swap_cache(struct folio *folio); > > void clear_shadow_from_swap_cache(int type, unsigned long begin, > > unsigned long end); > > void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry); > > +void swapcache_clear_nr(struct swap_info_struct *si, swp_entry_t entry= , int nr); > > struct folio *swap_cache_get_folio(swp_entry_t entry, > > struct vm_area_struct *vma, unsigned long addr); > > struct folio *filemap_get_incore_folio(struct address_space *mapping, > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > index 244106998a69..bae1b8165b11 100644 > > --- a/mm/swapfile.c > > +++ b/mm/swapfile.c > > @@ -3309,7 +3309,7 @@ void si_swapinfo(struct sysinfo *val) > > } > > > > /* > > - * Verify that a swap entry is valid and increment its swap map count. > > + * Verify that nr swap entries are valid and increment their swap map = count. > > * > > * Returns error code in following case. > > * - success -> 0 > > @@ -3319,66 +3319,76 @@ void si_swapinfo(struct sysinfo *val) > > * - swap-cache reference is requested but the entry is not used. -> E= NOENT > > * - swap-mapped reference requested but needs continued swap count. -= > ENOMEM > > */ > > -static int __swap_duplicate(swp_entry_t entry, unsigned char usage) > > +static int __swap_duplicate_nr(swp_entry_t entry, int nr, unsigned cha= r usage) > > perhaps its better to pass order instead of nr to all these functions to = make it > clearer that entry should be naturally aligned and be a power-of-2 number= of > pages, no bigger than SWAPFILE_CLUSTER? > > > { > > struct swap_info_struct *p; > > struct swap_cluster_info *ci; > > unsigned long offset; > > - unsigned char count; > > - unsigned char has_cache; > > - int err; > > + unsigned char count[SWAPFILE_CLUSTER]; > > + unsigned char has_cache[SWAPFILE_CLUSTER]; > > I'm not sure this 1K stack buffer is a good idea? > > Could you split it slightly differently so that loop 1 just does error ch= ecking > and bails out if an error is found, and loop 2 does the new value calcula= tion > and writeback? Then you don't need these arrays. right. we can totally remove thoe arrays by re-reading swap_map. > > > + int err, i; > > > > p =3D swp_swap_info(entry); > > > > offset =3D swp_offset(entry); > > ci =3D lock_cluster_or_swap_info(p, offset); > > > > - count =3D p->swap_map[offset]; > > - > > - /* > > - * swapin_readahead() doesn't check if a swap entry is valid, so = the > > - * swap entry could be SWAP_MAP_BAD. Check here with lock held. > > - */ > > - if (unlikely(swap_count(count) =3D=3D SWAP_MAP_BAD)) { > > - err =3D -ENOENT; > > - goto unlock_out; > > - } > > - > > - has_cache =3D count & SWAP_HAS_CACHE; > > - count &=3D ~SWAP_HAS_CACHE; > > - err =3D 0; > > - > > - if (usage =3D=3D SWAP_HAS_CACHE) { > > + for (i =3D 0; i < nr; i++) { > > + count[i] =3D p->swap_map[offset + i]; > > > > - /* set SWAP_HAS_CACHE if there is no cache and entry is u= sed */ > > - if (!has_cache && count) > > - has_cache =3D SWAP_HAS_CACHE; > > - else if (has_cache) /* someone else added cac= he */ > > - err =3D -EEXIST; > > - else /* no users remaining */ > > + /* > > + * swapin_readahead() doesn't check if a swap entry is va= lid, so the > > + * swap entry could be SWAP_MAP_BAD. Check here with lock= held. > > + */ > > + if (unlikely(swap_count(count[i]) =3D=3D SWAP_MAP_BAD)) { > > err =3D -ENOENT; > > + goto unlock_out; > > + } > > > > - } else if (count || has_cache) { > > - > > - if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX) > > - count +=3D usage; > > - else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) > > - err =3D -EINVAL; > > - else if (swap_count_continued(p, offset, count)) > > - count =3D COUNT_CONTINUED; > > - else > > - err =3D -ENOMEM; > > - } else > > - err =3D -ENOENT; /* unused swap entry */ > > + has_cache[i] =3D count[i] & SWAP_HAS_CACHE; > > + count[i] &=3D ~SWAP_HAS_CACHE; > > + err =3D 0; > > + > > + if (usage =3D=3D SWAP_HAS_CACHE) { > > + > > + /* set SWAP_HAS_CACHE if there is no cache and en= try is used */ > > + if (!has_cache[i] && count[i]) > > + has_cache[i] =3D SWAP_HAS_CACHE; > > + else if (has_cache[i]) /* someone else a= dded cache */ > > + err =3D -EEXIST; > > + else /* no users remai= ning */ > > + err =3D -ENOENT; > > + } else if (count[i] || has_cache[i]) { > > + > > + if ((count[i] & ~COUNT_CONTINUED) < SWAP_MAP_MAX) > > + count[i] +=3D usage; > > + else if ((count[i] & ~COUNT_CONTINUED) > SWAP_MAP= _MAX) > > + err =3D -EINVAL; > > + else if (swap_count_continued(p, offset + i, coun= t[i])) > > + count[i] =3D COUNT_CONTINUED; > > + else > > + err =3D -ENOMEM; > > + } else > > + err =3D -ENOENT; /* unused swap = entry */ > > > > - if (!err) > > - WRITE_ONCE(p->swap_map[offset], count | has_cache); > > + if (err) > > + break; > > + } > > > > + if (!err) { > > + for (i =3D 0; i < nr; i++) > > + WRITE_ONCE(p->swap_map[offset + i], count[i] | ha= s_cache[i]); > > + } > > unlock_out: > > unlock_cluster_or_swap_info(p, ci); > > return err; > > } > > > > +static int __swap_duplicate(swp_entry_t entry, unsigned char usage) > > +{ > > + return __swap_duplicate_nr(entry, 1, usage); > > +} > > + > > /* > > * Help swapoff by noting that swap entry belongs to shmem/tmpfs > > * (in which case its reference count is never incremented). > > @@ -3417,17 +3427,33 @@ int swapcache_prepare(swp_entry_t entry) > > return __swap_duplicate(entry, SWAP_HAS_CACHE); > > } > > > > -void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry) > > +int swapcache_prepare_nr(swp_entry_t entry, int nr) > > +{ > > + return __swap_duplicate_nr(entry, nr, SWAP_HAS_CACHE); > > +} > > + > > +void swapcache_clear_nr(struct swap_info_struct *si, swp_entry_t entry= , int nr) > > { > > struct swap_cluster_info *ci; > > unsigned long offset =3D swp_offset(entry); > > - unsigned char usage; > > + unsigned char usage[SWAPFILE_CLUSTER]; > > + int i; > > > > ci =3D lock_cluster_or_swap_info(si, offset); > > - usage =3D __swap_entry_free_locked(si, offset, SWAP_HAS_CACHE); > > + for (i =3D 0; i < nr; i++) > > + usage[i] =3D __swap_entry_free_locked(si, offset + i, SWA= P_HAS_CACHE); > > unlock_cluster_or_swap_info(si, ci); > > - if (!usage) > > - free_swap_slot(entry); > > + for (i =3D 0; i < nr; i++) { > > + if (!usage[i]) { > > + free_swap_slot(entry); > > + entry.val++; > > + } > > + } > > +} > > This is pretty similar to swap_nr_free() which you added in patch 2. Exce= pt > swap_nr_free() passes 1 as last param to __swap_entry_free_locked() and t= his > passes SWAP_HAS_CACHE. Perhaps their should be a common helper? I think > swap_nr_free()'s usage bitmap is preferable to this version's char array = too. right. > > Thanks, > Ryan > > > + > > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry) > > +{ > > + swapcache_clear_nr(si, entry, 1); > > } > > > > struct swap_info_struct *swp_swap_info(swp_entry_t entry) Thanks Barry