Re: [PATCH 2/4] mm/shmem, swap: avoid redundant Xarray lookup during swapin

[Dev Jain]

On 18/06/25 12:52 pm, Kairui Song wrote:
> On Wed, Jun 18, 2025 at 3:17 PM Dev Jain <dev.jain@xxxxxxx> wrote:
> > On 18/06/25 12:05 am, Kairui Song wrote:
> > > From: Kairui Song <kasong@xxxxxxxxxxx>
> > >
> > > Currently shmem calls xa_get_order to get the swap radix entry order,
> > > requiring a full tree walk. This can be easily combined with the swap
> > > entry value checking (shmem_confirm_swap) to avoid the duplicated
> > > lookup, which should improve the performance.
> > Nice spot!
> >
> > > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > > ---
> > >    mm/shmem.c | 33 ++++++++++++++++++++++++---------
> > >    1 file changed, 24 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index 4e7ef343a29b..0ad49e57f736 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -505,15 +505,27 @@ static int shmem_replace_entry(struct address_space *mapping,
> > >
> > >    /*
> > >     * Sometimes, before we decide whether to proceed or to fail, we must check
> > > - * that an entry was not already brought back from swap by a racing thread.
> > > + * that an entry was not already brought back or split by a racing thread.
> > >     *
> > >     * Checking folio is not enough: by the time a swapcache folio is locked, it
> > >     * might be reused, and again be swapcache, using the same swap as before.
> > > + * Returns the swap entry's order if it still presents, else returns -1.
> > >     */
> > > -static bool shmem_confirm_swap(struct address_space *mapping,
> > > -                            pgoff_t index, swp_entry_t swap)
> > > +static int shmem_swap_check_entry(struct address_space *mapping, pgoff_t index,
> > > +                               swp_entry_t swap)
> > I think the function name shmem_confirm_swap is already good enough? Anyhow the
> > changed name should at least be shmem_check_entry_is_swap.
> Good, I can keep the function name unchanged or follow your
> suggestion, I thought a `confirm` function returning non-binary return

True. I will vote for keeping the name unchanged; you have already documented
the return value so it should be fine. Just can you put a new line between
"Returns the swap entry's order..." and the previous line to make it clear.

> value may look strange. I'm terrible at naming things :P