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 CFE15C54E67 for ; Thu, 14 Mar 2024 13:43:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4DDD8800A7; Thu, 14 Mar 2024 09:43:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4B56B80073; Thu, 14 Mar 2024 09:43:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3A3E0800A7; Thu, 14 Mar 2024 09:43:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 2BAD080073 for ; Thu, 14 Mar 2024 09:43:45 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id C982540923 for ; Thu, 14 Mar 2024 13:43:44 +0000 (UTC) X-FDA: 81895762368.17.CF578DA Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf05.hostedemail.com (Postfix) with ESMTP id 10A48100026 for ; Thu, 14 Mar 2024 13:43:41 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf05.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=1710423822; 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=an9yv2DD+c8glXZy6KljzpLJUcWhTR0TsrRi/EJUEMg=; b=af9bTJj884vlVe1cpPvG391FxHKbQeFRU9S7OIam1x/Q/I51vEJQBFO5wCXuw3rPff6n1g LHTJ3m8MeR4RYmYtM3fTPyl1QX2uQY5HmqCVcuEBCcUiNuTmAQA09TDVaX/SbUg8zhhaUX 9qZUqyThpbEmDZrkyCemzBpMj74MyYI= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf05.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=1710423822; a=rsa-sha256; cv=none; b=r7+LL0gB3DAxdyci95tmTehpd0FshDFMmh4ROR4yD900vyiHPQbeEEWSAocW1xho8Y5veP uSZ0GG9+Aa39FCzY14Jlpl7M6sRJ659JHvj5wlFxLkwTZajgiK8MhBWdMB0Dbqd6nIl3mO aS1/JWG9iMnKN0U2+mti1mL8f85/GsY= 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 0E8B61007; Thu, 14 Mar 2024 06:44:17 -0700 (PDT) Received: from [10.1.30.182] (XHFQ2J9959.cambridge.arm.com [10.1.30.182]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A83463F762; Thu, 14 Mar 2024 06:43:36 -0700 (PDT) Message-ID: <716bb29c-d2a2-4eef-b300-b037f08f458f@arm.com> Date: Thu, 14 Mar 2024 13:43:34 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH v3 2/5] mm: swap: introduce swap_nr_free() for batched swap_free() Content-Language: en-GB To: Chuanhua Han Cc: Barry Song <21cnbao@gmail.com>, akpm@linux-foundation.org, linux-mm@kvack.org, chengming.zhou@linux.dev, chrisl@kernel.org, david@redhat.com, hannes@cmpxchg.org, kasong@tencent.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mhocko@suse.com, nphamcs@gmail.com, shy828301@gmail.com, steven.price@arm.com, surenb@google.com, wangkefeng.wang@huawei.com, willy@infradead.org, xiang@kernel.org, ying.huang@intel.com, yosryahmed@google.com, yuzhao@google.com, Chuanhua Han , Barry Song References: <20240304081348.197341-1-21cnbao@gmail.com> <20240304081348.197341-3-21cnbao@gmail.com> <499a60c6-eeb8-4bbd-8563-9717c0d2e43d@arm.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 10A48100026 X-Stat-Signature: tmp8jcsu9n3w3snnkeppkcskdca4iuxk X-HE-Tag: 1710423821-862969 X-HE-Meta: U2FsdGVkX19iHWMcZOR9hcbvgn6zhayx27mc+//cAmmNnXiNUz9y1/Hn9qKoKVzWxUn00yOgxiWwxqmOnT6/KAkaTLmj6t5jEoWQldLbcZxolCCQU9pLDiIECxoxBYYIlCbmSEQOfZ8s29L2Zo90HUiTv59qwD0wTZUdCbRDYRnT5yx8OBs6RsVCfVpitiDo+sneXyTCibojLaL3LPh1FIlKlOhI59S2UAcvJNYy108EdRD+tAaKLHS9kpVIGGFuLJD6+8FXcul3/bYt8wAzSRCLcur+swPxQYpo73miNC/i9zS1l09iLptv/fImJfdTLW2DECnL7lDs0X63WlORlb7m4tjRS3AUvIHrOEE0c/kOyCE7rpV+9mNwY7w/XqeAToda1akRCo+5Tb/EvNcZgR++F3Fp5kxKV9c0Kd/zmQRxEDfg35EuPaM9UNGu1wSKDzrceFeCVktlN3xwusVqYr5iaK1Ni3TTgzBI8Tg4ZTg35bwWmRY2q1vcEKjb+rWdZDAEfIPBXKK3/IESyE/nnMIeeg/QTdfj+6bv6NxeAxg83aXewL8+f4d5/v0BgZWjnzG85X/bz+ITI2VSR7LZaT32jmUPkt0LdxcxnY23dDh8YV2cUezqIweCJXfIM50qUvBFdY7w91fa9qeJjOsAPYJwqHzTSbCHee5sGMFXPLkUzhwN6O+jkmKI8xlZ0DzVNhOTrPvn6Y+DEHRizxM08cjp4T0XLxogNKtLpbEmv7aqlreFJl70jF+Jtw+artQnLggt3Fq3e6hEgrhRucnLjrc2PVrs72WlyBcGG3j+yTEiw9BLlaR3UHEHg5TTj5TEIxq3HSrH9GeFlJpec8t4jrn6F0eQCa1ELTC0nP3bm4h/FwV2nS4laZf2jaAA6wbUOulXsU0rhIB8elrW8GAS4JWzw+2yc0+NUWPkxevZHmxVrBGNbWP2LbJtRfxL+61Iranq3TND9mtf0gTG+Y0 2btJqAle PVHegsBOGtNExn+fzC3GL/jXKJ1HWiNhwxjwYXN7Hv7D4TPudgo90GACnEtLuTK6yymt1A40AeWRea7CJHi5vauRRtTFJV4Mat+svPjDhu3CQJ+RaD4KnNfKJAh8paA54UwyFDoRX/twD3CRTAkhzMSZp66Wd52K7btEifQuxH0M1XdYR6PuRFd1CmnhaSQmBLoPVHc6W2LIR3KqGQwYqoUcqCKnCT89T7TKl9q1fppupCmUCxSv9wvG5HsPzONe4t8Lr4ahF3TmH/t5rXcmd0bQywewLnSZy/dwgLrJZf2Gz3ttClhTSbZD3EE8ZYK9cNilOg2Pc3OH8PF9OpojopGFnIg== 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 14/03/2024 13:12, Chuanhua Han wrote: > Ryan Roberts 于2024年3月12日周二 02:51写道： >> >> On 04/03/2024 08:13, Barry Song wrote: >>> 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 | 6 ++++++ >>> mm/swapfile.c | 35 +++++++++++++++++++++++++++++++++++ >>> 2 files changed, 41 insertions(+) >>> >>> diff --git a/include/linux/swap.h b/include/linux/swap.h >>> index 2955f7a78d8d..d6ab27929458 100644 >>> --- a/include/linux/swap.h >>> +++ b/include/linux/swap.h >>> @@ -481,6 +481,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_nr_free(swp_entry_t entry, int nr_pages); >> >> nit: In my swap-out v4 series, I've created a batched version of >> free_swap_and_cache() and called it free_swap_and_cache_nr(). Perhaps it is >> preferable to align the naming schemes - i.e. call this swap_free_nr(). Your >> scheme doesn't really work when applied to free_swap_and_cache(). > Thanks for your suggestions, and for the next version, we'll see which > package is more appropriate! >> >>> extern void swapcache_free_entries(swp_entry_t *entries, int n); >>> extern int free_swap_and_cache(swp_entry_t); >>> int swap_type_of(dev_t device, sector_t offset); >>> @@ -561,6 +562,11 @@ static inline void swap_free(swp_entry_t swp) >>> { >>> } >>> >>> +void swap_nr_free(swp_entry_t entry, int nr_pages) >>> +{ >>> + >>> +} >>> + >>> static inline void put_swap_folio(struct folio *folio, swp_entry_t swp) >>> { >>> } >>> diff --git a/mm/swapfile.c b/mm/swapfile.c >>> index 3f594be83b58..244106998a69 100644 >>> --- a/mm/swapfile.c >>> +++ b/mm/swapfile.c >>> @@ -1341,6 +1341,41 @@ void swap_free(swp_entry_t entry) >>> __swap_entry_free(p, entry); >>> } >>> >>> +/* >>> + * Called after swapping in a large folio, batched free swap entries >>> + * for this large folio, entry should be for the first subpage and >>> + * its offset is aligned with nr_pages >>> + */ >>> +void swap_nr_free(swp_entry_t entry, int nr_pages) >>> +{ >>> + int i; >>> + struct swap_cluster_info *ci; >>> + struct swap_info_struct *p; >>> + unsigned type = swp_type(entry); >> >> nit: checkpatch.py will complain about bare "unsigned", preferring "unsigned >> int" or at least it did for me when I did something similar in my swap-out patch >> set. > Gee, thanks for pointing that out! >> >>> + unsigned long offset = swp_offset(entry); >>> + DECLARE_BITMAP(usage, SWAPFILE_CLUSTER) = { 0 }; >> >> I don't love this, as it could blow the stack if SWAPFILE_CLUSTER ever >> increases. But the only other way I can think of is to explicitly loop over >> fixed size chunks, and that's not much better. > Is it possible to save kernel stack better by using bit_map here? If > SWAPFILE_CLUSTER=512, we consume only (512/64)*8= 64 bytes. I'm not sure I've understood what you are saying? You're already using DECLARE_BITMAP(), so its already consuming 64 bytes if SWAPFILE_CLUSTER=512, no? I actually did a bad job of trying to express a couple of different points: - Are there any configurations today where SWAPFILE_CLUSTER > 512? I'm not sure. Certainly not for arm64, but not sure about other architectures. For example if an arch had 64K pages with 8192 entries per THP and supports SWAP_THP, that's 1K for the bitmap, which is now looking pretty big for the stack. - Would it be better to decouple stack usage from SWAPFILE_CLUSTER and instead define a fixed stack size (e.g. 64 bytes -> 512 entries). Then free the range of entries in batches no bigger than this size. This approach could also allow removing the constraint that the range has to be aligned and fit in a single cluster. Personally I think an approach like this would be much more robust, in return for a tiny bit more complexity. >> >>> + >>> + /* all swap entries are within a cluster for mTHP */ >>> + VM_BUG_ON(offset % SWAPFILE_CLUSTER + nr_pages > SWAPFILE_CLUSTER); >>> + >>> + if (nr_pages == 1) { >>> + swap_free(entry); >>> + return; >>> + } >>> + >>> + p = _swap_info_get(entry); >> >> You need to handle this returning NULL, like swap_free() does. > Yes, you're right! We did forget to judge NULL here. >> >>> + >>> + ci = lock_cluster(p, offset); >> >> The existing swap_free() calls lock_cluster_or_swap_info(). So if swap is backed >> by rotating media, and clusters are not in use, it will lock the whole swap >> info. But your new version only calls lock_cluster() which won't lock anything >> if clusters are not in use. So I think this is a locking bug. > Again, you're right, it's bug! >> >>> + for (i = 0; i < nr_pages; i++) { >>> + if (__swap_entry_free_locked(p, offset + i, 1)) >>> + __bitmap_set(usage, i, 1); >>> + } >>> + unlock_cluster(ci); >>> + >>> + for_each_clear_bit(i, usage, nr_pages) >>> + free_swap_slot(swp_entry(type, offset + i)); >>> +} >>> + >>> /* >>> * Called after dropping swapcache to decrease refcnt to swap entries. >>> */ >> >> Thanks, >> Ryan >> >> > >