On Tue, Sep 24, 2024 at 8:08 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Tue, Sep 24, 2024 at 7:32 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> >
> > On Mon, Sep 23, 2024 at 8:25 PM Baolin Wang
> > <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > >
> > > On 2024/9/24 10:15, Yosry Ahmed wrote:
> > > > On Mon, Sep 23, 2024 at 6:55 PM Baolin Wang
> > > > <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 2024/9/24 07:11, Nhat Pham wrote:
> > > >>> The SWAP_MAP_SHMEM state was originally introduced in the commit
> > > >>> aaa468653b4a ("swap_info: note SWAP_MAP_SHMEM"), to quickly determine if a
> > > >>> swap entry belongs to shmem during swapoff.
> > > >>>
> > > >>> However, swapoff has since been rewritten drastically in the commit
> > > >>> b56a2d8af914 ("mm: rid swapoff of quadratic complexity"). Now
> > > >>> having swap count == SWAP_MAP_SHMEM value is basically the same as having
> > > >>> swap count == 1, and swap_shmem_alloc() behaves analogously to
> > > >>> swap_duplicate()
> > > >>>
> > > >>> This RFC proposes the removal of this state and the associated helper to
> > > >>> simplify the state machine (both mentally and code-wise). We will also
> > > >>> have an extra state/special value that can be repurposed (for swap entries
> > > >>> that never gets re-duplicated).
> > > >>>
> > > >>> Another motivation (albeit a bit premature at the moment) is the new swap
> > > >>> abstraction I am currently working on, that would allow for swap/zswap
> > > >>> decoupling, swapoff optimization, etc. The fewer states and swap API
> > > >>> functions there are, the simpler the conversion will be.
> > > >>>
> > > >>> I am sending this series first as an RFC, just in case I missed something
> > > >>> or misunderstood this state, or if someone has a swap optimization in mind
> > > >>> for shmem that would require this special state.
> > > >>
> > > >> The idea makes sense to me. I did a quick test with shmem mTHP, and
> > > >> encountered the following warning which is triggered by
> > > >> 'VM_WARN_ON(usage == 1 && nr > 1)' in __swap_duplicate().
> > > >
> > > > Apparently __swap_duplicate() does not currently handle increasing the
> > > > swap count for multiple swap entries by 1 (i.e. usage == 1) because it
> > > > does not handle rolling back count increases when
> > > > swap_count_continued() fails.
> > > >
> > > > I guess this voids my Reviewed-by until we sort this out. Technically
> > > > swap_count_continued() won't ever be called for shmem because we only
> > > > ever increment the count by 1, but there is no way to know this in
> > > > __swap_duplicate() without SWAP_HAS_SHMEM.
> >
> > Ah this is my bad. I compiled with CONFIG_THP_SWAP, but forgot to
> > remove the swapfile check (that's another can of worms, but I need
> > data before submitting the patch to remove it...)
> >
> > One thing we can do is instead of warning here, we can handle it in
> > the for loop check, where we have access to count - that's the point
> > of having that for-loop check anyway? :)
> >
> > There's a couple of ways to go about it:
> >
> > 1. VM_WARN_ON(usage == 1 && nr > 1 && count != 0 );
>
> Hmm that should work, although it's a bit complicated tbh.
>
> > (or more accurately, (count & ~COUNT_CONTINUED) >= SWAP_MAP_MAX))
>
> I think this will make the warning very hard to hit if there's a
> misuse of __swap_duplicate(). It will only be hit when an entry needs
> count continuation, which I am not sure is very common. If there's a
> bug, the warning will potentially catch it too late, if ever.
>
> The side effect here is failing to decrement the swap count of some
> swap entries which will lead to them never being freed, essentially
> leaking swap capacity slowly over time. I am not sure if there are
> more detrimental effects.
>
> >
> > 2. Alternatively, instead of warning here, we can simply return
> > -ENOMEM. Then, at shmem callsite, have a VM_WARN_ON/VM_BUG_ON(), since
> > this MUST succeed.
>
> We still fail to rollback incremented counts though when we return
> -ENOMEM, right? Maybe I didn't get what you mean.
My understanding now is that there are two for loops. One for loop
that checks the entry's states, and one for loop that does the actual
incrementing work (or state modification).
We can check in the first for loop, if it is safe to proceed:
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;
} else if (usage == 1 && nr > 1 && (count & ~COUNT_CONTINUED) >=
SWAP_MAP_MAX)) {
/* the batched variants currently do not support rollback */
err = -ENOMEM;
}
At this point, IIUC, we have not done any incrementing, so no rollback
needed? :)
>
> >
> > Either solutions should follow with careful documentation to make it
> > clear the expectation/guarantee of the new API.
> >
> > Yosry, Baolin, how do you two feel about this? Would something like
> > this work? I need to test it first, but let me know if I'm missing
> > something.
> >
> > If this does not work, we can do what Baolin is suggesting, and
> > perhaps maintain the swap_shmem_alloc() helper. It's less than ideal,
> > but at least we still lose a state...
>
> Depending on the complexity tbh, right now removing SWAP_MAP_SHMEM is
> just a cleanup with small wins, so if it's too complicated to remove
> it it may not be worth it. I am assuming with your ongoing work, it
> becomes much more valuable, so maybe if it's too complicated we can
> defer it until the benefits are realizable?
I agree :)
