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 32EE5C4345F for ; Mon, 15 Apr 2024 08:06:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B23026B0088; Mon, 15 Apr 2024 04:06:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AD3506B0089; Mon, 15 Apr 2024 04:06:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9C2EA6B008A; Mon, 15 Apr 2024 04:06:17 -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 7AC976B0088 for ; Mon, 15 Apr 2024 04:06:17 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 3EA90A0DB6 for ; Mon, 15 Apr 2024 08:06:17 +0000 (UTC) X-FDA: 82011033594.05.C7773AD Received: from mail-vk1-f173.google.com (mail-vk1-f173.google.com [209.85.221.173]) by imf03.hostedemail.com (Postfix) with ESMTP id 9B92820016 for ; Mon, 15 Apr 2024 08:06:15 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ZFdoI9sD; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.173 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=1713168375; 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=jZov5IXbVD5zRtgFDmBf+N6QydhzyAhsqPviO3FYvFo=; b=CtXZ6aShU/4+q2hIb89Fi8cubmBJopFprskhBaSFMvIbCcEkBH3HvrDMlrBIIzApItwy/l zHE8IRSqQo7ON63xDeOgA1GdPEKx6iCLAxAPgUMhDdUBV98hADv2N3pnETRXkjcwpJ36pK de6K5wbI6tjWKePbnP/Ef+uHDto0dA0= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ZFdoI9sD; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.173 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713168375; a=rsa-sha256; cv=none; b=0gDOLqpz9CUcbnOuBVOQIOfwWH80beL25IdmST9NguUXAavLFderkdsY2k6kIJtErDBd3U R1z14ADyLW3qL53VfBOJaoedyyGO2IbWB4ttGoFWZhx86P2YyWeRL5APv6fgwhXLPv0Izw s/2N1swfqQJ57YN9n8acPkr7S1v8hBU= Received: by mail-vk1-f173.google.com with SMTP id 71dfb90a1353d-4dac112e192so805368e0c.1 for ; Mon, 15 Apr 2024 01:06:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1713168374; x=1713773174; 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=jZov5IXbVD5zRtgFDmBf+N6QydhzyAhsqPviO3FYvFo=; b=ZFdoI9sDD6Ncbn06GRxfcSi/gJIr5/g+kQKsDcEyOQGnWEeN6TPpEtqhn9BqWYEJFE L053TioOtgohljfIzBHBsHdpa6d81UIeKJ/Zc+mj/vJHucjG9JdPRqmUQA8248ssHy8R i3wTa9lwogdLmZpXcVk+gc6b6Gjbd5AROinForexm8rF4c7+EUbPYPl66WpDmNVaoJP8 ph8SiSQ5SUoCpD+dpH6iAOeYMG0Vt7duIgOpkK7aSV4d6nETk06cD2+CdEUnC8Q39gL9 wXlrrigS4L1P19L60HIDA8v6iEuDSkDSEYrB3AH7US7GVRoCQ+n3JC7iWRSQ/I9jnM4I xuKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713168374; x=1713773174; 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=jZov5IXbVD5zRtgFDmBf+N6QydhzyAhsqPviO3FYvFo=; b=lYJ4Pv6IN4/ggHKYhrgvy5Q0yil1QRJRZoSzMwLjTzbLuhp7BE78mA2s9qQ+x5TfFx /o2ly/Rn9/9zJRepuJ2aM1+0U2UBSfVai1mSo3YEBQhkyYZE2ecEz9EHjEUpU0TimxO1 FQJ1caG2isrszymQW69g6pqOV2rK2vg8dXdakGuF+1SAklezBt2Vz6p2Hw7S2/sYqyth qgsSrvH2tLS3/CwzAZROUqbQuK+fEVUHU407ntLgOqr+SF6bnAts7phBauChkJ8WlZu+ MeUmEj+HKEewRGZ3t2CxnlxdNA9ZzzZFGCphKJeoaxyeKsRzLbZy+koS8Tx9l3GdOU/I wqHw== X-Forwarded-Encrypted: i=1; AJvYcCVZz8G1bi1PqdLzxy+78LjQWOGax4QPJ9fdBog2kWZtkBgNqQqsK+9mOeJTRex5eZmdsEO+j4as7MppGsaa8JGq4N8= X-Gm-Message-State: AOJu0YzQIRpUNcmPYRNNgS85amBzYc5My+9kN1gmv72tYnGJ7SxcwRZm FofjYfLUuNCicS7DSUfltJK5NUKQv0dp056TNpU/U7J807DUtV2qNG9QM3njina0R55CUns6NCb UunZ5o08/ebiEHuJY6nETcXtEtnc= X-Google-Smtp-Source: AGHT+IGXW7+hY2jMvtyJ3Z+5CSKVwo38ThgHuB85w+Tj37wRqEtGUCd1FDt5/WygLFYUwmoRhZql8ze4TzHINlVA+uw= X-Received: by 2002:a05:6122:2209:b0:4c9:a9c9:4b3b with SMTP id bb9-20020a056122220900b004c9a9c94b3bmr7330288vkb.9.1713168374572; Mon, 15 Apr 2024 01:06:14 -0700 (PDT) MIME-Version: 1.0 References: <20240409082631.187483-1-21cnbao@gmail.com> <20240409082631.187483-2-21cnbao@gmail.com> <87y19f2lq3.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Mon, 15 Apr 2024 20:06:03 +1200 Message-ID: Subject: Re: [PATCH v2 1/5] mm: swap: introduce swap_free_nr() for batched swap_free() To: "Huang, Ying" 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 9B92820016 X-Stat-Signature: r7mkhf9njg6r7smwr6xa9oijm8ypc5og X-Rspam-User: X-HE-Tag: 1713168375-488394 X-HE-Meta: U2FsdGVkX18eIkDrl+1dnxY70MruitDBPt3tWAW5mhuZczV07xqB06vCSRJPXBX0vJLZJ/pYF2LiJY2Hbj0I4KuJrqm/7+oZHHQWiTwOB0n6buAEaw7WdejaV5pDIYncC1WIkvMUpP8cLR0MtttVuRQAIJsQz9vwApkPqQ90xBtsPUlV4xKjnELFDbshU3cdZCibeCHvVkNPgRUh78zLK96XPWhQUx5aqu8KraBMhe1D66JZh0pYuJ3rlhnklZyvtFDS/Zz+ywjLU8plREfdyjHKi3ykjQCXf9JBNst1QUsnbq0v6tAqf7TMKlaW9LRu6u6Q07xiUfX5+S0z1W+d3odynWdWyKeFbZdX7qFCkDyYrKQr7fLUvDwZoK5SuQtuaEYoKUAsZVEagLfmKVDGSj1eX5WApzSeApZ++ayVbPoYMhGgkFsd/0hF3J/WnKVrGZVM9qOCFH0gwfICmWT30kQ/zj8A3MyazT/W872tZQ2ZDPnyk8MbjRo6NfRSM9oQS7fs7H953MsqbbwvLlCyTXw8hCAnRdIFdkuTmirEq/pv41CkJM/lY5m6pTBZffDoZDky5zSp8YNPFSVl4mnIiQZ0847PE0l4Flspf7Lna2hEDZloq2O4Cbdnaq8rJwBEGQ6ot8HTMKhEafX/ZnGXpPgxxlN1Smuxu6tgcIcWHKAsi1q6yQQn8S3wyYiOZUDRvN3UHHJGYMSRHoY5vpA6VWGsJgj+80LzDC0B5oivw9JkxxBA3KhAfQLxICClKGwC0MUri9mNwXVDgbnM6+bhXctrxlOygVGvhcSstiUWXfA2kxjlYoSsV27raKBwnGZeJjkIGFt0rV9uCeRE7t1QXxbxkt22Ug7Y9yg01zEgkDf3XtAlaRemR4/LznfPqYTbWjpHPJI9CoQitVKph6c1hCd1BJC28z3sj7AM3CREjAcXKwd3hGIPpyfTcd0tSS8Bp2pLLyhyi6p1BB1QDVs aHW6HRX4 V4LqPl/SaUgTE0QPsgmHGIWpiOJ6YtAzMTm8OtXrE0ebjaLf8012P0hKA5szkyQqZ3ixn7uIq0Gsjfi/HCadYlKKGzA== 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 Mon, Apr 15, 2024 at 7:04=E2=80=AFPM Barry Song <21cnbao@gmail.com> wrot= e: > > 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 to the= whole > > > folio. To avoid frequently acquiring and releasing swap 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/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 swp) > > > { > > > } > > > > > > +void swap_free_nr(swp_entry_t entry, int nr_pages) > > > +{ > > > +} > > > + > > > static inline void put_swap_folio(struct folio *folio, swp_entry_t s= wp) > > > { > > > } > > > 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_CLUST= ER) > > > + > > > +/* > > > + * Called after swapping in a large folio, > > > > IMHO, it's not good to document the caller in the function definition. > > 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 discontiguou= s > and not-aligned offsets, we can't pass entry as the parameter, 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 should be for > the first subpage" and drop "and its offset is aligned with nr_pages", > the latter is not important to this context at all. upon further consideration, the comment is inaccurate since we do support nr_pages =3D=3D 1, and do_swap_page() has indeed been invoked with this val= ue. Therefore, we should completely remove the comment. > > > > > > + */ > > > +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_CLUST= ER); > > > + > > > + if (nr_pages =3D=3D 1) { > > > + swap_free(entry); > > > + return; > > > + } > > > > Is it possible to unify swap_free() and swap_free_nr() into one functio= n > > with acceptable performance? IIUC, the general rule in mTHP effort is > > to avoid duplicate functions between mTHP and normal small folio. > > Right? > > I don't see why. 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 possibl= e > 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 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 > > > > > > + > > > + 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, remain_n= r); > > > + > > > + 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)); > > > + > > > + bitmap_clear(usage, 0, SWAP_BATCH_NR); > > > + remain_nr -=3D batch_nr; > > > + } > > > + } > > > +} > > > + > > > /* > > > * Called after dropping swapcache to decrease refcnt to swap entrie= s. > > > */ > > > > put_swap_folio() implements batching in another method. Do you think > > 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, 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. > > > > > -- > > Best Regards, > > Huang, Ying > > Thanks > Barry