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

[Kairui Song]

On Fri, Mar 14, 2025 at 3:32 PM Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> wrote:
>
> on 3/14/2025 1:42 AM, Kairui Song wrote:
> > On Thu, Mar 13, 2025 at 8:09 PM Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >> ---
> >>  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,
> Hello Kairui,
>
> Thanks for feedback.
> >
> > 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.
> Sure, I will do this in next version.
> >
> > 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.
> >
> Sorry, I don't quite follow. Are you suggesting that should fold this
> patch to later one which removes the HAS_CACHE check and renmae the
> swap_entry_range_free.

Hi,

Just some of my nitpicks :)

After you move these parts out of swap_entry_put_locked, there is
almost nothing left in swap_entry_put_locked except an "open coded
memset". And in your next patch (also after the whole series), all
callers of __swap_entries_free will have to call an "open coded
memset" anyway, so these changes seem redundant and could be improved.

BTW your next patch has a typo in the commit message:
s/__swap_entriy_free/__swap_entries_free/g.

>
> Thanks,
> Kemeng
>