on 3/19/2025 1:18 PM, Kairui Song wrote:
> On Tue, Mar 18, 2025 at 2:10 PM Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> wrote:
>>
>> As all callers of swap_entry_range_free() have already ensured slots to
>> be freed are marked as SWAP_HAS_CACHE while holding the cluster lock,
>> the BUG_ON check can be safely removed. After this, the function
>> swap_entry_range_free() could drop any kind of last flag, rename it to
>> swap_entries_free() and update it's comment accordingly.
>>
>> This is a preparation to use swap_entries_free() to drop last ref count
>> and SWAP_MAP_SHMEM flag.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
>> Reviewed-by: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
>> ---
>> mm/swapfile.c | 27 +++++++++++++--------------
>> 1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 5a775456e26c..0aa7ce82c013 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -52,9 +52,9 @@
>> static bool swap_count_continued(struct swap_info_struct *, pgoff_t,
>> unsigned char);
>> static void free_swap_count_continuations(struct swap_info_struct *);
>> -static void swap_entry_range_free(struct swap_info_struct *si,
>> - struct swap_cluster_info *ci,
>> - swp_entry_t entry, unsigned int nr_pages);
>> +static void swap_entries_free(struct swap_info_struct *si,
>> + struct swap_cluster_info *ci,
>> + swp_entry_t entry, unsigned int nr_pages);
>> static void swap_range_alloc(struct swap_info_struct *si,
>> unsigned int nr_entries);
>> static bool folio_swapcache_freeable(struct folio *folio);
>> @@ -1463,7 +1463,7 @@ static unsigned char swap_entry_put(struct swap_info_struct *si,
>> ci = lock_cluster(si, offset);
>> usage = swap_entry_put_locked(si, offset, 1);
>> if (!usage)
>> - swap_entry_range_free(si, ci, swp_entry(si->type, offset), 1);
>> + swap_entries_free(si, ci, swp_entry(si->type, offset), 1);
>> unlock_cluster(ci);
>>
>> return usage;
>> @@ -1493,7 +1493,7 @@ static bool swap_entries_put_nr(struct swap_info_struct *si,
>> for (i = 0; i < nr; i++)
>> WRITE_ONCE(si->swap_map[offset + i], SWAP_HAS_CACHE);
>> if (!has_cache)
>> - swap_entry_range_free(si, ci, entry, nr);
>> + swap_entries_free(si, ci, entry, nr);
>> unlock_cluster(ci);
>>
>> return has_cache;
>> @@ -1512,12 +1512,12 @@ static bool swap_entries_put_nr(struct swap_info_struct *si,
>> }
>>
>> /*
>> - * Drop the last HAS_CACHE flag of swap entries, caller have to
>> - * ensure all entries belong to the same cgroup.
>> + * Drop the last flag(1, SWAP_HAS_CACHE or SWAP_MAP_SHMEM) of swap entries,
>> + * caller have to ensure all entries belong to the same cgroup and cluster.
>> */
>> -static void swap_entry_range_free(struct swap_info_struct *si,
>> - struct swap_cluster_info *ci,
>> - swp_entry_t entry, unsigned int nr_pages)
>> +static void swap_entries_free(struct swap_info_struct *si,
>> + struct swap_cluster_info *ci,
>> + swp_entry_t entry, unsigned int nr_pages)
>> {
>> unsigned long offset = swp_offset(entry);
>> unsigned char *map = si->swap_map + offset;
>> @@ -1530,7 +1530,6 @@ static void swap_entry_range_free(struct swap_info_struct *si,
>>
>> ci->count -= nr_pages;
>> do {
>> - VM_BUG_ON(*map != SWAP_HAS_CACHE);
>
> Hi Kemeng
>
> Instead of just dropping this check, maybe it's better to change it to
> something like VM_BUG_ON(!*map); to catch potential SWAP double free?
> I've found this check very helpful for debugging, especially for
> catching concurrency problems.
Sure, will keep VM_BUG_ON as it's useful.
>
> Or more strictly: VM_BUG_ON(*map != SWAP_HAS_CACHE && *map != 1 &&
> *map != SWAP_MAP_SHMEM);, you may introduce a helper to check if a
> entry is the "last map" like this and use it somewhere else too.
I'd add VM_BUG_ON in this way to catch unexpected freeing.
Thanks,
Kemeng
>
>
>> *map = 0;
>> } while (++map < map_end);
>>
>> @@ -1553,7 +1552,7 @@ static void cluster_swap_free_nr(struct swap_info_struct *si,
>> ci = lock_cluster(si, offset);
>> do {
>> if (!swap_entry_put_locked(si, offset, usage))
>> - swap_entry_range_free(si, ci, swp_entry(si->type, offset), 1);
>> + swap_entries_free(si, ci, swp_entry(si->type, offset), 1);
>> } while (++offset < end);
>> unlock_cluster(ci);
>> }
>> @@ -1596,11 +1595,11 @@ void put_swap_folio(struct folio *folio, swp_entry_t entry)
>>
>> ci = lock_cluster(si, offset);
>> if (swap_only_has_cache(si, offset, size))
>> - swap_entry_range_free(si, ci, entry, size);
>> + swap_entries_free(si, ci, entry, size);
>> else {
>> for (int i = 0; i < size; i++, entry.val++) {
>> if (!swap_entry_put_locked(si, offset + i, SWAP_HAS_CACHE))
>> - swap_entry_range_free(si, ci, entry, 1);
>> + swap_entries_free(si, ci, entry, 1);
>> }
>> }
>> unlock_cluster(ci);
>> --
>> 2.30.0
>>
>
