[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMgjq7AxBa6Y8vi1Fbj1Lua6ODfDwfqfGHTjhGtn24LBjManuA@mail.gmail.com>
Date: Mon, 8 Sep 2025 23:38:46 +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
Subject: Re: [PATCH v2 11/15] mm, swap: use the swap table for the swap cache
and switch API
On Sat, Sep 6, 2025 at 11:35 PM Chris Li <chrisl@...nel.org> wrote:
>
> Acked-by: Chris Li <chrisl@...nel.org>
>
> Some nitpick follows.
>
> Chris
Thanks!
> > +
> > +/*
> > + * swap_cluster_lock_by_folio - Locks the cluster that holds a folio's entries.
> > + * @folio: The folio.
> > + *
> > + * This locks the swap cluster that contains a folio's swap entries. The
> > + * swap entries of a folio are always in one single cluster, and a locked
> > + * swap cache folio is enough to stabilize the entries and the swap device.
>
> I was wondering if we have a better word than stabilize, we haven't
> defined what does stabilize mean. I assume it means protecting from
> racing access to the swap cache entry. If we describe what it protects
> or what it prevents, that would give more detailed meaning than
> stabilize.
Right, I used to use the word "pin". What it means here is: locking
the folio will ensure folio->swap won't change so the folio will have
a stable bind with the swap cluster its folio->swap points to. Also
the swap device can't be swapped off so there is no risk of UAF.
How about:
* This locks the swap cluster that contains a folio's swap entries. The
* swap entries of a folio are always in one single cluster. The folio has
* to be locked so its swap entries won't change and the cluster is binded
* to the folio.
...
> > @@ -123,57 +136,45 @@ void *swap_cache_get_shadow(swp_entry_t entry)
> > * SWAP_HAS_CACHE to avoid race or conflict.
> > * Return: Returns 0 on success, error code otherwise.
> > */
> > -int swap_cache_add_folio(struct folio *folio, swp_entry_t entry,
> > - gfp_t gfp, void **shadowp)
> > +void swap_cache_add_folio(struct folio *folio, swp_entry_t entry, void **shadowp)
> > {
> > - struct address_space *address_space = swap_address_space(entry);
> > - pgoff_t idx = swap_cache_index(entry);
> > - XA_STATE_ORDER(xas, &address_space->i_pages, idx, folio_order(folio));
> > - unsigned long i, nr = folio_nr_pages(folio);
> > - void *old;
> > -
> > - xas_set_update(&xas, workingset_update_node);
> > -
> > - VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> > - VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);
> > - VM_BUG_ON_FOLIO(!folio_test_swapbacked(folio), folio);
> > + void *shadow = NULL;
> > + unsigned long swp_tb, exist;
> > + struct swap_cluster_info *ci;
> > + unsigned int ci_start, ci_off, ci_end;
> > + unsigned long nr_pages = folio_nr_pages(folio);
> > +
> > + VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> > + VM_WARN_ON_ONCE_FOLIO(folio_test_swapcache(folio), folio);
> > + VM_WARN_ON_ONCE_FOLIO(!folio_test_swapbacked(folio), folio);
> > +
> > + swp_tb = folio_to_swp_tb(folio);
> > + ci_start = swp_cluster_offset(entry);
> > + ci_end = ci_start + nr_pages;
> > + ci_off = ci_start;
> > + ci = swap_cluster_lock(__swap_entry_to_info(entry), swp_offset(entry));
> > + do {
> > + exist = __swap_table_xchg(ci, ci_off, swp_tb);
>
> Thanks for changing it to xchg. I understand that by "exist" you mean
> the previous existing swap table entry. However after it was taken out
> from the swap table, is it still considered an "existing entry"? I am
> considering "old" or "prior" might be a better name. Just nitpicks
> anyway. If we use "old", we can rename "swp_tb" to "new_tb" to make it
> obvious what we are replacing with.
Good suggestion.
>
> Also I saw this kind of for loop repeat a few places.
> Maybe considering some for loop macro to do:
>
> for_each_folio_offset(folio, ci, ci_off) {
> exist = __swap_table_xchg(ci, ci_off, swp_tb);
> ...
> } end_for_each_folio_offset();
>
> The kernel has a lot of similar for loop macros.
>
There seem to be only a few users like this, but I can have a try.
Will use this style if it helps to reduce LOC or make it easier.
Thanks!
Powered by blists - more mailing lists