Re: [PATCH v2 10/15] mm, swap: wrap swap cache replacement with a helper

[David Hildenbrand <david@redhat.com>]

On 08.09.25 16:20, Kairui Song wrote:
> On Mon, Sep 8, 2025 at 8:35 PM David Hildenbrand <david@redhat.com> wrote:
>>
>>
>>>
>>> +/**
>>> + * __swap_cache_replace_folio - Replace a folio in the swap cache.
>>> + * @mapping: Swap mapping address space.
>>> + * @entry: The first swap entry that the new folio corresponds to.
>>> + * @old: The old folio to be replaced.
>>> + * @new: The new folio.
>>> + *
>>> + * Replace a existing folio in the swap cache with a new folio.
>>> + *
>>> + * Context: Caller must ensure both folios are locked, and lock the
>>> + * swap address_space that holds the entries to be replaced.
>>> + */
>>> +void __swap_cache_replace_folio(struct address_space *mapping,
>>> +                             swp_entry_t entry,
>>> +                             struct folio *old, struct folio *new)
>>
>> Can't we just use "new->swap.val" directly and avoid passing in the
>> entry, documenting that new->swap.val must be setup properly in advance?
> 
> Thanks for the suggestion.
> 
> I was thinking about the opposite. I think maybe it's better that the
> caller never sets the new folio's entry value, so folio->swap is always
> modified in mm/swap_state.c, and let __swap_cache_replace_folio set
> new->swap, to make it easier to track the folio->swap
> usage.
> 
> This can be done easily for migration and shmem parts, the huge split
> code will need a bit more cleanup.

Right, but it's probably worth it.

> 
> It's a trivial change I think. But letting __swap_cache_replace_folio
> setup new's swap and flags may deduplicate some code. So I thought
> maybe this can be better cleaned up later. So for now I just add a
> debug check here that `entry == new->swap`.
> 
> And the debug check does imply that we can just drop the entry params
> in this patch, there will be no feature change.

Well, the current API as you introduce it here is confusing, as it's not 
clear who is supposed to initialize what.

So better to it cleanly right from the start.

> 
>> Similarly, can't we obtain "mapping" from new?
> 
> This is doable. But this patch is only an intermediate patch, next
> commit will let the pass in ci instead. Of course the `ci` can be
> retrieved from `entry` directly too, but it's the caller's
> responsibility to lock the `ci`, so passing in a locked ci explicitly
> might be more intuitive? Also might save a tiny bit of CPU time from
> recalculating and load the `ci`.

Well, no other swap_cache_* functions consumes an address space, right?

-- 
Cheers

David / dhildenb