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 AC24CCD1284 for ; Thu, 4 Apr 2024 13:43:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3DC276B0082; Thu, 4 Apr 2024 09:43:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 363A56B0083; Thu, 4 Apr 2024 09:43:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 204F76B0087; Thu, 4 Apr 2024 09:43:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id F20CD6B0082 for ; Thu, 4 Apr 2024 09:43:49 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id B3105120EE2 for ; Thu, 4 Apr 2024 13:43:49 +0000 (UTC) X-FDA: 81971967378.23.296059A Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf24.hostedemail.com (Postfix) with ESMTP id C0CE3180010 for ; Thu, 4 Apr 2024 13:43:46 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=CtVYms7b; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf24.hostedemail.com: domain of chrisl@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=chrisl@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712238226; 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=Zufn2Y1GKYbei+rnjZ27gQisDXNu5uhUljoy/yWGCWI=; b=Jrv6TpIa6tldA1xqaKczgdNEOtfodHvZoiA1b4eP3xLIX48Gf92JPeb4on26HpX+/D1faV AWlwwZB/BSLEHEFenNXGMNqhCFFLeLh5GX01PFXNwnvoFAGUqwWJGTxxfGAnlRkcF89uiz QZCSkHgkFs3A/GbPV4KPIV2AtuVLpSc= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=CtVYms7b; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf24.hostedemail.com: domain of chrisl@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=chrisl@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712238226; a=rsa-sha256; cv=none; b=omHyeI28cNWJNa399O8M4I1MQbFICutnuxojEfcgzv3+7hgLOjozWT11WDWGA2aHtM6RCh 6pOwWdmG85sQf4US3mecVZdD3SOJ23yPdYawofDpIeZ6Gp9Y+5vHAh6wErt8/jPd0qtB6V sZziuYh1fV0S31QSimrXf+SXNRV4FNY= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 805C5616E3 for ; Thu, 4 Apr 2024 13:43:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2B837C43399 for ; Thu, 4 Apr 2024 13:43:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1712238225; bh=o9MhFdlGnw32ziXC3IWaYQCUzaU9mJTeg/Ipm0c064U=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=CtVYms7bi0GUMlzh7g8mj6AxPNKgrpVJAR/mdYntbvUHD1o9iscjF/KBUd8SkeCC/ EaHxT4su0hC9xaaNUy2EzPbyfCTxjYLwU4K5oAuSOkcFC4iaBKN+vbHz/DUf9W93dz 0ru/mTraP0VbRMYnQMNCKeHcAKXg8CiCcWbwkMkDH/fb3Oluq3T6ZDt529BSM/gWdy eRH+GggnHsZ4AioLJ/7k7MchPBVBBt/Kn9a1sWsUrXyg1xU490GsLgj03GSWYIRIMd /FrG5EET5LUiErRN9HXmHQRhp/D9OJ2PJFzUl9nCrePexKTOGIcLWCNwiaX+FmYPFX w+BwdMoI4w5mA== Received: by mail-lj1-f174.google.com with SMTP id 38308e7fff4ca-2d718efedb2so16693191fa.0 for ; Thu, 04 Apr 2024 06:43:45 -0700 (PDT) X-Forwarded-Encrypted: i=1; AJvYcCUUrg12yDF65xjl+HdPUPOR2l03G6V3XlhzRAv93i4wWGwZeJo0lBCldOW4BTgHzM6i016nXzlSF65LBesiu7m0StQ= X-Gm-Message-State: AOJu0Yzi3iOqxgL+Xp6BDM/WWX5QMZ/Du3kAOTVgztqYkcgq6ZvKjTlR 87Dx58fhAIfoOak6YFx8JbJtX8In/mE7lSTCBM1t2r7fhsyzMRA6jCkRtH+l21qXgZrNG1O72c9 w83hZ0W+xqlHFJf/V2dR8atGBCw== X-Google-Smtp-Source: AGHT+IECbm50o89l/5du0PboRTqVze4XM7Gg5DfNdiM8lsiR9jlOlNvwkQ6rhzvERNmqUAUF1jBJ11DEiIYDIxU9RrY= X-Received: by 2002:a05:651c:210e:b0:2d6:8868:f1a6 with SMTP id a14-20020a05651c210e00b002d68868f1a6mr3184834ljq.43.1712238223818; Thu, 04 Apr 2024 06:43:43 -0700 (PDT) MIME-Version: 1.0 References: <20240403114032.1162100-1-ryan.roberts@arm.com> <20240403114032.1162100-2-ryan.roberts@arm.com> <2acd461f-1d75-434c-a2f2-a3a8e1daad8f@arm.com> In-Reply-To: <2acd461f-1d75-434c-a2f2-a3a8e1daad8f@arm.com> From: Chris Li Date: Thu, 4 Apr 2024 06:43:31 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v6 1/6] mm: swap: Remove CLUSTER_FLAG_HUGE from swap_cluster_info:flags To: Ryan Roberts Cc: Andrew Morton , David Hildenbrand , Matthew Wilcox , Huang Ying , Gao Xiang , Yu Zhao , Yang Shi , Michal Hocko , Kefeng Wang , Barry Song <21cnbao@gmail.com>, Lance Yang , linux-mm@kvack.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: C0CE3180010 X-Stat-Signature: 6684ngphaed799d87gttjgrxfnqu3pou X-Rspam-User: X-HE-Tag: 1712238226-397102 X-HE-Meta: U2FsdGVkX1/9joz8ipO5wQaPRRPDJC5R813H45aHbGiDKMFiNvee3BOe5BGpOJELB6haxSkZsl144JJquP56ttkbYjMAqJ0FipBxOb4fJg/gCSKttGKIfOb7ORRz3uqvbX/8gRCpTVxCCXeQi8P3DgZX8xnfUwIA7OpBv0ACW8uHhoezKBw9fRudXJGcyKMM05Pg4RnxSlnhy5uxrXSsmhF6ohslXUaNCzRPVSCu0A+3rrlLdH7h58XAhEqYTJuXDA0Q3B9p2IBCRvRU9Cot5Y2Cd56azp5ufap2Q8+qMpodIaznQJcyT6tFpdlS1Qc95zQYsUBhgCaOByz/wUYmGbKni3TLlzAlKR446+1nEFVPGHf5we6e0hA1QO3WGgC+jNxLuiBwwqQUCHgGAqjakamJ71wTiyLNIebMduPhEn/3Y+rOpIG8ZEEAq/MjdAL0Bbv5WuK/n1NJk/GgmsCKNyu5tYlRIkmJNMEhczzkn6Voh1IAs1BYaCXppe1EWSf4omZPfA4CkMF9eBceSF8WBmjK4qlPPXIrseQiIIfB3iK6vdf3sdXG7VuETVSJyNGgsszkLxGH/HTBCGVMpBjQ1DSU4emI9pIhs68Ab6nZBRkN5IBR+D86+ZcntnTHhKSkvosMWCFwX8ja3FEBArruAKmtl1v93uf9AHNIVOU8atXdocWHIoB+K7AB2CuEvTqSKrwvYDVX0cWNR0KL8WMt0hUlrJdwtMfk7voXQMDWxGz5XR9AjNF6VP4htkuyOHUXxNGKmzzRh+bag7oHMqb2k1kxs5YrxpjduY0KrIzxFY3VVOSxyij4VEdvng8CZypvhwY4sw+QHPRuF+hTjgRU/3raQY6WtYNtSjBRCKQhs/i3QDtaoRrwHI9NORb1+/6h2xUl8Vz3wNr6Akje9t5dMa12iNIjEPWt/v1e5GeH2+mMEHdkRVeJI3g3s14SmG7QbY0kWBcW3kNcNTQ+uNV a2jHpA19 MXL0aqkKyOk/rMSwn2xZiydCgbKm9EwVTr8ktymohum2L/M5chtVbZoW6LES0AChx8c0b9/x4A6Bgmnv0j7X2pbbbyxUmKZKVXoznghsj7s2JVWAoF78NJ7PNSaQwE+gTZB7Inx3pYenbCT2xn7beedX33CecVGbL+A5Z1AY63xXDis8Z8VjzMxvoOhJZy8I7Uzd9qhjVl+CApJp78ejkqdYMvLkWHivNZRtEa5an/2U5xYOreo6dHcNJAYTb+/Jo0ELT6YKRnWZrVyLh25xEj1g9tiIBzTu5IGq9aKVZtSrfQmAs8tIDvR6HnHXlzCyHuqZm5HO3A31H7LTGOVO+XsQQu5Bo7cRyYY21UhElscqlzkoGMN3xVwzHd5zjdVCNq2wW119JBqobr35LabVy0ANtog== 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, Apr 4, 2024 at 12:06=E2=80=AFAM Ryan Roberts = wrote: > > On 03/04/2024 23:12, Chris Li wrote: > > Hi Ryan, > > > > Sorry for the late reply. I want to review this series but don't have > > the chance to do it sooner. > > No problem. This series is now in mm-unstable, so if you want to request = any > changes in the other patches, I'd prefer it sooner rather than later, if = possible. > > > > > On Wed, Apr 3, 2024 at 4:40=E2=80=AFAM Ryan Roberts wrote: > >> > >> As preparation for supporting small-sized THP in the swap-out path, > >> without first needing to split to order-0, Remove the CLUSTER_FLAG_HUG= E, > >> which, when present, always implies PMD-sized THP, which is the same a= s > >> the cluster size. > >> > >> The only use of the flag was to determine whether a swap entry refers = to > >> a single page or a PMD-sized THP in swap_page_trans_huge_swapped(). > >> Instead of relying on the flag, we now pass in nr_pages, which > >> originates from the folio's number of pages. This allows the logic to > >> work for folios of any order. > >> > >> The one snag is that one of the swap_page_trans_huge_swapped() call > >> sites does not have the folio. But it was only being called there to > >> shortcut a call __try_to_reclaim_swap() in some cases. > >> __try_to_reclaim_swap() gets the folio and (via some other functions) > >> calls swap_page_trans_huge_swapped(). So I've removed the problematic > >> call site and believe the new logic should be functionally equivalent. > >> > >> That said, removing the fast path means that we will take a reference > >> and trylock a large folio much more often, which we would like to avoi= d. > >> The next patch will solve this. > >> > >> Removing CLUSTER_FLAG_HUGE also means we can remove split_swap_cluster= () > >> which used to be called during folio splitting, since > >> split_swap_cluster()'s only job was to remove the flag. > > > > Seems necessary to remove the assumption of large folio be PMD size. > > > > Acked-by: Chris Li > > Thanks! > > > > >> > >> Reviewed-by: "Huang, Ying" > >> Signed-off-by: Ryan Roberts > >> --- > >> include/linux/swap.h | 10 ---------- > >> mm/huge_memory.c | 3 --- > >> mm/swapfile.c | 47 ++++++++-----------------------------------= - > >> 3 files changed, 8 insertions(+), 52 deletions(-) > >> > >> diff --git a/include/linux/swap.h b/include/linux/swap.h > >> index a211a0383425..f6f78198f000 100644 > >> --- a/include/linux/swap.h > >> +++ b/include/linux/swap.h > >> @@ -259,7 +259,6 @@ struct swap_cluster_info { > >> }; > >> #define CLUSTER_FLAG_FREE 1 /* This cluster is free */ > >> #define CLUSTER_FLAG_NEXT_NULL 2 /* This cluster has no next cluster = */ > >> -#define CLUSTER_FLAG_HUGE 4 /* This cluster is backing a transparent = huge page */ > >> > >> /* > >> * We assign a cluster to each CPU, so each CPU can allocate swap ent= ry from > >> @@ -590,15 +589,6 @@ static inline int add_swap_extent(struct swap_inf= o_struct *sis, > >> } > >> #endif /* CONFIG_SWAP */ > >> > >> -#ifdef CONFIG_THP_SWAP > >> -extern int split_swap_cluster(swp_entry_t entry); > >> -#else > >> -static inline int split_swap_cluster(swp_entry_t entry) > >> -{ > >> - return 0; > >> -} > >> -#endif > >> - > >> #ifdef CONFIG_MEMCG > >> static inline int mem_cgroup_swappiness(struct mem_cgroup *memcg) > >> { > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >> index ea6d1f09a0b9..3ca9282a0dc9 100644 > >> --- a/mm/huge_memory.c > >> +++ b/mm/huge_memory.c > >> @@ -2844,9 +2844,6 @@ static void __split_huge_page(struct page *page,= struct list_head *list, > >> shmem_uncharge(folio->mapping->host, nr_dropped); > >> remap_page(folio, nr); > >> > >> - if (folio_test_swapcache(folio)) > >> - split_swap_cluster(folio->swap); > >> - > >> /* > >> * set page to its compound_head when split to non order-0 pag= es, so > >> * we can skip unlocking it below, since PG_locked is transfer= red to > >> diff --git a/mm/swapfile.c b/mm/swapfile.c > >> index 5e6d2304a2a4..0d44ee2b4f9c 100644 > >> --- a/mm/swapfile.c > >> +++ b/mm/swapfile.c > >> @@ -343,18 +343,6 @@ static inline void cluster_set_null(struct swap_c= luster_info *info) > >> info->data =3D 0; > >> } > >> > >> -static inline bool cluster_is_huge(struct swap_cluster_info *info) > >> -{ > >> - if (IS_ENABLED(CONFIG_THP_SWAP)) > >> - return info->flags & CLUSTER_FLAG_HUGE; > >> - return false; > >> -} > >> - > >> -static inline void cluster_clear_huge(struct swap_cluster_info *info) > >> -{ > >> - info->flags &=3D ~CLUSTER_FLAG_HUGE; > >> -} > >> - > >> static inline struct swap_cluster_info *lock_cluster(struct swap_info= _struct *si, > >> unsigned long off= set) > >> { > >> @@ -1027,7 +1015,7 @@ static int swap_alloc_cluster(struct swap_info_s= truct *si, swp_entry_t *slot) > >> offset =3D idx * SWAPFILE_CLUSTER; > >> ci =3D lock_cluster(si, offset); > >> alloc_cluster(si, idx); > >> - cluster_set_count_flag(ci, SWAPFILE_CLUSTER, CLUSTER_FLAG_HUGE= ); > >> + cluster_set_count(ci, SWAPFILE_CLUSTER); > >> > >> memset(si->swap_map + offset, SWAP_HAS_CACHE, SWAPFILE_CLUSTER= ); > >> unlock_cluster(ci); > >> @@ -1365,7 +1353,6 @@ void put_swap_folio(struct folio *folio, swp_ent= ry_t entry) > >> > >> ci =3D lock_cluster_or_swap_info(si, offset); > >> if (size =3D=3D SWAPFILE_CLUSTER) { > >> - VM_BUG_ON(!cluster_is_huge(ci)); > >> map =3D si->swap_map + offset; > >> for (i =3D 0; i < SWAPFILE_CLUSTER; i++) { > >> val =3D map[i]; > >> @@ -1373,7 +1360,6 @@ void put_swap_folio(struct folio *folio, swp_ent= ry_t entry) > >> if (val =3D=3D SWAP_HAS_CACHE) > >> free_entries++; > >> } > >> - cluster_clear_huge(ci); > >> if (free_entries =3D=3D SWAPFILE_CLUSTER) { > >> unlock_cluster_or_swap_info(si, ci); > >> spin_lock(&si->lock); > >> @@ -1395,23 +1381,6 @@ void put_swap_folio(struct folio *folio, swp_en= try_t entry) > >> unlock_cluster_or_swap_info(si, ci); > >> } > >> > >> -#ifdef CONFIG_THP_SWAP > >> -int split_swap_cluster(swp_entry_t entry) > >> -{ > >> - struct swap_info_struct *si; > >> - struct swap_cluster_info *ci; > >> - unsigned long offset =3D swp_offset(entry); > >> - > >> - si =3D _swap_info_get(entry); > >> - if (!si) > >> - return -EBUSY; > >> - ci =3D lock_cluster(si, offset); > >> - cluster_clear_huge(ci); > >> - unlock_cluster(ci); > >> - return 0; > >> -} > >> -#endif > >> - > >> static int swp_entry_cmp(const void *ent1, const void *ent2) > >> { > >> const swp_entry_t *e1 =3D ent1, *e2 =3D ent2; > >> @@ -1519,22 +1488,23 @@ int swp_swapcount(swp_entry_t entry) > >> } > >> > >> static bool swap_page_trans_huge_swapped(struct swap_info_struct *si, > >> - swp_entry_t entry) > >> + swp_entry_t entry, > >> + unsigned int nr_pages) > >> { > >> struct swap_cluster_info *ci; > >> unsigned char *map =3D si->swap_map; > >> unsigned long roffset =3D swp_offset(entry); > >> - unsigned long offset =3D round_down(roffset, SWAPFILE_CLUSTER)= ; > >> + unsigned long offset =3D round_down(roffset, nr_pages); > > > > It is obvious this code only works for powers two nr_pages. The > > SWAPFILE_CLSTER is a power of two. If we switch to an API for > > nr_pages, we might want to warn/ban passing in the non-power of two > > nr_pages. > > Indeed. I could change the prototype to pass order instead of nr_pages, t= hen > generate nr_pages (=3D 1 << order) inside the function. But given the fun= ction is > static and only called from a single callsite, I don't see it as hugely > important. I'd prefer to leave as is at this stage, unless you have stron= g > objection. That is fine with me. > > > > >> int i; > >> bool ret =3D false; > >> > >> ci =3D lock_cluster_or_swap_info(si, offset); > >> - if (!ci || !cluster_is_huge(ci)) { > >> + if (!ci || nr_pages =3D=3D 1) { > >> if (swap_count(map[roffset])) > >> ret =3D true; > >> goto unlock_out; > >> } > >> - for (i =3D 0; i < SWAPFILE_CLUSTER; i++) { > >> + for (i =3D 0; i < nr_pages; i++) { > > > > Here we assume the swap entry offset is contiguous. That is beyond > > your patch's scope. If in the future we want to have non-contiguous > > swap entries to swap out large pages, we will need to find out and > > change all the places that have the assumption of contiguous swap > > entries. > > Yes there are tonnes of places that make this assumption :) Yes, that is why I want to have some wrapper API to mark the existing place that makes the assumption. Hopefully we can just change those wrapper functions to implement the non-contiguous version of swap. Thank you for the patch. Chris