Re: [PATCH 2/8] mm/swapfile: add batched version of folio_put_swap

[Dev Jain]

On 25/07/26 10:23 am, Barry Song wrote:
> On Thu, Jul 23, 2026 at 3:09 PM Dev Jain <dev.jain@xxxxxxx> wrote:
>>
>> Add folio_put_swap_pages to handle a batch of consecutive pages. Note
>> that folio_put_swap already can handle a subset of this: nr_pages == 1 and
>> nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.
>>
>> Currently we have a not-so-nice logic of passing in subpage == NULL if
>> we mean to exercise the logic on the entire folio, and subpage != NULL if
>> we want to exercise the logic on only that subpage. Remove this
>> indirection: the caller invokes folio_put_swap_pages() if it wants to
>> operate on a range of pages in the folio (i.e nr_pages may be anything
>> between 1 and folio_nr_pages()), and invokes folio_put_swap() if it
>> wants to operate on the entire folio.
>>
>> Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> 
> Reviewed-by: Barry Song <baohua@xxxxxxxxxx>

Thanks for your review.


> 
> [...]
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index a620d425ec95e..d88df15018268 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5198,7 +5198,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>>         if (unlikely(folio != swapcache)) {
>>                 folio_add_new_anon_rmap(folio, vma, address, RMAP_EXCLUSIVE);
>>                 folio_add_lru_vma(folio, vma);
>> -               folio_put_swap(swapcache, NULL);
>> +               folio_put_swap(swapcache);
>>         } else if (!folio_test_anon(folio)) {
>>                 /*
>>                  * We currently only expect !anon folios that are fully
>> @@ -5207,12 +5207,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>>                 VM_WARN_ON_ONCE_FOLIO(folio_nr_pages(folio) != nr_pages, folio);
>>                 VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
>>                 folio_add_new_anon_rmap(folio, vma, address, rmap_flags);
>> -               folio_put_swap(folio, NULL);
>> +               folio_put_swap(folio);
>>         } else {
>>                 VM_WARN_ON_ONCE(nr_pages != 1 && nr_pages != folio_nr_pages(folio));
>>                 folio_add_anon_rmap_ptes(folio, page, nr_pages, vma, address,
>>                                          rmap_flags);
>> -               folio_put_swap(folio, nr_pages == 1 ? page : NULL);
>> +               folio_put_swap_pages(folio, page, nr_pages);
> 
> Yes. In do_swap_page(), `page` points to the first page of the
> large folio.
>         if (folio_test_large(folio) && folio_test_swapcache(folio)) {
>                 ...
>                 page = &folio->page;
>         }
> 
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1817,27 +1817,25 @@ int folio_dup_swap_pages(struct folio *folio, struct page *page,
>>  }
>>
>>  /**
>> - * folio_put_swap() - Decrease swap count of swap entries of a folio.
>> + * folio_put_swap_pages() - Decrease swap count of swap entries of a folio.
>>   * @folio: folio with swap entries bounded, must be in swap cache and locked.
>> - * @page: if not NULL, only decrease the swap count of this page.
>> + * @page: the first page in the folio to decrease the swap count for.
>> + * @nr_pages: the number of pages in the folio to decrease the swap count for.
>>   *
>>   * This won't free the swap slots even if swap count drops to zero, they are
>>   * still pinned by the swap cache. User may call folio_free_swap to free them.
>>   * Context: Caller must ensure the folio is locked and in the swap cache.
>>   */
>> -void folio_put_swap(struct folio *folio, struct page *page)
>> +void folio_put_swap_pages(struct folio *folio, struct page *page,
>> +               unsigned long nr_pages)
>>  {
>>         swp_entry_t entry = folio->swap;
>> -       unsigned long nr_pages = folio_nr_pages(folio);
>>         struct swap_info_struct *si = __swap_entry_to_info(entry);
>>
>>         VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
>>         VM_WARN_ON_FOLIO(!folio_test_swapcache(folio), folio);
>>
>> -       if (page) {
>> -               entry.val += folio_page_idx(folio, page);
>> -               nr_pages = 1;
>> -       }
>> +       entry.val += folio_page_idx(folio, page);
>>
>>         swap_put_entries_cluster(si, swp_offset(entry), nr_pages, false);
> 
> Maybe a guard against potential overflow would help? also in patch 1?
> 
> idx = folio_page_idx(folio, page);
> 
> VM_WARN_ON_FOLIO(idx + nr_pages > folio_nr_pages(folio), folio);

We had this discussion before and I am struggling to recall that - I think
there are currently both cases, the batched helpers not doing the sanity checking but
documenting it (I haven't documented it in this one, and in set_softleaf_ptes
too, will audit later), or doing the sanity checking.

But I also recall the expectation is that CONFIG_DEBUG_VM is not used in production
kernels, so I can put this in v2.

> 
> Best Regards
> Barry