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 F3D64C4345F for ; Tue, 16 Apr 2024 01:42:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4C72E6B0085; Mon, 15 Apr 2024 21:42:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4771A6B0088; Mon, 15 Apr 2024 21:42:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 340026B0089; Mon, 15 Apr 2024 21:42:56 -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 1603B6B0085 for ; Mon, 15 Apr 2024 21:42:56 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id C5D5C808B8 for ; Tue, 16 Apr 2024 01:42:55 +0000 (UTC) X-FDA: 82013696310.04.3D35752 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.9]) by imf25.hostedemail.com (Postfix) with ESMTP id D8827A000C for ; Tue, 16 Apr 2024 01:42:52 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=BtwDKwSM; spf=pass (imf25.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.9 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=1713231773; 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=IJ/GnghNC5FG5XNKqyd57O25uoqfIFWW/fWCh5Sm0MQ=; b=pnd0eny8Ic49anuhM2DJbqap0oevpo++GI/8HG/5vtqfM5tp8fcKSpdvJ09uD5zosKeVOJ 9B+IFi1IeP+8qZXKNTWC6xnidbgWaDWrnJF0YCdkFzOg10X8ezcFH3e1xdOhi6CKxn01G4 0zefF1QieELo2mKaw/CZFwsmIoyGP/c= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713231773; a=rsa-sha256; cv=none; b=01Nug+MpCOM9Kg5ELWBPHyA2EMI+15y9wRfovqEBbI0RwoHaouexS6eLJkrW4quXP8oh+L VS9f+FEZQGgWVTNiT2uacivjY7KabHd2zATY/XZsjVL+wuXc6wN3EQ9z/FauUMqaQJXMY0 UEob+/9Tcz0Fc60qOJm3PHkCfjV0lCw= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=BtwDKwSM; spf=pass (imf25.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.9 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=1713231773; x=1744767773; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=yjgus5KxN0bcJTL0XhNlIOKq4ZEdv6JoH0IaNEoUcJE=; b=BtwDKwSM8RibXFLIvICgh8aNUnpao8gwHFl6cDUwJV44z6xr024h2PxY NdiNlFpzQKMUFno7ud2kk27xpLazpnVwHkZJe7yWejuxwVByqpPvbgtKe KuaeKywL7qI4tQoet1wFkui9OTedHsqmWxwCm3d9GG7XGotrLxYVJuor9 y2mn3//bJknwH+Wxu4iU8HDuGkcNcrgcKP0SnjjEQyC7zREWe0Pfb0ytf bL7Bc9N9mtQvWJmu/iElwNUbqAhE0XbGX8O0bSOaUTFQ7KCxlu8TMgggf IH7I11tR5/bTlrkfWdR7+qoEi43uTT8+2Pg57gYB+B5tbCNM4llsYRhd2 w==; X-CSE-ConnectionGUID: y+9vnhlfRcqjiQsAz/5Jug== X-CSE-MsgGUID: oV/7OTueTEakrjUrbksrwQ== X-IronPort-AV: E=McAfee;i="6600,9927,11045"; a="31127220" X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208";a="31127220" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by orvoesa101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2024 18:42:51 -0700 X-CSE-ConnectionGUID: 9eKsD4wFRROMTWY14nTPgg== X-CSE-MsgGUID: s5lVaS31TS64kgwnNUDPNg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208";a="22113243" Received: from unknown (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmviesa010-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2024 18:42:46 -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 "Mon, 15 Apr 2024 21:01:16 +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> Date: Tue, 16 Apr 2024 09:40:53 +0800 Message-ID: <877cgy2ifu.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-Rspamd-Queue-Id: D8827A000C X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: a4o41h63xdqs98q7kz8hiecs1ggdm5sm X-HE-Tag: 1713231772-335731 X-HE-Meta: U2FsdGVkX18UsFVDs0I9pN8O4enPHmc53UYggNj35D+Hg6qn661QBEqAPlCbKYwUTCVF813yAHT+4b/xbYnqcjXtYtZRVjAvnIumYllTdtp643I/BoVRNlwJ8utjO9Rw56oqN8RiUvH1CMDelc4On/PxD7hukuW2a2Edxv/fggN6Hy/w3aiu6j/cSsVxKkG7Fk4nQqn7XAqp3Ma73kcIte0GoqjE8Ci/RtV+Dg2DwJrnH0bUqgA44TJYaY7T1neCGHCkeftirNPupeax2hYypls8rYvwwXj8G/chUqChDzGz0qDrqM12122kw7laDraUwznbpiQD4AMUgOmSrmvSx0JVR1WfrYOzYSOot8TVdIU6wFmKWnSO2OMCdd4mdM+/KSdboh5lwidIFH5YMKAPzbODz2194uSxxdTUidWZj7QrcIIVi+ktVYMACWqZR1gVUZ3RvtZPUEnBVKUSh7yrmhTjlsAv/wPAGeA4ENX292z8ReQrrclEvTogmv8/8cF32ygF9jD6JmPR0tJfRQJYJforuReO1QNwuZ8MdFpyMIbLK9txuTRBeD/GdQkx3QfESA5dSs+00wdN9aBY0X2TgLQq+aKs2yTeSEb2PlkWfx7LM1vymTdwtnZDA7NLCs/q+XLg/gMpgFdlRSfN17KLdI0FaGVK49U6QTJANU0f128s1Q39b6MfiNSIAqrwei8iPA4VR+kedIWKqT7KSTuX2tRbmOIqXHYXMhG70UOJpoGblpoKbcsaDLw3WO7fcBnPpaZwDnfHpwl+9vF+bLtnx+3VCSVokjQqrFxQXdk+m8g1a10kWByoEgJ2v8+f9D5lsedSmPBvQaSx07M6zAhqju2qIXgBd2p3r0DWgkxqYqLwaz6m8UmiPd2amti7RKj2dflGOiVpek8= 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 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 swaps related t= o the whole >> >> >> > folio. To avoid frequently acquiring and releasing swap locks, i= t 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/swap.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_pages); >> >> >> > extern void swapcache_free_entries(swp_entry_t *entries, 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 sw= p) >> >> >> > { >> >> >> > } >> >> >> > >> >> >> > +void swap_free_nr(swp_entry_t entry, int nr_pages) >> >> >> > +{ >> >> >> > +} >> >> >> > + >> >> >> > static inline void put_swap_folio(struct folio *folio, swp_entr= y_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 entry) >> >> >> > __swap_entry_free(p, entry); >> >> >> > } >> >> >> > >> >> >> > +/* >> >> >> > + * Free up the maximum number of swap entries at once 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 function definit= ion. >> >> >> Because this will discourage function reusing. >> >> > >> >> > ok. right now there is only one user that is why it is added. but i= agree >> >> > we can actually remove this. >> >> > >> >> >> >> >> >> > batched free swap entries >> >> >> > + * for this large folio, entry should be for the first subpage = and >> >> >> > + * its offset is aligned with nr_pages >> >> >> >> >> >> Why do we need this? >> >> > >> >> > This is a fundamental requirement for the existing kernel, folio's >> >> > swap offset is naturally aligned from the first moment add_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 folio to discon= tiguous >> >> > and not-aligned offsets, we can't pass entry as the parameter, we s= hould >> >> > 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 should be for >> >> > the first subpage" and drop "and its offset is aligned with nr_page= s", >> >> > the latter is not important to this context at all. >> >> >> >> IIUC, all these are requirements of the only caller now, not 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 its 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 mTHP */ >> >> >> > + 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 fu= nction >> >> >> with acceptable performance? IIUC, the general rule in mTHP effor= t is >> >> >> to avoid duplicate functions between mTHP and normal small folio. >> >> >> Right? >> >> > >> >> > I don't see why. >> >> >> >> Because duplicated implementation are hard to maintain in the long te= rm. >> > >> > sorry, i actually meant "I don't see why not", for some reason, 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 p= ossible >> >> > way is making swap_free() a wrapper of swap_free_nr() always using >> >> > 1 as the argument. In both cases, we are changing the semantics of >> >> > swap_free_nr() to partially freeing large folio cases and have to d= rop >> >> > "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 batch_nr) { >> >> >> > + batch_nr =3D min_t(int, SWAP_BATCH_NR, rem= ain_nr); >> >> >> > + >> >> >> > + ci =3D lock_cluster_or_swap_info(p, offset= ); >> >> >> > + for (j =3D 0; j < batch_nr; j++) { >> >> >> > + if (__swap_entry_free_locked(p, of= fset + 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, off= set + i * SWAP_BATCH_NR + j)); >> >> >> > + >> >> >> > + bitmap_clear(usage, 0, SWAP_BATCH_NR); >> >> >> > + remain_nr -=3D batch_nr; >> >> >> > + } >> >> >> > + } >> >> >> > +} >> >> >> > + >> >> >> > /* >> >> >> > * Called after dropping swapcache to decrease refcnt to swap e= ntries. >> >> >> > */ >> >> >> >> >> >> put_swap_folio() implements batching in another method. Do you th= ink >> >> >> that it's good to use the batching method in that function 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() might 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 within >> > 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_CLUSTER (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_CA= CHE)) { > 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, SWAPFILE_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 in range. > we are mTHP, so we can't assume our size is SWAPFILE_CLUSTER? > or you want to check free_entries =3D=3D "1 << swap_entry_order(folio_ord= er(folio))" > instead of SWAPFILE_CLUSTER for the "for (i =3D 0; i < size; i++, entry.v= al++)" > path? Just replace SWAPFILE_CLUSTER with "nr_pages" in your code. > >> >> >> >> And, we should add batching in __swap_entry_free(). That will help >> >> free_swap_and_cache_nr() too. > > Chris Li and I actually discussed it before, while I completely > agree this can be batched. but i'd like to defer this as an incremental > patchset later to keep this swapcache-refault small. OK. >> >> Please consider this too. -- Best Regards, Huang, Ying