Re: [RFC PATCH 0/2] remove SWAP_MAP_SHMEM

[Barry Song <baohua@kernel.org>]

On Thu, Sep 26, 2024 at 2:37 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Tue, Sep 24, 2024 at 6:53 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
> >
> >
> > One benefit I can mention is that removing 'SWAP_MAP_SHMEM' can help to
> > batch free shmem swap entries in __swap_entries_free(), similar to the
> > commit bea67dcc5eea ("mm: attempt to batch free swap entries for
> > zap_pte_range()") did, which can improve the performance of shmem mTHP
> > munmap() function based on my testing.
>
> Yeah, the problem with having an extraneous state is you have to
> constantly check for it in code, and/or keep it in mind when you
> develop things. I've been constantly having to check for this state
> when I develop code around this area, and it gets old fast.
>
> If we can use it to optimize something, I can understand keeping it.
> But it just seems like dead code to me :)
>
> My preference is to do this as simply as possible - add another case
> (usage == 1, nr > 1, and we need to add swap continuations) in the
> check in __swap_duplicate()'s first loop, and just WARN right there.
>
> That case CANNOT happen UNLESS we introduce a bug, or have a new use
> case. When we actually have a use case, we can always introduce
> handling/fallback logic for that case.
>
> Barry, Yosry, Baolin, Ying, how do you feel about this?

I’m not entirely clear on your point. If your proposal is to support the
case where usage == 1 and nr > 1 only when we don’t require
CONTINUED, and to issue a warning once we determine that
CONTINUED is needed, then I’m completely on board with that
approach.

It seems that your intention is to simply relocate the existing warning
to the scenario where CONTINUED is actually required, rather than
maintaining a warning for the case where usage == 1 and nr > 1 at
all times?

I wasn't actually suggesting a rollback as you posted:
     err = __swap_duplicate(entry, 1, nr);
     if (err == -ENOMEM) {
         /* fallback to non-batched version */
         for (i = 0; i < nr; i++) {
             cur_entry = (swp_entry_t){entry.val + i};
             if (swap_duplicate(cur_entry)) {
                 /* rollback */
                 while (--i >= 0) {
                      cur_entry = (swp_entry_t){entry.val + i};
                      swap_free(cur_entry);
                 }

I suggested checking for all errors except -ENOMEM in the first loop. If all
conditions indicate that only CONTINUED is necessary (with no other
issues like ENOENT, EEXIST, etc., for any entry), we can proceed
with a for loop for swap_duplicate(), eliminating the need for a rollback.

However, I agree that we can proceed with that only when there is actually
a user involved. (There's no need to address it right now.)

Thanks
Barry