On 2024/9/24 23:48, Nhat Pham wrote:
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? :)
Right, looks good (although need some cleanup pointed by Yosry).
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 :)
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.
Without this patch set, I need do following changes to batch free shmem
swap entries:
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 0cded32414a1..94e28cd60c52 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -163,7 +163,7 @@ static bool swap_is_last_map(struct swap_info_struct
*si,
unsigned char *map_end = map + nr_pages;
unsigned char count = *map;
- if (swap_count(count) != 1)
+ if (swap_count(count) != 1 && swap_count(count) != SWAP_MAP_SHMEM)
return false;
while (++map < map_end) {
@@ -1503,10 +1503,10 @@ static bool __swap_entries_free(struct
swap_info_struct *si,
unsigned int type = swp_type(entry);
struct swap_cluster_info *ci;
bool has_cache = false;
- unsigned char count;
+ unsigned char count = swap_count(data_race(si->swap_map[offset]));
int i;
- if (nr <= 1 || swap_count(data_race(si->swap_map[offset])) != 1)
+ if (nr <= 1 || (count != 1 && count != SWAP_MAP_SHMEM))
goto fallback;
/* cross into another cluster */
if (nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER)
