| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMgjq7Aznd7=m6JTNGM4EyFj+6pqHTRBCo2hsQL-cKi0LZggOg@mail.gmail.com>
Date: Sat, 30 Aug 2025 23:24:53 +0800
From: Kairui Song <ryncsn@...il.com>
To: Chris Li <chrisl@...nel.org>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
Matthew Wilcox <willy@...radead.org>, Hugh Dickins <hughd@...gle.com>, Barry Song <baohua@...nel.org>,
Baoquan He <bhe@...hat.com>, Nhat Pham <nphamcs@...il.com>,
Kemeng Shi <shikemeng@...weicloud.com>, Baolin Wang <baolin.wang@...ux.alibaba.com>,
Ying Huang <ying.huang@...ux.alibaba.com>, Johannes Weiner <hannes@...xchg.org>,
David Hildenbrand <david@...hat.com>, Yosry Ahmed <yosryahmed@...gle.com>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Zi Yan <ziy@...dia.com>,
linux-kernel@...r.kernel.org, kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH 7/9] mm, swap: remove contention workaround for swap cache
On Sat, Aug 30, 2025 at 1:03 PM Chris Li <chrisl@...nel.org> wrote:
>
> Hi Kairui,
>
> It feels so good to remove that 64M swap cache space. Thank you for
> making it happen.
>
> Some nitpick follows. I am fine as is as well.
>
> Acked-by: Chris Li <chrisl@...nel.org>
Thanks.
>
> Chris
>
> On Fri, Aug 22, 2025 at 12:21 PM Kairui Song <ryncsn@...il.com> wrote:
> >
> > From: Kairui Song <kasong@...cent.com>
> >
> > Swap cluster setup will try to shuffle the clusters on initialization.
> > It was helpful to avoid contention for the swap cache space. The cluster
> > size (2M) was much smaller than each swap cache space (64M), so shuffling
> > the cluster means the allocator will try to allocate swap slots that are
> > in different swap cache spaces for each CPU, reducing the chance of two
> > CPUs using the same swap cache space, and hence reducing the contention.
> >
> > Now, swap cache is managed by swap clusters, this shuffle is pointless.
> > Just remove it, and clean up related macros.
> >
> > This should also improve the HDD swap performance as shuffling IO is a
> > bad idea for HDD, and now the shuffling is gone.
>
> Did you have any numbers to prove that :-). Last time the swap
> allocator stress testing has already destroyed two of my SAS drives
> dedicated for testing. So I am not very keen on running the HDD swap
> stress test. The HDD swap stress test are super slow to run, it takes
> ages.
I did some test months before, removing the cluster shuffle did help.
I didn't test it again this time, only did some stress test. Doing
performance test on HDD is really not a good experience as my HDD
drives are too old so a long running test kills them easily.
And I couldn't find any other factor that is causing a serial HDD IO
regression, maybe the bot can help verify. If this doesn't help, we'll
think of something else. But I don't think HDD based SWAP will ever
have a practical good performance as they are terrible at rand read...
Anyway, let me try again with HDD today, maybe I'll get some useful data.
>
> >
> > Reported-by: kernel test robot <oliver.sang@...el.com>
> > Closes: https://lore.kernel.org/oe-lkp/202504241621.f27743ec-lkp@intel.com
> > Signed-off-by: Kairui Song <kasong@...cent.com>
> > ---
> > mm/swap.h | 4 ----
> > mm/swapfile.c | 32 ++++++++------------------------
> > mm/zswap.c | 7 +++++--
> > 3 files changed, 13 insertions(+), 30 deletions(-)
> >
> > diff --git a/mm/swap.h b/mm/swap.h
> > index 4af42bc2cd72..ce3ec62cc05e 100644
> > --- a/mm/swap.h
> > +++ b/mm/swap.h
> > @@ -153,10 +153,6 @@ int swap_writeout(struct folio *folio, struct swap_iocb **swap_plug);
> > void __swap_writepage(struct folio *folio, struct swap_iocb **swap_plug);
> >
> > /* linux/mm/swap_state.c */
> > -/* One swap address space for each 64M swap space */
> > -#define SWAP_ADDRESS_SPACE_SHIFT 14
> > -#define SWAP_ADDRESS_SPACE_PAGES (1 << SWAP_ADDRESS_SPACE_SHIFT)
> > -#define SWAP_ADDRESS_SPACE_MASK (SWAP_ADDRESS_SPACE_PAGES - 1)
> > extern struct address_space swap_space __ro_after_init;
> > static inline struct address_space *swap_address_space(swp_entry_t entry)
> > {
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index df68b5e242a6..0c8001c99f30 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -3203,21 +3203,14 @@ static int setup_swap_map(struct swap_info_struct *si,
> > return 0;
> > }
> >
> > -#define SWAP_CLUSTER_INFO_COLS \
> > - DIV_ROUND_UP(L1_CACHE_BYTES, sizeof(struct swap_cluster_info))
> > -#define SWAP_CLUSTER_SPACE_COLS \
> > - DIV_ROUND_UP(SWAP_ADDRESS_SPACE_PAGES, SWAPFILE_CLUSTER)
> > -#define SWAP_CLUSTER_COLS \
> > - max_t(unsigned int, SWAP_CLUSTER_INFO_COLS, SWAP_CLUSTER_SPACE_COLS)
> > -
> > static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
> > union swap_header *swap_header,
> > unsigned long maxpages)
> > {
> > unsigned long nr_clusters = DIV_ROUND_UP(maxpages, SWAPFILE_CLUSTER);
> > struct swap_cluster_info *cluster_info;
> > - unsigned long i, j, idx;
> > int err = -ENOMEM;
> > + unsigned long i;
>
> Nitpick: This line location change is not necessary.
>
> >
> > cluster_info = kvcalloc(nr_clusters, sizeof(*cluster_info), GFP_KERNEL);
> > if (!cluster_info)
> > @@ -3266,22 +3259,13 @@ static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
> > INIT_LIST_HEAD(&si->frag_clusters[i]);
> > }
> >
> > - /*
> > - * Reduce false cache line sharing between cluster_info and
> > - * sharing same address space.
> > - */
> > - for (j = 0; j < SWAP_CLUSTER_COLS; j++) {
> > - for (i = 0; i < DIV_ROUND_UP(nr_clusters, SWAP_CLUSTER_COLS); i++) {
> > - struct swap_cluster_info *ci;
> > - idx = i * SWAP_CLUSTER_COLS + j;
> > - ci = cluster_info + idx;
> > - if (idx >= nr_clusters)
> > - continue;
> > - if (ci->count) {
> > - ci->flags = CLUSTER_FLAG_NONFULL;
> > - list_add_tail(&ci->list, &si->nonfull_clusters[0]);
> > - continue;
> > - }
> > + for (i = 0; i < nr_clusters; i++) {
> > + struct swap_cluster_info *ci = &cluster_info[i];
>
> struct swap_cluster_info *ci = cluster_info + i;
> looks simpler. Pure nitpick and personal preference, you don't have to
> follow it.
>
> > +
> > + if (ci->count) {
> > + ci->flags = CLUSTER_FLAG_NONFULL;
> > + list_add_tail(&ci->list, &si->nonfull_clusters[0]);
> > + } else {
> > ci->flags = CLUSTER_FLAG_FREE;
> > list_add_tail(&ci->list, &si->free_clusters);
> > }
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index c869859eec77..c0a9be14a725 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -237,10 +237,13 @@ static bool zswap_has_pool;
> > * helpers and fwd declarations
> > **********************************/
> >
> > +/* One swap address space for each 64M swap space */
> > +#define ZSWAP_ADDRESS_SPACE_SHIFT 14
> > +#define ZSWAP_ADDRESS_SPACE_PAGES (1 << ZSWAP_ADDRESS_SPACE_SHIFT)
> > static inline struct xarray *swap_zswap_tree(swp_entry_t swp)
> > {
> > return &zswap_trees[swp_type(swp)][swp_offset(swp)
> > - >> SWAP_ADDRESS_SPACE_SHIFT];
> > + >> ZSWAP_ADDRESS_SPACE_SHIFT];
> > }
> >
> > #define zswap_pool_debug(msg, p) \
> > @@ -1771,7 +1774,7 @@ int zswap_swapon(int type, unsigned long nr_pages)
> > struct xarray *trees, *tree;
> > unsigned int nr, i;
> >
> > - nr = DIV_ROUND_UP(nr_pages, SWAP_ADDRESS_SPACE_PAGES);
> > + nr = DIV_ROUND_UP(nr_pages, ZSWAP_ADDRESS_SPACE_PAGES);
> > trees = kvcalloc(nr, sizeof(*tree), GFP_KERNEL);
> > if (!trees) {
> > pr_err("alloc failed, zswap disabled for swap type %d\n", type);
> > --
> > 2.51.0
> >
>
Powered by blists - more mailing lists