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 19819CD1292 for ; Thu, 4 Apr 2024 07:06:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A81736B0098; Thu, 4 Apr 2024 03:06:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A58E26B0099; Thu, 4 Apr 2024 03:06:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8F9326B009A; Thu, 4 Apr 2024 03:06:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 6FB066B0098 for ; Thu, 4 Apr 2024 03:06:17 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id EBCB11C0748 for ; Thu, 4 Apr 2024 07:06:16 +0000 (UTC) X-FDA: 81970965552.21.6CC33EC Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf15.hostedemail.com (Postfix) with ESMTP id CA050A000C for ; Thu, 4 Apr 2024 07:06:14 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712214375; 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; bh=g/aX80iFLCO/FfU8A7tj3YRxYUIwPn9GWCCE36tlrUg=; b=OpSwa62ulKRvGY5SE8bOwAl2Z4uCAOOP/OuWlJCpKOrorzKchpF/tJ40VUb1XAbACMEAD5 YkGEElhsQM1edld3Nl2UMqYKUXc3njDqrN7lvD1xg+PBJOussNROsi9n5bPBVllA/d5NKh Ege2WdwLDUj03QkV9kRXGTpk7iS28zo= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712214375; a=rsa-sha256; cv=none; b=izwmAeiHwgVHL1phIg73B1r+jj/G43Hsb3gK2+YHgNoX9X8yb6xSRA9A1/igUtLV5iuohL oFpvw+5fYyVm0zjTrRGuTIVqvAsh9RrDgmfHEVAiGNJyxThJqAMmLdPRc2e8xsvzfJ86vY Jh9LrmcfJu7vhYJgeZnFBhiD0U04w2A= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A2B901007; Thu, 4 Apr 2024 00:06:44 -0700 (PDT) Received: from [10.57.72.245] (unknown [10.57.72.245]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E86B23F64C; Thu, 4 Apr 2024 00:06:11 -0700 (PDT) Message-ID: <2acd461f-1d75-434c-a2f2-a3a8e1daad8f@arm.com> Date: Thu, 4 Apr 2024 08:06:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 1/6] mm: swap: Remove CLUSTER_FLAG_HUGE from swap_cluster_info:flags Content-Language: en-GB To: Chris Li 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 References: <20240403114032.1162100-1-ryan.roberts@arm.com> <20240403114032.1162100-2-ryan.roberts@arm.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: CA050A000C X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: 8fe9dxnsb8knmww3ahogom9s3y7r54y1 X-HE-Tag: 1712214374-685256 X-HE-Meta: U2FsdGVkX1/y2YCcLMj9VmOOJel0nZ1G908B4x5CvvzsM7KWNk/Ej/dKaAL9QMhTetCJ0EpTWNqwA+BZFpPGwrrXeUcmCi7FphG/hl6nhtseOY8hjGIoK06WQPb8Sm2FaftwoIr8ug1bwA3iNoM6k853dgtLx9VW5Cv9XJaX90wzWagSXzP2+b/awZCI4zivNwedDSE+slXLkV+VTdEYmOSMfTE8y17QFgsjy8dBX51TZXATEXT7IfuHRUsgFzclb/HAhdBesb8XcHOvOaZ0V1S+iARClOwEfWq7ZVc0kRpvnpeqiYeC6pSMM/oGuwSIjw9QqrujmOSmLAsHEiIufxocBfQV3McsqZyYVyOEDMQ9HcQPYZSN5eqkx4Zh8ntNTR6e4pCpXLIhJPobI3AvqUlfQWaW10PiOTlF+32aXgoF3sVFc+hRIERv273lDYyx9vXA6lQUHjLm7TZf4C8se3LwQXzd/zNaBwTRfu+0HkFUrTyVeEei/fg63Xxxz4jGSi+UFFGzjg5rjQPU6cqMptPYi2q3MtpvBnEiQTRS908f0a/yuVBDUt0fTDsoH8kM5cc4zzMCt/b/C4o5hGeX4OkdN4KG+5p24vQpHMx8KD8isWJCEE+PdY3iM3QLzN2ZgndbF+JfXUhh2uZxDIxMONIDA2R1FolW1TKreDu3BI+C6kRy7932UJn4vbtL9Mv/3kdDEuFifqfZ6GiY7+glcihmorOUkNRN1zXvgt/vTOjQ2piZk3IpsDpk4oEYMHHh3qqgNzjIRpHuaB5iT9iXtBGBwzenDzIgpKOr8XS/EoGZkE10VFALfLB3JpRdGWgx6rCR31mCaCLkN7ZKo6ZAv+tZAxP6S+TYULHO8TgQMQOIC/e3RGnv4A2U0QFSD000rUCvUm3K0/MEBKUffcNmO0njOsDwZBIcjMDywfXksYRDHmuFwkPCLIc38klq20OFrqiMT6D90By1o1P+/si 41QXOg8n Te1xmGhBH2AUtKZieSJFazTnV6H7lJWrsUB76TEiyifeTxHcx+C08SNZdOK+wcrtSYK38hvFjmCpHVVl3v0H8s/xW61Nu27+lD9LKPpu5b7zJGjmTYiYYtb6RtCQyRofUAklalphouBPdxiV3E77H77cPHJ13vsXn6akmdUfs20z5md68LWDqRd9FVg4HQW6MfBmyL7nglWD5SoG1tp4KoHABBH7sEf0HzX58qe3kNWlEZYb5Y4/5nl1r6UkF0jQhlQeu/GHEputMEM8NVzyukdbj2cnzgRrzYdfTOfvj0HXdd1AbH/xyxIS1HYasNFti0OgZiS5bOk3NPDrcY+lzWdkDQ+QQCalMdnuLu1nxtwdx1S0= 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 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 AM 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_HUGE, >> which, when present, always implies PMD-sized THP, which is the same as >> 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 avoid. >> 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 entry from >> @@ -590,15 +589,6 @@ static inline int add_swap_extent(struct swap_info_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 pages, so >> * we can skip unlocking it below, since PG_locked is transferred 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_cluster_info *info) >> info->data = 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 &= ~CLUSTER_FLAG_HUGE; >> -} >> - >> static inline struct swap_cluster_info *lock_cluster(struct swap_info_struct *si, >> unsigned long offset) >> { >> @@ -1027,7 +1015,7 @@ static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) >> offset = idx * SWAPFILE_CLUSTER; >> ci = 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_entry_t entry) >> >> ci = lock_cluster_or_swap_info(si, offset); >> if (size == SWAPFILE_CLUSTER) { >> - VM_BUG_ON(!cluster_is_huge(ci)); >> map = si->swap_map + offset; >> for (i = 0; i < SWAPFILE_CLUSTER; i++) { >> val = map[i]; >> @@ -1373,7 +1360,6 @@ void put_swap_folio(struct folio *folio, swp_entry_t entry) >> if (val == SWAP_HAS_CACHE) >> free_entries++; >> } >> - cluster_clear_huge(ci); >> if (free_entries == 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_entry_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 = swp_offset(entry); >> - >> - si = _swap_info_get(entry); >> - if (!si) >> - return -EBUSY; >> - ci = 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 = ent1, *e2 = 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 = si->swap_map; >> unsigned long roffset = swp_offset(entry); >> - unsigned long offset = round_down(roffset, SWAPFILE_CLUSTER); >> + unsigned long offset = 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, then generate nr_pages (= 1 << order) inside the function. But given the function 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 strong objection. > >> int i; >> bool ret = false; >> >> ci = lock_cluster_or_swap_info(si, offset); >> - if (!ci || !cluster_is_huge(ci)) { >> + if (!ci || nr_pages == 1) { >> if (swap_count(map[roffset])) >> ret = true; >> goto unlock_out; >> } >> - for (i = 0; i < SWAPFILE_CLUSTER; i++) { >> + for (i = 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 :) > > Chris > >> if (swap_count(map[offset + i])) { >> ret = true; >> break; >> @@ -1556,7 +1526,7 @@ static bool folio_swapped(struct folio *folio) >> if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!folio_test_large(folio))) >> return swap_swapcount(si, entry) != 0; >> >> - return swap_page_trans_huge_swapped(si, entry); >> + return swap_page_trans_huge_swapped(si, entry, folio_nr_pages(folio)); >> } >> >> /** >> @@ -1622,8 +1592,7 @@ int free_swap_and_cache(swp_entry_t entry) >> } >> >> count = __swap_entry_free(p, entry); >> - if (count == SWAP_HAS_CACHE && >> - !swap_page_trans_huge_swapped(p, entry)) >> + if (count == SWAP_HAS_CACHE) >> __try_to_reclaim_swap(p, swp_offset(entry), >> TTRS_UNMAPPED | TTRS_FULL); >> put_swap_device(p); >> -- >> 2.25.1 >>