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 E00D1C4345F for ; Thu, 18 Apr 2024 08:57:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 703FF6B0087; Thu, 18 Apr 2024 04:57:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6DA956B0088; Thu, 18 Apr 2024 04:57:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 57B996B0089; Thu, 18 Apr 2024 04:57:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 316026B0087 for ; Thu, 18 Apr 2024 04:57:31 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 94D201210A6 for ; Thu, 18 Apr 2024 08:57:30 +0000 (UTC) X-FDA: 82022049060.18.54CFAAA Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.16]) by imf22.hostedemail.com (Postfix) with ESMTP id 5621FC000D for ; Thu, 18 Apr 2024 08:57:27 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=Oy1NUpxM; spf=pass (imf22.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.16 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713430648; 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=8boA4MBloviJ96kqmFsdT0YwRR6jmZQJ2jqB41+FYSs=; b=UTfX11N/t5dM3r7FcIY708ZyAMzRk216PRWWCdt0y09R7iopbYTHCenxCpEgXfl+LVydt9 +/JEbaFyH6zhmhd06HQhKxcMIPPP9aWmm9lQAOWgMIC5es9gRym8xETlAEHEupjWgfKKFM 5wCJ24E+zZqRLVOwdvK3R0/6qMIdt9Q= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713430648; a=rsa-sha256; cv=none; b=p4V0vZK8rzO8eV3XjwC138ZrL86b0/Eo5m6mFpM8d7fvQ9Lr2F/s2/Kn07VyiRinhXU6VN kfojQYBHDpjLwcFFqbqHr8QOJCkZjRWst+Mrd8DNwHd5M8nYxiyGTyHJwQ1hJM7JvGQlae 1Subw7Bun1jyc5H7unwp/EKLGYQmc1s= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=Oy1NUpxM; spf=pass (imf22.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.16 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1713430648; x=1744966648; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=ibuEIRA3NzNs6eVaJHLMM5650OJmlfASuidh5sZ4E0o=; b=Oy1NUpxMQ6rY+o8WrfzH00iQEGiR2gAYOC6DhdYsdDZ1osmRAuuM1eZS Xgb4k5JzVqiDf/16C99PSTf8+w8gjHq3P4HZAcbknkJ3h4NhKT/Mm1Rvm P0K/SvyjBHt5Pk6vW7QtLnuyOD5qpIvX3fDH++jfEDOgk1nnRuLeEsVse bf8gd3tpuE5U7ME7jGjA/BBb59bYiubVWMT4FaHe7PzVZbynEjrsXrPSY pLz3DtuwZA4upHcnYN/zc8qK1gogayn80Juo1kiouMNWGvyN3OoMKFIKZ q4n+RnFJPdnYCm3HkFF8d8yTJkjLsqjuU8JWcL89BOJUMWyvxs2COUGmX A==; X-CSE-ConnectionGUID: X6VfMsclS/mxsLQecOCosQ== X-CSE-MsgGUID: dK6TnyOhRc+9FyMI5TywtQ== X-IronPort-AV: E=McAfee;i="6600,9927,11047"; a="9513210" X-IronPort-AV: E=Sophos;i="6.07,211,1708416000"; d="scan'208";a="9513210" Received: from fmviesa006.fm.intel.com ([10.60.135.146]) by fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2024 01:57:26 -0700 X-CSE-ConnectionGUID: lrHWGTjCT4O2ymnv2VzK5A== X-CSE-MsgGUID: UOkOnFOgSyCj4qORQxmvnQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,211,1708416000"; d="scan'208";a="22947148" Received: from unknown (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmviesa006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2024 01:57:21 -0700 From: "Huang, Ying" To: Barry Song <21cnbao@gmail.com> Cc: akpm@linux-foundation.org, linux-mm@kvack.org, baolin.wang@linux.alibaba.com, chrisl@kernel.org, david@redhat.com, hanchuanhua@oppo.com, hannes@cmpxchg.org, hughd@google.com, kasong@tencent.com, ryan.roberts@arm.com, surenb@google.com, v-songbaohua@oppo.com, willy@infradead.org, xiang@kernel.org, yosryahmed@google.com, yuzhao@google.com, ziy@nvidia.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/5] mm: swap: introduce swap_free_nr() for batched swap_free() In-Reply-To: (Barry Song's message of "Thu, 18 Apr 2024 17:27:48 +1200") References: <20240409082631.187483-1-21cnbao@gmail.com> <20240409082631.187483-2-21cnbao@gmail.com> <87y19f2lq3.fsf@yhuang6-desk2.ccr.corp.intel.com> <87jzkz2g3t.fsf@yhuang6-desk2.ccr.corp.intel.com> <87bk6b2elo.fsf@yhuang6-desk2.ccr.corp.intel.com> <877cgy2ifu.fsf@yhuang6-desk2.ccr.corp.intel.com> <87ttk20zns.fsf@yhuang6-desk2.ccr.corp.intel.com> <87jzkw25hl.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Thu, 18 Apr 2024 16:55:28 +0800 Message-ID: <87le5bxd6n.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-Stat-Signature: u6mx8wyrqnigtqwokh7cm67qerjubd3o X-Rspamd-Queue-Id: 5621FC000D X-Rspamd-Server: rspam06 X-Rspam-User: X-HE-Tag: 1713430647-270095 X-HE-Meta: U2FsdGVkX18fzLAL6PVLkDKwHSSXFIeVG4gh7egUHPAIffuH9jC2LjvKDyfKuTlwk4D6CT7wbEW3QwzpiWLZ8X0qK9gH4p4NxyZ1TGYSeyojcYX2sOqvCXbkV8IdI3WONC+MLXAB9bFF6jaYMmqMhw4Gq4nxybVbnvRGSrhfZcpvQAZH863GFmseHWvWkhCNZc3tdPA7CUBnqCaio/66BpaYa797JfgO3Xxen2W+pUT4c6slM1g+ci9rb2IhR4zqHLDcOBmsqO0g4lmMonSfXGQ3xI2gnCtbsTvvsyd1SRmIcyh2vUEtN5Oe2T9kRZ+A9YjgG6XJAtVzp+mSPqjBfC7+Cr6+wlsGoaojKR+9SuU1k+8OteNuscgpaXssi1jAXjON/modIeZ2BflA+ZC4ifalNJ2hPKy8Oc5OXeh9pI7oucqWwM2/e8GYC6jhAIAPxy4rv+r2g7IRD0Rcy6g62rixz46uE+OF3mO3gkjSZ58KF0OfhhcZJIdDxfdfdhWNLumSzDXex5IhoUrELFR0POAbu9FLvx9YqXnBUH8dgK45ntCWz2bqPeXRUl5inocsXcTGYcY0w+A3cdfWVno00tjh3MhfijSK2xp+cZQlM6yWJcsqocaIBMVodYtAuJYZrptEuWMjCDP6CoYWzlYtJargSgUmq70f3MLBDmz+x8p5hlr9HoFGCaVy+wZtDubZgEta2lN0y7RDhuvH4UU3p0cn88RjvoQF3A+vpRcTwvFqa4azYbjePKBv8r20dNAl7Lz3gOXqJRaB5daWMnIkyKRT/NKdM28O2nJ3W6kzxKwo2g/NTY0ec+csqpuowQ1oBtDOxLls9LPNME8mYxXowzdBRand4aJLS5Fh+sZk1LPwpS2Q3gPLt5bI7FpkkqQe2ofFQnJwSnafw9Vo5uzzVYRLE/4O2YjUZNumu+tIFKQvT1ojjyXZKQZCm2nAtrLzwX+ZGK1jES3SsxcSrJ7 Hael2CLu 9KcSFtiAVrC/5tKlKZ0ze54zNKWs/R0c801wgOBNlvUt4cLPNjd22RnKGvveFIZ/nSm2TAcFnRnYc1yAvc751YTCXZcS8LhRi6k3jg5Jzvd4yRkt0fjgTSRwRAbXMfeIQqTms6XlZJY+6jNOTb3cJb/slOwYeZHUwxIJ/4oDItAS9NQ73WZSPaAumKDU8pv84BxmwbczGAnrJ6AI0GdvEl7mxhzlYXlsIDymtfVxreMJfoEuqVD8mscUI9cgZVn6R5sMrqsFJcfSYotvqwFXRDfdMhHNXRFNqDN/mv7Owrcv7BNnotpqZ7Yiczg== 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: Barry Song <21cnbao@gmail.com> writes: > On Wed, Apr 17, 2024 at 1:35=E2=80=AFPM Barry Song <21cnbao@gmail.com> wr= ote: >> >> On Wed, Apr 17, 2024 at 12:34=E2=80=AFPM Huang, Ying wrote: >> > >> > Barry Song <21cnbao@gmail.com> writes: >> > >> > > On Tue, Apr 16, 2024 at 3:13=E2=80=AFPM Huang, Ying wrote: >> > >> >> > >> Barry Song <21cnbao@gmail.com> writes: >> > >> >> > >> > On Tue, Apr 16, 2024 at 1:42=E2=80=AFPM Huang, Ying wrote: >> > >> >> >> > >> >> Barry Song <21cnbao@gmail.com> writes: >> > >> >> >> > >> >> > On Mon, Apr 15, 2024 at 8:53=E2=80=AFPM Huang, Ying wrote: >> > >> >> >> >> > >> >> >> Barry Song <21cnbao@gmail.com> writes: >> > >> >> >> >> > >> >> >> > On Mon, Apr 15, 2024 at 8:21=E2=80=AFPM Huang, Ying wrote: >> > >> >> >> >> >> > >> >> >> >> Barry Song <21cnbao@gmail.com> writes: >> > >> >> >> >> >> > >> >> >> >> > On Mon, Apr 15, 2024 at 6:19=E2=80=AFPM Huang, Ying wrote: >> > >> >> >> >> >> >> > >> >> >> >> >> Barry Song <21cnbao@gmail.com> writes: >> > >> >> >> >> >> >> > >> >> >> >> >> > From: Chuanhua Han >> > >> >> >> >> >> > >> > >> >> >> >> >> > While swapping in a large folio, we need to free swap= s related to the whole >> > >> >> >> >> >> > folio. To avoid frequently acquiring and releasing sw= ap locks, it is better >> > >> >> >> >> >> > to introduce an API for batched free. >> > >> >> >> >> >> > >> > >> >> >> >> >> > Signed-off-by: Chuanhua Han >> > >> >> >> >> >> > Co-developed-by: Barry Song >> > >> >> >> >> >> > Signed-off-by: Barry Song >> > >> >> >> >> >> > --- >> > >> >> >> >> >> > include/linux/swap.h | 5 +++++ >> > >> >> >> >> >> > mm/swapfile.c | 51 ++++++++++++++++++++++++++= ++++++++++++++++++ >> > >> >> >> >> >> > 2 files changed, 56 insertions(+) >> > >> >> >> >> >> > >> > >> >> >> >> >> > diff --git a/include/linux/swap.h b/include/linux/swa= p.h >> > >> >> >> >> >> > index 11c53692f65f..b7a107e983b8 100644 >> > >> >> >> >> >> > --- a/include/linux/swap.h >> > >> >> >> >> >> > +++ b/include/linux/swap.h >> > >> >> >> >> >> > @@ -483,6 +483,7 @@ extern void swap_shmem_alloc(swp_= entry_t); >> > >> >> >> >> >> > extern int swap_duplicate(swp_entry_t); >> > >> >> >> >> >> > extern int swapcache_prepare(swp_entry_t); >> > >> >> >> >> >> > extern void swap_free(swp_entry_t); >> > >> >> >> >> >> > +extern void swap_free_nr(swp_entry_t entry, int nr_p= ages); >> > >> >> >> >> >> > extern void swapcache_free_entries(swp_entry_t *entr= ies, int n); >> > >> >> >> >> >> > extern void free_swap_and_cache_nr(swp_entry_t entry= , int nr); >> > >> >> >> >> >> > int swap_type_of(dev_t device, sector_t offset); >> > >> >> >> >> >> > @@ -564,6 +565,10 @@ static inline void swap_free(swp= _entry_t swp) >> > >> >> >> >> >> > { >> > >> >> >> >> >> > } >> > >> >> >> >> >> > >> > >> >> >> >> >> > +void swap_free_nr(swp_entry_t entry, int nr_pages) >> > >> >> >> >> >> > +{ >> > >> >> >> >> >> > +} >> > >> >> >> >> >> > + >> > >> >> >> >> >> > static inline void put_swap_folio(struct folio *foli= o, swp_entry_t swp) >> > >> >> >> >> >> > { >> > >> >> >> >> >> > } >> > >> >> >> >> >> > diff --git a/mm/swapfile.c b/mm/swapfile.c >> > >> >> >> >> >> > index 28642c188c93..f4c65aeb088d 100644 >> > >> >> >> >> >> > --- a/mm/swapfile.c >> > >> >> >> >> >> > +++ b/mm/swapfile.c >> > >> >> >> >> >> > @@ -1356,6 +1356,57 @@ void swap_free(swp_entry_t ent= ry) >> > >> >> >> >> >> > __swap_entry_free(p, entry); >> > >> >> >> >> >> > } >> > >> >> >> >> >> > >> > >> >> >> >> >> > +/* >> > >> >> >> >> >> > + * Free up the maximum number of swap entries at onc= e to limit the >> > >> >> >> >> >> > + * maximum kernel stack usage. >> > >> >> >> >> >> > + */ >> > >> >> >> >> >> > +#define SWAP_BATCH_NR (SWAPFILE_CLUSTER > 512 ? 512 = : SWAPFILE_CLUSTER) >> > >> >> >> >> >> > + >> > >> >> >> >> >> > +/* >> > >> >> >> >> >> > + * Called after swapping in a large folio, >> > >> >> >> >> >> >> > >> >> >> >> >> IMHO, it's not good to document the caller in the funct= ion definition. >> > >> >> >> >> >> Because this will discourage function reusing. >> > >> >> >> >> > >> > >> >> >> >> > ok. right now there is only one user that is why it is a= dded. but i agree >> > >> >> >> >> > we can actually remove this. >> > >> >> >> >> > >> > >> >> >> >> >> >> > >> >> >> >> >> > batched free swap entries >> > >> >> >> >> >> > + * for this large folio, entry should be for the fir= st subpage and >> > >> >> >> >> >> > + * its offset is aligned with nr_pages >> > >> >> >> >> >> >> > >> >> >> >> >> Why do we need this? >> > >> >> >> >> > >> > >> >> >> >> > This is a fundamental requirement for the existing kerne= l, folio's >> > >> >> >> >> > swap offset is naturally aligned from the first moment a= dd_to_swap >> > >> >> >> >> > to add swapcache's xa. so this comment is describing the= existing >> > >> >> >> >> > fact. In the future, if we want to support swap-out foli= o to discontiguous >> > >> >> >> >> > and not-aligned offsets, we can't pass entry as the para= meter, we should >> > >> >> >> >> > instead pass ptep or another different data struct which= can connect >> > >> >> >> >> > multiple discontiguous swap offsets. >> > >> >> >> >> > >> > >> >> >> >> > I feel like we only need "for this large folio, entry sh= ould be for >> > >> >> >> >> > the first subpage" and drop "and its offset is aligned w= ith nr_pages", >> > >> >> >> >> > the latter is not important to this context at all. >> > >> >> >> >> >> > >> >> >> >> IIUC, all these are requirements of the only caller now, n= ot the >> > >> >> >> >> function itself. If only part of the all swap entries of = a mTHP are >> > >> >> >> >> called with swap_free_nr(), can swap_free_nr() still do it= s work? If >> > >> >> >> >> so, why not make swap_free_nr() as general as possible? >> > >> >> >> > >> > >> >> >> > right , i believe we can make swap_free_nr() as general as = possible. >> > >> >> >> > >> > >> >> >> >> >> > >> >> >> >> >> >> > >> >> >> >> >> > + */ >> > >> >> >> >> >> > +void swap_free_nr(swp_entry_t entry, int nr_pages) >> > >> >> >> >> >> > +{ >> > >> >> >> >> >> > + int i, j; >> > >> >> >> >> >> > + struct swap_cluster_info *ci; >> > >> >> >> >> >> > + struct swap_info_struct *p; >> > >> >> >> >> >> > + unsigned int type =3D swp_type(entry); >> > >> >> >> >> >> > + unsigned long offset =3D swp_offset(entry); >> > >> >> >> >> >> > + int batch_nr, remain_nr; >> > >> >> >> >> >> > + DECLARE_BITMAP(usage, SWAP_BATCH_NR) =3D { 0 }; >> > >> >> >> >> >> > + >> > >> >> >> >> >> > + /* all swap entries are within a cluster for mT= HP */ >> > >> >> >> >> >> > + VM_BUG_ON(offset % SWAPFILE_CLUSTER + nr_pages = > SWAPFILE_CLUSTER); >> > >> >> >> >> >> > + >> > >> >> >> >> >> > + if (nr_pages =3D=3D 1) { >> > >> >> >> >> >> > + swap_free(entry); >> > >> >> >> >> >> > + return; >> > >> >> >> >> >> > + } >> > >> >> >> >> >> >> > >> >> >> >> >> Is it possible to unify swap_free() and swap_free_nr() = into one function >> > >> >> >> >> >> with acceptable performance? IIUC, the general rule in= mTHP effort is >> > >> >> >> >> >> to avoid duplicate functions between mTHP and normal sm= all folio. >> > >> >> >> >> >> Right? >> > >> >> >> >> > >> > >> >> >> >> > I don't see why. >> > >> >> >> >> >> > >> >> >> >> Because duplicated implementation are hard to maintain in = the long term. >> > >> >> >> > >> > >> >> >> > sorry, i actually meant "I don't see why not", for some re= ason, the "not" >> > >> >> >> > was missed. Obviously I meant "why not", there was a "but" = after it :-) >> > >> >> >> > >> > >> >> >> >> >> > >> >> >> >> > but we have lots of places calling swap_free(), we may >> > >> >> >> >> > have to change them all to call swap_free_nr(entry, 1); = the other possible >> > >> >> >> >> > way is making swap_free() a wrapper of swap_free_nr() al= ways using >> > >> >> >> >> > 1 as the argument. In both cases, we are changing the se= mantics of >> > >> >> >> >> > swap_free_nr() to partially freeing large folio cases an= d have to drop >> > >> >> >> >> > "entry should be for the first subpage" then. >> > >> >> >> >> > >> > >> >> >> >> > Right now, the semantics is >> > >> >> >> >> > * swap_free_nr() for an entire large folio; >> > >> >> >> >> > * swap_free() for one entry of either a large folio or a= small folio >> > >> >> >> >> >> > >> >> >> >> As above, I don't think the these semantics are important = for >> > >> >> >> >> swap_free_nr() implementation. >> > >> >> >> > >> > >> >> >> > right. I agree. If we are ready to change all those callers= , nothing >> > >> >> >> > can stop us from removing swap_free(). >> > >> >> >> > >> > >> >> >> >> >> > >> >> >> >> >> >> > >> >> >> >> >> > + >> > >> >> >> >> >> > + remain_nr =3D nr_pages; >> > >> >> >> >> >> > + p =3D _swap_info_get(entry); >> > >> >> >> >> >> > + if (p) { >> > >> >> >> >> >> > + for (i =3D 0; i < nr_pages; i +=3D batc= h_nr) { >> > >> >> >> >> >> > + batch_nr =3D min_t(int, SWAP_BA= TCH_NR, remain_nr); >> > >> >> >> >> >> > + >> > >> >> >> >> >> > + ci =3D lock_cluster_or_swap_inf= o(p, offset); >> > >> >> >> >> >> > + for (j =3D 0; j < batch_nr; j++= ) { >> > >> >> >> >> >> > + if (__swap_entry_free_l= ocked(p, offset + i * SWAP_BATCH_NR + j, 1)) >> > >> >> >> >> >> > + __bitmap_set(us= age, j, 1); >> > >> >> >> >> >> > + } >> > >> >> >> >> >> > + unlock_cluster_or_swap_info(p, = ci); >> > >> >> >> >> >> > + >> > >> >> >> >> >> > + for_each_clear_bit(j, usage, ba= tch_nr) >> > >> >> >> >> >> > + free_swap_slot(swp_entr= y(type, offset + i * SWAP_BATCH_NR + j)); >> > >> >> >> >> >> > + >> > >> >> >> >> >> > + bitmap_clear(usage, 0, SWAP_BAT= CH_NR); >> > >> >> >> >> >> > + remain_nr -=3D batch_nr; >> > >> >> >> >> >> > + } >> > >> >> >> >> >> > + } >> > >> >> >> >> >> > +} >> > >> >> >> >> >> > + >> > >> >> >> >> >> > /* >> > >> >> >> >> >> > * Called after dropping swapcache to decrease refcn= t to swap entries. >> > >> >> >> >> >> > */ >> > >> >> >> >> >> >> > >> >> >> >> >> put_swap_folio() implements batching in another method.= Do you think >> > >> >> >> >> >> that it's good to use the batching method in that funct= ion here? It >> > >> >> >> >> >> avoids to use bitmap operations and stack space. >> > >> >> >> >> > >> > >> >> >> >> > Chuanhua has strictly limited the maximum stack usage to= several >> > >> >> >> >> > unsigned long, >> > >> >> >> >> >> > >> >> >> >> 512 / 8 =3D 64 bytes. >> > >> >> >> >> >> > >> >> >> >> So, not trivial. >> > >> >> >> >> >> > >> >> >> >> > so this should be safe. on the other hand, i believe this >> > >> >> >> >> > implementation is more efficient, as put_swap_folio() m= ight lock/ >> > >> >> >> >> > unlock much more often whenever __swap_entry_free_locked= returns >> > >> >> >> >> > 0. >> > >> >> >> >> >> > >> >> >> >> There are 2 most common use cases, >> > >> >> >> >> >> > >> >> >> >> - all swap entries have usage count =3D=3D 0 >> > >> >> >> >> - all swap entries have usage count !=3D 0 >> > >> >> >> >> >> > >> >> >> >> In both cases, we only need to lock/unlock once. In fact,= I didn't >> > >> >> >> >> find possible use cases other than above. >> > >> >> >> > >> > >> >> >> > i guess the point is free_swap_slot() shouldn't be called w= ithin >> > >> >> >> > lock_cluster_or_swap_info? so when we are freeing nr_pages = slots, >> > >> >> >> > we'll have to unlock and lock nr_pages times? and this is = the most >> > >> >> >> > common scenario. >> > >> >> >> >> > >> >> >> No. In put_swap_folio(), free_entries is either SWAPFILE_CLU= STER (that >> > >> >> >> is, nr_pages) or 0. These are the most common cases. >> > >> >> >> >> > >> >> > >> > >> >> > i am actually talking about the below code path, >> > >> >> > >> > >> >> > void put_swap_folio(struct folio *folio, swp_entry_t entry) >> > >> >> > { >> > >> >> > ... >> > >> >> > >> > >> >> > ci =3D lock_cluster_or_swap_info(si, offset); >> > >> >> > ... >> > >> >> > for (i =3D 0; i < size; i++, entry.val++) { >> > >> >> > if (!__swap_entry_free_locked(si, offset + i, = SWAP_HAS_CACHE)) { >> > >> >> > unlock_cluster_or_swap_info(si, ci); >> > >> >> > free_swap_slot(entry); >> > >> >> > if (i =3D=3D size - 1) >> > >> >> > return; >> > >> >> > lock_cluster_or_swap_info(si, offset); >> > >> >> > } >> > >> >> > } >> > >> >> > unlock_cluster_or_swap_info(si, ci); >> > >> >> > } >> > >> >> > >> > >> >> > but i guess you are talking about the below code path: >> > >> >> > >> > >> >> > void put_swap_folio(struct folio *folio, swp_entry_t entry) >> > >> >> > { >> > >> >> > ... >> > >> >> > >> > >> >> > ci =3D lock_cluster_or_swap_info(si, offset); >> > >> >> > if (size =3D=3D SWAPFILE_CLUSTER) { >> > >> >> > map =3D si->swap_map + offset; >> > >> >> > for (i =3D 0; i < SWAPFILE_CLUSTER; i++) { >> > >> >> > val =3D map[i]; >> > >> >> > VM_BUG_ON(!(val & SWAP_HAS_CACHE)); >> > >> >> > if (val =3D=3D SWAP_HAS_CACHE) >> > >> >> > free_entries++; >> > >> >> > } >> > >> >> > if (free_entries =3D=3D SWAPFILE_CLUSTER) { >> > >> >> > unlock_cluster_or_swap_info(si, ci); >> > >> >> > spin_lock(&si->lock); >> > >> >> > mem_cgroup_uncharge_swap(entry, SWAPFI= LE_CLUSTER); >> > >> >> > swap_free_cluster(si, idx); >> > >> >> > spin_unlock(&si->lock); >> > >> >> > return; >> > >> >> > } >> > >> >> > } >> > >> >> > } >> > >> >> >> > >> >> I am talking about both code paths. In 2 most common cases, >> > >> >> __swap_entry_free_locked() will return 0 or !0 for all entries i= n range. >> > >> > >> > >> > I grasp your point, but if conditions involving 0 or non-0 values= fail, we'll >> > >> > end up repeatedly unlocking and locking. Picture a scenario with = a large >> > >> > folio shared by multiple processes. One process might unmap a por= tion >> > >> > while another still holds an entire mapping to it. This could lea= d to situations >> > >> > where free_entries doesn't equal 0 and free_entries doesn't equal >> > >> > nr_pages, resulting in multiple unlock and lock operations. >> > >> >> > >> This is impossible in current caller, because the folio is in the s= wap >> > >> cache. But if we move the change to __swap_entry_free_nr(), we may= run >> > >> into this situation. >> > > >> > > I don't understand why it is impossible, after try_to_unmap_one() ha= s done >> > > on one process, mprotect and munmap called on a part of the large fo= lio >> > > pte entries which now have been swap entries, we are removing the PTE >> > > for this part. Another process can entirely hit the swapcache and ha= ve >> > > all swap entries mapped there, and we call swap_free_nr(entry, nr_pa= ges) in >> > > do_swap_page. Within those swap entries, some have swapcount=3D1 and= others >> > > have swapcount > 1. Am I missing something? >> > >> > For swap entries with swapcount=3D1, its sis->swap_map[] will be >> > >> > 1 | SWAP_HAS_CACHE >> > >> > so, __swap_entry_free_locked() will return SWAP_HAS_CACHE instead of 0. >> > >> > The swap entries will be free in >> > >> > folio_free_swap >> > delete_from_swap_cache >> > put_swap_folio >> > >> >> Yes. I realized this after replying to you yesterday. >> >> > >> > Chuanhua has invested significant effort in following Ryan's sugg= estion >> > >> > for the current approach, which generally handles all cases, espe= cially >> > >> > partial unmapping. Additionally, the widespread use of swap_free_= nr() >> > >> > as you suggested across various scenarios is noteworthy. >> > >> > >> > >> > Unless there's evidence indicating performance issues or bugs, I = believe >> > >> > the current approach remains preferable. >> > >> >> > >> TBH, I don't like the large stack space usage (64 bytes). How abou= t use >> > >> a "unsigned long" as bitmap? Then, we use much less stack space, u= se >> > >> bitmap =3D=3D 0 and bitmap =3D=3D (unsigned long)(-1) to check the = most common >> > >> use cases. And, we have enough batching. >> > > >> > > that is quite a straightforward modification like, >> > > >> > > - #define SWAP_BATCH_NR (SWAPFILE_CLUSTER > 512 ? 512 : SWAPFILE_CLU= STER) >> > > + #define SWAP_BATCH_NR (SWAPFILE_CLUSTER > 64 ? 64 : SWAPFILE_CLUST= ER) >> > > >> > > there is no necessity to remove the bitmap API and move to raw >> > > unsigned long operations. >> > > as bitmap is exactly some unsigned long. on 64bit CPU, we are now one >> > > unsigned long, >> > > on 32bit CPU, it is now two unsigned long. >> > >> > Yes. We can still use most bitmap APIs if we use "unsigned long" as >> > bitmap. The advantage of "unsigned long" is to guarantee that >> > bitmap_empty() and bitmap_full() is trivial. We can use that for >> > optimization. For example, we can skip unlock/lock if bitmap_empty(). >> >> anyway we have avoided lock_cluster_or_swap_info and unlock_cluster_or_s= wap_info >> for each individual swap entry. >> >> if bitma_empty(), we won't call free_swap_slot() so no chance to >> further take any lock, >> right? >> >> the optimization of bitmap_full() seems to be more useful only after we = have >> void free_swap_slot(swp_entry_t entry, int nr) >> >> in which we can avoid many spin_lock_irq(&cache->free_lock); >> >> On the other hand, it seems we can directly call >> swapcache_free_entries() to skip cache if >> nr_pages >=3D SWAP_BATCH(64) this might be an optimization as we are now >> having a bitmap exactly equals 64. > > Hi ying, > considering the below code which has changed bitmap to 64 and generally s= upport > different nr_pages(1 and ever cross cluster), > > #define SWAP_BATCH_NR (SWAPFILE_CLUSTER > 64 ? 64 : SWAPFILE_CLUSTER) > > void swap_free_nr(swp_entry_t entry, int nr_pages) > { > int i =3D 0, j; > struct swap_cluster_info *ci; > struct swap_info_struct *p; > unsigned int type =3D swp_type(entry); > unsigned long offset =3D swp_offset(entry); > int batch_nr, remain_nr; > DECLARE_BITMAP(usage, SWAP_BATCH_NR) =3D { 0 }; > > remain_nr =3D nr_pages; > p =3D _swap_info_get(entry); > if (!p) > return; > > for ( ; ; ) { > batch_nr =3D min3(SWAP_BATCH_NR, remain_nr, > (int)(SWAPFILE_CLUSTER - (offset % > SWAPFILE_CLUSTER))); > > ci =3D lock_cluster_or_swap_info(p, offset); > for (j =3D 0; j < batch_nr; j++) { > if (__swap_entry_free_locked(p, offset + i * > SWAP_BATCH_NR + j, 1)) > __bitmap_set(usage, j, 1); > } > unlock_cluster_or_swap_info(p, ci); > > for_each_clear_bit(j, usage, batch_nr) > free_swap_slot(swp_entry(type, offset + i * > SWAP_BATCH_NR + j)); > > i +=3D batch_nr; > if (i >=3D nr_pages) > break; > > bitmap_clear(usage, 0, SWAP_BATCH_NR); > remain_nr -=3D batch_nr; > } > } > > I still don't see the benefits of using bitmap_full and bitmap_empty over= simple > for_each_clear_bit() unless we begin to support free_swap_slot_nr(), whic= h, > I believe, needs a separate incremental patchset. > > using bitmap_empty and full, if we want to free all slots, we need > if (bitmap_empty(usage)) > { > for (i=3D0;i free_swap_slot(swp_entry(type, offset + i * SWAP_BATCH_NR += j)); > } > This seems just a game replacing for_each_clear_bit by > bitmap_empty()+another for loop. > > if we don't want to free any one, we need > if(bitmap_full(usage) > do_nothing. > > in the for_each_clear_bit() case, the loop just simply ends. > > What's your proposal code to use bitmap_empty and bitmap_full here? > Am I missing something? My idea is something as below. It's only build tested. static void cluster_swap_free_nr(struct swap_info_struct *sis, unsigned long offset, int nr_pages) { struct swap_cluster_info *ci; DECLARE_BITMAP(to_free, BITS_PER_LONG) =3D { 0 }; int i, nr; ci =3D lock_cluster_or_swap_info(sis, offset); while (nr_pages) { nr =3D min(BITS_PER_LONG, nr_pages); for (i =3D 0; i < nr; i++) { if (!__swap_entry_free_locked(sis, offset + i, 1)) bitmap_set(to_free, i, 1); } if (!bitmap_empty(to_free, BITS_PER_LONG)) { unlock_cluster_or_swap_info(sis, ci); for_each_set_bit(i, to_free, BITS_PER_LONG) free_swap_slot(swp_entry(sis->type, offset + i)); if (nr =3D=3D nr_pages) return; bitmap_clear(to_free, 0, BITS_PER_LONG); ci =3D lock_cluster_or_swap_info(sis, offset); } offset +=3D nr; nr_pages -=3D nr; } unlock_cluster_or_swap_info(sis, ci); } void swap_free_nr(swp_entry_t entry, int nr_pages) { int nr; struct swap_info_struct *sis; unsigned long offset =3D swp_offset(entry); sis =3D _swap_info_get(entry); if (!sis) return; while (nr_pages >=3D 0) { nr =3D min_t(int, nr_pages, SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER); cluster_swap_free_nr(sis, offset, nr); offset +=3D nr; nr_pages -=3D nr; } } -- Best Regards, Huang, Ying