Re: [PATCH 1/1] mm: swap: add nr argument in swapcache_prepare and swapcache_clear to support large folios

[Barry Song]

On Fri, Aug 2, 2024 at 1:50 PM Huang, Ying <ying.huang@xxxxxxxxx> wrote:
> >
> > Right. I believe the change below can help improve readability and also
> > clarify the swap_count_continued() case.
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 2fa29bdec171..75a89ce18edc 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -3538,6 +3538,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> >
> >       offset = swp_offset(entry);
> >       VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER);
> > +     VM_WARN_ON(usage == 1 && nr > 1);
> >       ci = lock_cluster_or_swap_info(p, offset);
> >
> >       err = 0;
> > @@ -3564,17 +3565,9 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> >                               err = -EEXIST;
> >                       else                            /* no users remaining */
> >                               err = -ENOENT;
> > -
> >               } else if (count || has_cache) {
> > -
> > -                     if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
> > -                             continue;
> > -                     else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX)
> > +                     if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX)
> >                               err = -EINVAL;
> > -                     else if (swap_count_continued(p, offset + i, count))
> > -                             continue;
> > -                     else
> > -                             err = -ENOMEM;
> >               } else
> >                       err = -ENOENT;                  /* unused swap entry */
> >
> > @@ -3591,8 +3584,12 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> >                       has_cache = SWAP_HAS_CACHE;
> >               else if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
> >                       count += usage;
> > -             else
> > +             else if (swap_count_continued(p, offset + i, count))
> >                       count = COUNT_CONTINUED;
> > +             else {
> > +                     err = -ENOMEM;
> > +                     goto unlock_out;
> > +             }
> >
> >               WRITE_ONCE(p->swap_map[offset + i], count | has_cache);
> >       }
> >
> > This makes the two iterations become:
> >
> >       for (i = 0; i < nr; i++) {
> >               count = p->swap_map[offset + i];
> >
> >               /*
> >                * swapin_readahead() doesn't check if a swap entry is valid, so the
> >                * swap entry could be SWAP_MAP_BAD. Check here with lock held.
> >                */
> >               if (unlikely(swap_count(count) == SWAP_MAP_BAD)) {
> >                       err = -ENOENT;
> >                       goto unlock_out;
> >               }
> >
> >               has_cache = count & SWAP_HAS_CACHE;
> >               count &= ~SWAP_HAS_CACHE;
> >
> >               if (usage == SWAP_HAS_CACHE) {
> >                       /* set SWAP_HAS_CACHE if there is no cache and entry is used */
>
> The comments doen't apply now, we don't "set" here.
>
> >                       if (!has_cache && count)
> >                               continue;
> >                       else if (has_cache)             /* someone else added cache */
> >                               err = -EEXIST;
> >                       else                            /* no users remaining */
> >                               err = -ENOENT;
> >               } else if (count || has_cache) {
> >                       if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX)
> >                               err = -EINVAL;
> >               } else
> >                       err = -ENOENT;                  /* unused swap entry */
>
> It seems that this can be simplified to:
>
>                 if (!count && !has_cache) {
>                         err = -ENOENT;
>                 } else if (usage == SWAP_HAS_CACHE) {
>                         if (has_cache)
>                                 err = -EEXIST;
>                 } else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) {
>                         err = -EINVAL;
>                 }
>
> >               if (err)
> >                       goto unlock_out;
> >       }
> >
> >       for (i = 0; i < nr; i++) {
> >               count = p->swap_map[offset + i];
> >               has_cache = count & SWAP_HAS_CACHE;
> >               count &= ~SWAP_HAS_CACHE;
> >
> >               if (usage == SWAP_HAS_CACHE)
> >                       has_cache = SWAP_HAS_CACHE;
> >               else if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
> >                       count += usage;
> >               else if (swap_count_continued(p, offset + i, count))
> >                       count = COUNT_CONTINUED;
> >               else {
>
> Better to add some comments here,
>
>                         /*
>                          * Don't need to rollback changes, because if
>                          * usage == 1, there must be nr == 1.
>                          */
>
> >                       err = -ENOMEM;
> >                       goto unlock_out;
> >               }
> >
> >               WRITE_ONCE(p->swap_map[offset + i], count | has_cache);
> >       }
> >
> > Ying, do you feel more satisfied with the version above
> > compared to the code in mm-unstable?
>
> This looks good to me except some minor comments above.  Thanks!

Thanks very much, Ying. 

Hi Andrew,
Could you please help squash the below change?