| 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: <CAMgjq7D5WuNW_WpZe=U+U9pQ3xaYFxkG6kOXK8PD8E+VaBEoiA@mail.gmail.com>
Date: Sun, 6 Jul 2025 19:50:10 +0800
From: Kairui Song <ryncsn@...il.com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
Hugh Dickins <hughd@...gle.com>, Matthew Wilcox <willy@...radead.org>,
Kemeng Shi <shikemeng@...weicloud.com>, Chris Li <chrisl@...nel.org>,
Nhat Pham <nphamcs@...il.com>, Baoquan He <bhe@...hat.com>, Barry Song <baohua@...nel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 4/9] mm/shmem, swap: tidy up swap entry splitting
On Sun, Jul 6, 2025 at 11:38 AM Baolin Wang
<baolin.wang@...ux.alibaba.com> wrote:
>
>
>
> On 2025/7/5 02:17, Kairui Song wrote:
> > From: Kairui Song <kasong@...cent.com>
> >
> > Instead of keeping different paths of splitting the entry before the
> > swap in start, move the entry splitting after the swapin has put
> > the folio in swap cache (or set the SWAP_HAS_CACHE bit). This way
> > we only need one place and one unified way to split the large entry.
> > Whenever swapin brought in a folio smaller than the shmem swap entry,
> > split the entry and recalculate the entry and index for verification.
> >
> > This removes duplicated codes and function calls, reduces LOC,
> > and the split is less racy as it's guarded by swap cache now. So it
> > will have a lower chance of repeated faults due to raced split.
> > The compiler is also able to optimize the coder further:
> >
> > bloat-o-meter results with GCC 14:
> >
> > With DEBUG_SECTION_MISMATCH (-fno-inline-functions-called-once):
> > ./scripts/bloat-o-meter mm/shmem.o.old mm/shmem.o
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-82 (-82)
> > Function old new delta
> > shmem_swapin_folio 2361 2279 -82
> > Total: Before=33151, After=33069, chg -0.25%
> >
> > With !DEBUG_SECTION_MISMATCH:
> > ./scripts/bloat-o-meter mm/shmem.o.old mm/shmem.o
> > add/remove: 0/1 grow/shrink: 1/0 up/down: 949/-750 (199)
> > Function old new delta
> > shmem_swapin_folio 2878 3827 +949
> > shmem_split_large_entry.isra 750 - -750
> > Total: Before=33086, After=33285, chg +0.60%
> >
> > Since shmem_split_large_entry is only called in one place now. The
> > compiler will either generate more compact code, or inlined it for
> > better performance.
> >
> > Signed-off-by: Kairui Song <kasong@...cent.com>
> > ---
> > mm/shmem.c | 53 +++++++++++++++++++++--------------------------------
> > 1 file changed, 21 insertions(+), 32 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index e43becfa04b3..217264315842 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2266,14 +2266,15 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> > struct address_space *mapping = inode->i_mapping;
> > struct mm_struct *fault_mm = vma ? vma->vm_mm : NULL;
> > struct shmem_inode_info *info = SHMEM_I(inode);
> > + swp_entry_t swap, index_entry;
> > struct swap_info_struct *si;
> > struct folio *folio = NULL;
> > bool skip_swapcache = false;
> > - swp_entry_t swap;
> > int error, nr_pages, order, split_order;
> > + pgoff_t offset;
> >
> > VM_BUG_ON(!*foliop || !xa_is_value(*foliop));
> > - swap = radix_to_swp_entry(*foliop);
> > + swap = index_entry = radix_to_swp_entry(*foliop);
> > *foliop = NULL;
> >
> > if (is_poisoned_swp_entry(swap))
> > @@ -2321,46 +2322,35 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> > }
> >
> > /*
> > - * Now swap device can only swap in order 0 folio, then we
> > - * should split the large swap entry stored in the pagecache
> > - * if necessary.
> > - */
> > - split_order = shmem_split_large_entry(inode, index, swap, gfp);
> > - if (split_order < 0) {
> > - error = split_order;
> > - goto failed;
> > - }
> > -
> > - /*
> > - * If the large swap entry has already been split, it is
> > + * Now swap device can only swap in order 0 folio, it is
> > * necessary to recalculate the new swap entry based on
> > - * the old order alignment.
> > + * the offset, as the swapin index might be unalgined.
> > */
> > - if (split_order > 0) {
> > - pgoff_t offset = index - round_down(index, 1 << split_order);
> > -
> > + if (order) {
> > + offset = index - round_down(index, 1 << order);
> > swap = swp_entry(swp_type(swap), swp_offset(swap) + offset);
> > }
> >
> > - /* Here we actually start the io */
> > folio = shmem_swapin_cluster(swap, gfp, info, index);
> > if (!folio) {
> > error = -ENOMEM;
> > goto failed;
> > }
> > - } else if (order > folio_order(folio)) {
> > + }
> > +alloced:
> > + if (order > folio_order(folio)) {
> > /*
> > - * Swap readahead may swap in order 0 folios into swapcache
> > + * Swapin may get smaller folios due to various reasons:
> > + * It may fallback to order 0 due to memory pressure or race,
> > + * swap readahead may swap in order 0 folios into swapcache
> > * asynchronously, while the shmem mapping can still stores
> > * large swap entries. In such cases, we should split the
> > * large swap entry to prevent possible data corruption.
> > */
> > - split_order = shmem_split_large_entry(inode, index, swap, gfp);
> > + split_order = shmem_split_large_entry(inode, index, index_entry, gfp);
> > if (split_order < 0) {
> > - folio_put(folio);
> > - folio = NULL;
> > error = split_order;
> > - goto failed;
> > + goto failed_nolock;
> > }
> >
> > /*
> > @@ -2369,15 +2359,13 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> > * the old order alignment.
> > */
> > if (split_order > 0) {
> > - pgoff_t offset = index - round_down(index, 1 << split_order);
> > -
> > + offset = index - round_down(index, 1 << split_order);
> > swap = swp_entry(swp_type(swap), swp_offset(swap) + offset);
>
> Obviously, you should use the original swap value 'index_entry' to
> calculate the new swap value.
Thanks, good catch.
>
> With the following fix, you can add:
> Reviewed-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
> Tested-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index d530df550f7f..1e8422ac863e 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2361,7 +2361,7 @@ static int shmem_swapin_folio(struct inode *inode,
> pgoff_t index,
> */
> if (split_order > 0) {
> offset = index - round_down(index, 1 <<
> split_order);
> - swap = swp_entry(swp_type(swap),
> swp_offset(swap) + offset);
> + swap = swp_entry(swp_type(swap),
> swp_offset(index_swap) + offset);
> }
> } else if (order < folio_order(folio)) {
> swap.val = round_down(swap.val, 1 << folio_order(folio));
>
>
Powered by blists - more mailing lists