linux-kernel - Re: [PATCH 2/9] mm: swap: factor out the actual swap entry freeing logic to new helper

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMgjq7Ct8b25zuCEqJ4byU8gmrvz50pUqvyjYN=_RjTk_5U8cA@mail.gmail.com>
Date: Fri, 14 Mar 2025 01:42:00 +0800
From: Kairui Song <ryncsn@...il.com>
To: Kemeng Shi <shikemeng@...weicloud.com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/9] mm: swap: factor out the actual swap entry freeing
 logic to new helper

On Thu, Mar 13, 2025 at 8:09 PM Kemeng Shi <shikemeng@...weicloud.com> wrote:
>
> Factor out the actual swap entry freeing logic to new helper
> __swap_entries_free().
> This allow us to futher simplify other swap entry freeing code by
> leveraging __swap_entries_free() helper function.
>
> Signed-off-by: Kemeng Shi <shikemeng@...weicloud.com>
> ---
>  mm/swapfile.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 5a775456e26c..7c886f9dd6f9 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1347,6 +1347,25 @@ static struct swap_info_struct *_swap_info_get(swp_entry_t entry)
>         return NULL;
>  }
>
> +static inline void __swap_entries_free(struct swap_info_struct *si,
> +                                      struct swap_cluster_info *ci,
> +                                      swp_entry_t entry, unsigned int nr_pages)
> +{
> +       unsigned long offset = swp_offset(entry);
> +
> +       VM_BUG_ON(cluster_is_empty(ci));
> +       VM_BUG_ON(ci->count < nr_pages);
> +
> +       ci->count -= nr_pages;
> +       mem_cgroup_uncharge_swap(entry, nr_pages);
> +       swap_range_free(si, offset, nr_pages);
> +
> +       if (!ci->count)
> +               free_cluster(si, ci);
> +       else
> +               partial_free_cluster(si, ci);
> +}
> +
>  static unsigned char swap_entry_put_locked(struct swap_info_struct *si,
>                                            unsigned long offset,
>                                            unsigned char usage)
> @@ -1525,22 +1544,13 @@ static void swap_entry_range_free(struct swap_info_struct *si,
>
>         /* It should never free entries across different clusters */
>         VM_BUG_ON(ci != offset_to_cluster(si, offset + nr_pages - 1));
> -       VM_BUG_ON(cluster_is_empty(ci));
> -       VM_BUG_ON(ci->count < nr_pages);
>
> -       ci->count -= nr_pages;
>         do {
>                 VM_BUG_ON(*map != SWAP_HAS_CACHE);
>                 *map = 0;
>         } while (++map < map_end);
>
> -       mem_cgroup_uncharge_swap(entry, nr_pages);
> -       swap_range_free(si, offset, nr_pages);
> -
> -       if (!ci->count)
> -               free_cluster(si, ci);
> -       else
> -               partial_free_cluster(si, ci);
> +       __swap_entries_free(si, ci, entry, nr_pages);
>  }
>
>  static void cluster_swap_free_nr(struct swap_info_struct *si,
> --
> 2.30.0
>
>

Hi Kemeng,

This patch is a bit too trivial to be a standalone one, you can fold
it with the later one easily. Also you may want to carry the
VM_BUG_ON(ci != offset_to_cluster(si, offset + nr_pages - 1)); check
as well.

But, that is basically just renaming swap_entry_range_free, you may
just remove the HAS_CACHE check as you rename it, that way the changes
should be cleaner.