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 12F01C3DA64 for ; Thu, 1 Aug 2024 02:43:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7C0666B0088; Wed, 31 Jul 2024 22:43:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 747386B00AB; Wed, 31 Jul 2024 22:43:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 54C1A6B00AD; Wed, 31 Jul 2024 22:43:05 -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 2CCE36B0088 for ; Wed, 31 Jul 2024 22:43:05 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id C8A18A60A7 for ; Thu, 1 Aug 2024 02:43:04 +0000 (UTC) X-FDA: 82402129488.25.3AF913B Received: from mail-vk1-f180.google.com (mail-vk1-f180.google.com [209.85.221.180]) by imf26.hostedemail.com (Postfix) with ESMTP id EFB55140012 for ; Thu, 1 Aug 2024 02:43:02 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="Dg/JEQdY"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.180 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722480178; a=rsa-sha256; cv=none; b=oS4XNtoOmnihkNj13378GdvLMf4Il//M9SLYJwTMD0L4xX5fR1T42vAj6xyz3FpjIOSz9q yzsufW6CZU4ag1YyVlqGAcE6LUjMGotDBR5OoEhaYg3sGOgbwPnIctKCuLOefTYLO/3udd 83KEq4aAo1B9VNAni97jRsvg6+DaBZ0= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="Dg/JEQdY"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.180 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=1722480178; 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=Rakkb5FBCQKqLPVAz3DMrKm2zFVKJm0vW4XKM6tVmCw=; b=P/AQbfTj3tfbflJ4MZfQoRgFkQ0jCp9LPvMRIDj1b3IcFUBNipxgD3nLYl84u576r3156v 1Vhvmn7lCy58R7XrwGmWpzJ2PBRZUTPYpiZcHZ+XGqBjO9U2zfQa79LtoDDeKBev0k1Bes Pv6PJW79DsDxCdY3aTKvwoWPtPOOyS8= Received: by mail-vk1-f180.google.com with SMTP id 71dfb90a1353d-4f6d35d59ccso2016575e0c.1 for ; Wed, 31 Jul 2024 19:43:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1722480182; x=1723084982; 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=Rakkb5FBCQKqLPVAz3DMrKm2zFVKJm0vW4XKM6tVmCw=; b=Dg/JEQdY/fAGGogNzQ96vfspMQUBFr0yTUhpJ4O0bk+SlX15ZTXZm6fWgz19z0es/r nib2QCrP/kcF+BYWuCkWth73nECEVuj6UMbB/2ZARi/2LtPHSm0VoADvpLt9FVdBSTvI lV/URDfoIkQ83pXFyuLnYjUZHaSfoN+IzDB8GTi8wb55B37MZXUuuqckfBJRTqp0uHQ9 PhkcDNYnTEiJbaOc5dtA/OFrjT9tP+Ia1qsdKlyWWDOhKaFOJRiU6sOfZk+cawktcmNJ LDYa4dizmdp27Wyut8OJAh9LKknIY4pMG05FivfelJ6R9QBCijhGnIMLX9VuGQ/dqmvy 2aUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722480182; x=1723084982; 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=Rakkb5FBCQKqLPVAz3DMrKm2zFVKJm0vW4XKM6tVmCw=; b=ignQcTRhDs5/xRcBHBfpdBpcQMW90SA66EVNYCQi037Y0w0dPMrv9/BqKe+IA0pZK7 qgOBXsdHJiHPesGysmI50AnddwiNpm+uUYfPrQt1htixuK/WnYLJD8E/Ie6v+at/SVcJ zHJN8zoYsP4OCDiYDUKOu0z7EDRTfKddBT6rCg27mC/RGfndDDiGVDydmUKxUmgBBaQZ tzM0Fup03//1J2xZoxaySKEGXOAO9zHtH6yalPRVMyhggYruMmBfzVteCvp5CAI4mG5r 1aLdtSGN/j1FvXqC2PVEm2moW/RtXN3b1IqsmrDED+m+Y/0kjQavgJD/h2E4nDuKUtWW Ev1g== X-Forwarded-Encrypted: i=1; AJvYcCXWX9ockLjqQhDQ7Z/KcNTTLvJT2JQbTS744KANcttOFQkBW30l8FzVMy+x3i+xY/TH6GnSdbOM6L+Zigjk1A6HbGs= X-Gm-Message-State: AOJu0Ywdq0wYkungx1EyO8VWnIEGad3R3ht3bxo1e+YyXsS0Wmg4tVdH bj8IhJnnRA0D/KKqg9CfMm93BRX/fpVZAP99efXM3mNpykEGtXByZ3Z/8xlMjyolOjCPUrPx/ow wekXEOQFpTSRlCHlB1a82iyT7Eys= X-Google-Smtp-Source: AGHT+IFFtfyzDy7nHkIlV9+7nEx+8AuXTEheU1GQ/rLmeoZtQoEV+H4CfKlj7a8Lf5b6fTMKD/aNM3Plst94TJHmCGc= X-Received: by 2002:a05:6122:2507:b0:4ef:6865:8ffd with SMTP id 71dfb90a1353d-4f8965f01a1mr1630272e0c.10.1722480181782; Wed, 31 Jul 2024 19:43:01 -0700 (PDT) MIME-Version: 1.0 References: <20240730071339.107447-1-21cnbao@gmail.com> <20240730071339.107447-2-21cnbao@gmail.com> <874j86oubf.fsf@yhuang6-desk2.ccr.corp.intel.com> <87zfpynf2r.fsf@yhuang6-desk2.ccr.corp.intel.com> <871q39kq0e.fsf@yhuang6-desk2.ccr.corp.intel.com> <87wml1j7l9.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: <87wml1j7l9.fsf@yhuang6-desk2.ccr.corp.intel.com> From: Barry Song <21cnbao@gmail.com> Date: Thu, 1 Aug 2024 10:42:49 +0800 Message-ID: Subject: Re: [PATCH 1/1] mm: swap: add nr argument in swapcache_prepare and swapcache_clear to support large folios To: "Huang, Ying" Cc: akpm@linux-foundation.org, linux-mm@kvack.org, baolin.wang@linux.alibaba.com, chrisl@kernel.org, david@redhat.com, hannes@cmpxchg.org, hughd@google.com, kaleshsingh@google.com, kasong@tencent.com, linux-kernel@vger.kernel.org, mhocko@suse.com, minchan@kernel.org, nphamcs@gmail.com, ryan.roberts@arm.com, senozhatsky@chromium.org, shakeel.butt@linux.dev, shy828301@gmail.com, surenb@google.com, v-songbaohua@oppo.com, willy@infradead.org, xiang@kernel.org, yosryahmed@google.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: EFB55140012 X-Rspamd-Server: rspam01 X-Stat-Signature: pdfdpdwz33paxja87eewzgwiorru6r7d X-HE-Tag: 1722480182-663233 X-HE-Meta: U2FsdGVkX1+qIwZYtrGwT6lByKLvAPT1ox8y6kEz/JlYVngDQPA3s6kDQYIGRI3+I6jhyWjCWJPpp+QnwO3bNyWn+DbTerNxUNmoUjXxaemwQXBBxenC7XlDFOeTgeS87ArsOG7QSKh83Z9jR0l00Vz2cmbvPkwkpV9YliWPv0q3HzUwsL06aSt/X+72y6ESSaMSN3EeGuX8iqMAv9INm8sAttV/9hnTF/HIyLd66erjZLTb5MFSjB48jDYZG0eRwU5Q/D7i+GozRvPonDy3bVXim53tT/tya8xX65unxKb/vXQ+ory+7dDmGjnCaLjrQ8esMouuloSU8PBFWMP3zB8yj8UDVGAhXv3k2or3c48FHy84k/N77ScncTIRb3G5CRqdC+TPd3ty/4s2nrvZX2ltOgaoFSSgUVgkm2aOMrnllBm9RAMj1yVHoXaqBPEZFr3nNt367YVqsDjSG+jq8YB0xmMfcFn2P9jwqAnIsPxken/FGuS5xxKhRkOU0H3pm/6AOOzABB4E2ACNYbZojmOROP0UBvH3y/sgIFa7QIw+KzTrbQk3qN7qhIVWJjT/T/0ikgDUZ8KXQKPbfjKAj8uZJaFt4AxHl7S8vOPAlPWi5ualP9zkg1cP+N4iRGTGR9yywuWy+J7PX30IuTQ/vJW/l28qPrjf4PNewC3noGX9R0g2OSGyfqijVwWfYnaRE1YwgxvQyaD0QIV/ApITinvcu1aY79oS5VJqJdA/7VS7kX7cQbbH2aCzZGxGDLeN4tOtQotLsDvpABCcf3WmJYRkUempbOARvoAoJpJG7vrSmGmroMdaL+crMbqkPxSS19VsYkLWLq1QUpjpAVBp4CoBJs11G0x2bGWRcphJdluyhVoXza10RihjS7nnhcrKAuIB5/tZ7upp+mS6uhDOtiiAMre+GASrcCzDQ24IUI1dDhKq375I2b527+FGUarTiKgWX2yh4ZyJnaPbB54 nBjnVelK EHRCfJt3IgIhZyJHlzRamcVa9QPKHAU8674TK+LX3TbKMf7jG3P8e1gSC5YhkBwEgY6ndAfJl7ULZyR6rjU+/UaBDpz17FG+42Tmr2tc02HjvkXluWaRuFm6gOgdtWcVB79wRDmxI4KIjqwGbHljPVRsaS+0TAOaLx/2iyjqARZczSrpAFMz1Vo+MP071MY4iEkW5fSqoHRIvSbLTEi8Fd9mwSckR8np2gWkGztec2IHxEIuH+JCRCxD116bVf8EniYICvMRDPTQZ1tbVGwtLTXktJEarEIgo6lKKOWjN+j3wZjfZVD03GrHyPO5WpNQJ7rSQDAznMmlP7Wo4LCQkwh/pZsMSdErUtmq/26X1YzE+qgclg7ddRdJHLQth6x4DT0jfMatu20eH83yKFR18OehuKY0yqQJoNzFdCKljxZ3l2obZkXnjHwKI2/QO9yvWxa3a8r+P0dF1F6KQ2IHTE8w7eCICxuNZ7DrdWp6+fqI7DoEd1nlwnH8eNQ== 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, Aug 1, 2024 at 10:37=E2=80=AFAM Huang, Ying = wrote: > > Barry Song <21cnbao@gmail.com> writes: > > > On Thu, Aug 1, 2024 at 9:13=E2=80=AFAM Huang, Ying wrote: > >> > >> Barry Song <21cnbao@gmail.com> writes: > >> > >> > On Wed, Jul 31, 2024 at 4:28=E2=80=AFPM Huang, Ying wrote: > >> >> > >> >> Barry Song <21cnbao@gmail.com> writes: > >> >> > >> >> > On Wed, Jul 31, 2024 at 4:14=E2=80=AFPM Huang, Ying wrote: > >> >> >> > >> >> >> Hi, Barry, > >> >> >> > >> >> >> Barry Song <21cnbao@gmail.com> writes: > >> >> >> > >> >> >> > From: Barry Song > >> >> >> > > >> >> >> > Right now, swapcache_prepare() and swapcache_clear() supports = one entry > >> >> >> > only, to support large folios, we need to handle multiple swap= entries. > >> >> >> > > >> >> >> > To optimize stack usage, we iterate twice in __swap_duplicate(= ): the > >> >> >> > first time to verify that all entries are valid, and the secon= d time > >> >> >> > to apply the modifications to the entries. > >> >> >> > > >> >> >> > Currently, we're using nr=3D1 for the existing users. > >> >> >> > > >> >> >> > Reviewed-by: Baolin Wang > >> >> >> > Signed-off-by: Barry Song > >> >> >> > --- > >> >> >> > include/linux/swap.h | 4 +- > >> >> >> > mm/memory.c | 6 +-- > >> >> >> > mm/swap.h | 5 ++- > >> >> >> > mm/swap_state.c | 2 +- > >> >> >> > mm/swapfile.c | 101 +++++++++++++++++++++++++---------= --------- > >> >> >> > 5 files changed, 68 insertions(+), 50 deletions(-) > >> >> >> > > >> >> >> > diff --git a/include/linux/swap.h b/include/linux/swap.h > >> >> >> > index ba7ea95d1c57..5b920fa2315b 100644 > >> >> >> > --- a/include/linux/swap.h > >> >> >> > +++ b/include/linux/swap.h > >> >> >> > @@ -480,7 +480,7 @@ extern int get_swap_pages(int n, swp_entry= _t swp_entries[], int order); > >> >> >> > extern int add_swap_count_continuation(swp_entry_t, gfp_t); > >> >> >> > extern void swap_shmem_alloc(swp_entry_t); > >> >> >> > extern int swap_duplicate(swp_entry_t); > >> >> >> > -extern int swapcache_prepare(swp_entry_t); > >> >> >> > +extern int swapcache_prepare(swp_entry_t entry, int nr); > >> >> >> > 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)= ; > >> >> >> > @@ -554,7 +554,7 @@ static inline int swap_duplicate(swp_entry= _t swp) > >> >> >> > return 0; > >> >> >> > } > >> >> >> > > >> >> >> > -static inline int swapcache_prepare(swp_entry_t swp) > >> >> >> > +static inline int swapcache_prepare(swp_entry_t swp, int nr) > >> >> >> > { > >> >> >> > return 0; > >> >> >> > } > >> >> >> > diff --git a/mm/memory.c b/mm/memory.c > >> >> >> > index 833d2cad6eb2..b8675617a5e3 100644 > >> >> >> > --- a/mm/memory.c > >> >> >> > +++ b/mm/memory.c > >> >> >> > @@ -4081,7 +4081,7 @@ vm_fault_t do_swap_page(struct vm_fault = *vmf) > >> >> >> > * reusing the same entry. It's undetect= able as > >> >> >> > * pte_same() returns true due to entry = reuse. > >> >> >> > */ > >> >> >> > - if (swapcache_prepare(entry)) { > >> >> >> > + if (swapcache_prepare(entry, 1)) { > >> >> >> > /* Relax a bit to prevent rapid = repeated page faults */ > >> >> >> > schedule_timeout_uninterruptible= (1); > >> >> >> > goto out; > >> >> >> > @@ -4387,7 +4387,7 @@ vm_fault_t do_swap_page(struct vm_fault = *vmf) > >> >> >> > out: > >> >> >> > /* Clear the swap cache pin for direct swapin after PTL = unlock */ > >> >> >> > if (need_clear_cache) > >> >> >> > - swapcache_clear(si, entry); > >> >> >> > + swapcache_clear(si, entry, 1); > >> >> >> > if (si) > >> >> >> > put_swap_device(si); > >> >> >> > return ret; > >> >> >> > @@ -4403,7 +4403,7 @@ vm_fault_t do_swap_page(struct vm_fault = *vmf) > >> >> >> > folio_put(swapcache); > >> >> >> > } > >> >> >> > if (need_clear_cache) > >> >> >> > - swapcache_clear(si, entry); > >> >> >> > + swapcache_clear(si, entry, 1); > >> >> >> > if (si) > >> >> >> > put_swap_device(si); > >> >> >> > return ret; > >> >> >> > diff --git a/mm/swap.h b/mm/swap.h > >> >> >> > index baa1fa946b34..7c6330561d84 100644 > >> >> >> > --- a/mm/swap.h > >> >> >> > +++ b/mm/swap.h > >> >> >> > @@ -59,7 +59,7 @@ void __delete_from_swap_cache(struct folio *= folio, > >> >> >> > void delete_from_swap_cache(struct folio *folio); > >> >> >> > void clear_shadow_from_swap_cache(int type, unsigned long beg= in, > >> >> >> > unsigned long end); > >> >> >> > -void swapcache_clear(struct swap_info_struct *si, swp_entry_t= entry); > >> >> >> > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t= entry, int nr); > >> >> >> > struct folio *swap_cache_get_folio(swp_entry_t entry, > >> >> >> > struct vm_area_struct *vma, unsigned long addr); > >> >> >> > struct folio *filemap_get_incore_folio(struct address_space *= mapping, > >> >> >> > @@ -120,7 +120,7 @@ static inline int swap_writepage(struct pa= ge *p, struct writeback_control *wbc) > >> >> >> > return 0; > >> >> >> > } > >> >> >> > > >> >> >> > -static inline void swapcache_clear(struct swap_info_struct *s= i, swp_entry_t entry) > >> >> >> > +static inline void swapcache_clear(struct swap_info_struct *s= i, swp_entry_t entry, int nr) > >> >> >> > { > >> >> >> > } > >> >> >> > > >> >> >> > @@ -172,4 +172,5 @@ static inline unsigned int folio_swap_flag= s(struct folio *folio) > >> >> >> > return 0; > >> >> >> > } > >> >> >> > #endif /* CONFIG_SWAP */ > >> >> >> > + > >> >> >> > >> >> >> NITPICK: Is it necessary to add a blank line here? But I don't = think a > >> >> >> new version is necessary if this is the only change needed. > >> >> > > >> >> > No need to add a blank line; it was probably a mistake I made in = Vim. > >> >> > > >> >> >> > >> >> >> > #endif /* _MM_SWAP_H */ > >> >> >> > diff --git a/mm/swap_state.c b/mm/swap_state.c > >> >> >> > index a1726e49a5eb..b06f2a054f5a 100644 > >> >> >> > --- a/mm/swap_state.c > >> >> >> > +++ b/mm/swap_state.c > >> >> >> > @@ -477,7 +477,7 @@ struct folio *__read_swap_cache_async(swp_= entry_t entry, gfp_t gfp_mask, > >> >> >> > /* > >> >> >> > * Swap entry may have been freed since our call= er observed it. > >> >> >> > */ > >> >> >> > - err =3D swapcache_prepare(entry); > >> >> >> > + err =3D swapcache_prepare(entry, 1); > >> >> >> > if (!err) > >> >> >> > break; > >> >> >> > > >> >> >> > diff --git a/mm/swapfile.c b/mm/swapfile.c > >> >> >> > index 5f73a8553371..757d38a86f56 100644 > >> >> >> > --- a/mm/swapfile.c > >> >> >> > +++ b/mm/swapfile.c > >> >> >> > @@ -3363,7 +3363,7 @@ void si_swapinfo(struct sysinfo *val) > >> >> >> > } > >> >> >> > > >> >> >> > /* > >> >> >> > - * Verify that a swap entry is valid and increment its swap m= ap count. > >> >> >> > + * Verify that nr swap entries are valid and increment their = swap map counts. > >> >> >> > * > >> >> >> > * Returns error code in following case. > >> >> >> > * - success -> 0 > >> >> >> > @@ -3373,60 +3373,77 @@ void si_swapinfo(struct sysinfo *val) > >> >> >> > * - swap-cache reference is requested but the entry is not u= sed. -> ENOENT > >> >> >> > * - swap-mapped reference requested but needs continued swap= count. -> ENOMEM > >> >> >> > */ > >> >> >> > -static int __swap_duplicate(swp_entry_t entry, unsigned char = usage) > >> >> >> > +static int __swap_duplicate(swp_entry_t entry, unsigned char = usage, int nr) > >> >> >> > { > >> >> >> > struct swap_info_struct *p; > >> >> >> > struct swap_cluster_info *ci; > >> >> >> > unsigned long offset; > >> >> >> > unsigned char count; > >> >> >> > unsigned char has_cache; > >> >> >> > - int err; > >> >> >> > + int err, i; > >> >> >> > > >> >> >> > p =3D swp_swap_info(entry); > >> >> >> > > >> >> >> > offset =3D swp_offset(entry); > >> >> >> > + VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLU= STER); > >> >> >> > ci =3D lock_cluster_or_swap_info(p, offset); > >> >> >> > > >> >> >> > - count =3D p->swap_map[offset]; > >> >> >> > + err =3D 0; > >> >> >> > + for (i =3D 0; i < nr; i++) { > >> >> >> > + count =3D p->swap_map[offset + i]; > >> >> >> > > >> >> >> > - /* > >> >> >> > - * swapin_readahead() doesn't check if a swap entry is v= alid, so the > >> >> >> > - * swap entry could be SWAP_MAP_BAD. Check here with loc= k held. > >> >> >> > - */ > >> >> >> > - if (unlikely(swap_count(count) =3D=3D SWAP_MAP_BAD)) { > >> >> >> > - err =3D -ENOENT; > >> >> >> > - goto unlock_out; > >> >> >> > - } > >> >> >> > + /* > >> >> >> > + * swapin_readahead() doesn't check if a swap en= try is valid, so the > >> >> >> > + * swap entry could be SWAP_MAP_BAD. Check here = with lock held. > >> >> >> > + */ > >> >> >> > + if (unlikely(swap_count(count) =3D=3D SWAP_MAP_B= AD)) { > >> >> >> > + err =3D -ENOENT; > >> >> >> > + goto unlock_out; > >> >> >> > + } > >> >> >> > > >> >> >> > - has_cache =3D count & SWAP_HAS_CACHE; > >> >> >> > - count &=3D ~SWAP_HAS_CACHE; > >> >> >> > - err =3D 0; > >> >> >> > + has_cache =3D count & SWAP_HAS_CACHE; > >> >> >> > + count &=3D ~SWAP_HAS_CACHE; > >> >> >> > > >> >> >> > - if (usage =3D=3D SWAP_HAS_CACHE) { > >> >> >> > + if (usage =3D=3D SWAP_HAS_CACHE) { > >> >> >> > + /* set SWAP_HAS_CACHE if there is no cac= he and entry is used */ > >> >> >> > + if (!has_cache && count) > >> >> >> > + continue; > >> >> >> > + else if (has_cache) /* someo= ne else added cache */ > >> >> >> > + err =3D -EEXIST; > >> >> >> > + else /* no us= ers remaining */ > >> >> >> > + err =3D -ENOENT; > >> >> >> > > >> >> >> > - /* set SWAP_HAS_CACHE if there is no cache and e= ntry is used */ > >> >> >> > - if (!has_cache && count) > >> >> >> > - has_cache =3D SWAP_HAS_CACHE; > >> >> >> > - else if (has_cache) /* someone else = added cache */ > >> >> >> > - err =3D -EEXIST; > >> >> >> > - else /* no users rema= ining */ > >> >> >> > - err =3D -ENOENT; > >> >> >> > + } else if (count || has_cache) { > >> >> >> > > >> >> >> > - } else if (count || has_cache) { > >> >> >> > + if ((count & ~COUNT_CONTINUED) < SWAP_MA= P_MAX) > >> >> >> > + continue; > >> >> >> > + else if ((count & ~COUNT_CONTINUED) > SW= AP_MAP_MAX) > >> >> >> > + err =3D -EINVAL; > >> >> >> > + else if (swap_count_continued(p, offset = + i, count)) > >> >> >> > + continue; > >> >> >> > >> >> >> IIUC, this will make the change to swap map directly instead of > >> >> >> verification. If the verification failed for some entry later, = the > >> >> >> count will be wrong? Or I missed something? > >> >> > > >> >> > To avoid using a bitmap or a larger stack, we actually verify dur= ing > >> >> > the first iteration. > >> >> > This ensures that by the second iteration, we can safely commit t= he > >> >> > modification. > >> >> > > >> >> > I actually put some words in the changelog :-) > >> >> > > >> >> > To optimize stack usage, we iterate twice in __swap_duplicate(): = the > >> >> > first time to verify that all entries are valid, and the second t= ime > >> >> > to apply the modifications to the entries. > >> >> > >> >> Yes, I have seen it and I think that it is a good strategy. > >> >> > >> >> But, IIUC, swap_count_continued() will change the higher bits of th= e > >> >> swap_map instead of verifying. Or, my understanding is wrong? > >> >> > >> > > >> > Ying, your understanding is 100% correct. but the code also has noth= ing > >> > broken. we didn't extend swap_duplicate() to have argument nr, > >> > so all users which can set usage=3D1 will definitely have nr=3D1. > >> > > >> > int swap_duplicate(swp_entry_t entry) > >> > { > >> > int err =3D 0; > >> > > >> > while (!err && __swap_duplicate(entry, 1, 1) =3D=3D -ENOMEM) > >> > err =3D add_swap_count_continuation(entry, GFP_ATOMI= C); > >> > return err; > >> > } > >> > >> I understand that we don't have requirements to support "usage =3D=3D = 1 && > >> nr > 1" case for __swap_duplicate() at least for now. > >> > >> > Maybe I can add a VM_WARN_ON to warn those people who might > >> > want to extend swap_duplicate()? in that case, things could be quite > >> > tricky. > >> > > >> > --- a/mm/swapfile.c > >> > +++ b/mm/swapfile.c > >> > @@ -3386,6 +3386,7 @@ static int __swap_duplicate(swp_entry_t entry, > >> > unsigned char usage, int nr) > >> > > >> > offset =3D swp_offset(entry); > >> > VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER= ); > >> > + VM_WARN_ON(usage =3D=3D 1 && nr > 1); > >> > ci =3D lock_cluster_or_swap_info(p, offset); > >> > > >> > err =3D 0; > >> > >> Please add this. And, I think that we need to make it explicit in pat= ch > >> description and comments to avoid potential confusing. > > > > cool. make sense to me. I will post something for Andrew to squash into= . > > > >> > >> And, because it's hard to implement the verify and change strategy if > >> "usage =3D=3D 1". Can we only use that strategy for "usage =3D=3D > >> SWAP_HAS_CACHE"? > > > > I believe Baolin also needs the case for shmem. I don't feel a strong > > need to split two logics(1 and non-1) as the code will be quite ugly :-= ) > > Don't need to split like that, it could be something like > > for (i =3D 0; i < nr; i++) { > if (usage =3D=3D SWAP_HAS_CACHE) { > /* Only verify for SWAP_HAS_CACHE */ > } > } > > for (i =3D 0; i < nr; i++) { > if (usage =3D=3D SWAP_HAS_CACHE) { > } else { > /* Verify and change for usage =3D=3D 1 */ > } > } > but we also have cases where nr can be > 1 __swap_duplicate(entry, SWAP_MAP_SHMEM, 1); > -- > Best Regards, > Huang, Ying > > >> > >> >> >> > >> >> >> > + else > >> >> >> > + err =3D -ENOMEM; > >> >> >> > + } else > >> >> >> > + err =3D -ENOENT; /* unu= sed swap entry */ > >> >> >> > > >> >> >> > - if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX) > >> >> >> > + if (err) > >> >> >> > + goto unlock_out; > >> >> >> > + } > >> >> >> > + > >> >> >> > + for (i =3D 0; i < nr; i++) { > >> >> >> > + count =3D p->swap_map[offset + i]; > >> >> >> > + has_cache =3D count & SWAP_HAS_CACHE; > >> >> >> > + count &=3D ~SWAP_HAS_CACHE; > >> >> >> > + > >> >> >> > + if (usage =3D=3D SWAP_HAS_CACHE) > >> >> >> > + has_cache =3D SWAP_HAS_CACHE; > >> >> >> > + else if ((count & ~COUNT_CONTINUED) < SWAP_MAP_M= AX) > >> >> >> > count +=3D usage; > >> >> >> > - else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_M= AX) > >> >> >> > - err =3D -EINVAL; > >> >> >> > - else if (swap_count_continued(p, offset, count)) > >> >> >> > - count =3D COUNT_CONTINUED; > >> >> >> > else > >> >> >> > - err =3D -ENOMEM; > >> >> >> > - } else > >> >> >> > - err =3D -ENOENT; /* unused swap= entry */ > >> >> >> > + count =3D COUNT_CONTINUED; > >> >> >> > > >> >> >> > - if (!err) > >> >> >> > - WRITE_ONCE(p->swap_map[offset], count | has_cach= e); > >> >> >> > + WRITE_ONCE(p->swap_map[offset + i], count | has_= cache); > >> >> >> > + } > >> >> >> > > >> >> >> > unlock_out: > >> >> >> > unlock_cluster_or_swap_info(p, ci); > >> >> >> > @@ -3439,7 +3456,7 @@ static int __swap_duplicate(swp_entry_t = entry, unsigned char usage) > >> >> >> > */ > >> >> >> > void swap_shmem_alloc(swp_entry_t entry) > >> >> >> > { > >> >> >> > - __swap_duplicate(entry, SWAP_MAP_SHMEM); > >> >> >> > + __swap_duplicate(entry, SWAP_MAP_SHMEM, 1); > >> >> >> > } > >> >> >> > > >> >> >> > /* > >> >> >> > @@ -3453,29 +3470,29 @@ int swap_duplicate(swp_entry_t entry) > >> >> >> > { > >> >> >> > int err =3D 0; > >> >> >> > > >> >> >> > - while (!err && __swap_duplicate(entry, 1) =3D=3D -ENOMEM= ) > >> >> >> > + while (!err && __swap_duplicate(entry, 1, 1) =3D=3D -ENO= MEM) > >> >> >> > err =3D add_swap_count_continuation(entry, GFP_A= TOMIC); > >> >> >> > return err; > >> >> >> > } > >> >> >> > > >> >> >> > /* > >> >> >> > - * @entry: swap entry for which we allocate swap cache. > >> >> >> > + * @entry: first swap entry from which we allocate nr swap ca= che. > >> >> >> > * > >> >> >> > - * Called when allocating swap cache for existing swap entry, > >> >> >> > + * Called when allocating swap cache for existing swap entrie= s, > >> >> >> > * This can return error codes. Returns 0 at success. > >> >> >> > * -EEXIST means there is a swap cache. > >> >> >> > * Note: return code is different from swap_duplicate(). > >> >> >> > */ > >> >> >> > -int swapcache_prepare(swp_entry_t entry) > >> >> >> > +int swapcache_prepare(swp_entry_t entry, int nr) > >> >> >> > { > >> >> >> > - return __swap_duplicate(entry, SWAP_HAS_CACHE); > >> >> >> > + return __swap_duplicate(entry, SWAP_HAS_CACHE, nr); > >> >> >> > } > >> >> >> > > >> >> >> > -void swapcache_clear(struct swap_info_struct *si, swp_entry_t= entry) > >> >> >> > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t= entry, int nr) > >> >> >> > { > >> >> >> > unsigned long offset =3D swp_offset(entry); > >> >> >> > > >> >> >> > - cluster_swap_free_nr(si, offset, 1, SWAP_HAS_CACHE); > >> >> >> > + cluster_swap_free_nr(si, offset, nr, SWAP_HAS_CACHE); > >> >> >> > } > >> >> >> > > >> >> >> > struct swap_info_struct *swp_swap_info(swp_entry_t entry) > >> >> >> > >> >> >> -- > >> >> >> Best Regards, > >> >> >> Huang, Ying > >> >> > > > > > Thanks > > Barry