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?
