Re: [PATCH v3 1/4] mm: swap: Remove CLUSTER_FLAG_HUGE from swap_cluster_info:flags

[David Hildenbrand]

On 04.03.24 22:55, Ryan Roberts wrote:
> On 04/03/2024 20:50, David Hildenbrand wrote:
> > > > > This is the existing free_swap_and_cache(). I think _swap_info_get() would
> > > > > break
> > > > > if this could race with swapoff(), and __swap_entry_free() looks up the cluster
> > > > > from an array, which would also be freed by swapoff if racing:
> > > > >
> > > > > int free_swap_and_cache(swp_entry_t entry)
> > > > > {
> > > > >       struct swap_info_struct *p;
> > > > >       unsigned char count;
> > > > >
> > > > >       if (non_swap_entry(entry))
> > > > >           return 1;
> > > > >
> > > > >       p = _swap_info_get(entry);
> > > > >       if (p) {
> > > > >           count = __swap_entry_free(p, entry);
> > > >
> > > > If count dropped to 0 and
> > > >
> > > > >           if (count == SWAP_HAS_CACHE)
> > > >
> > > >
> > > > count is now SWAP_HAS_CACHE, there is in fact no swap entry anymore. We removed
> > > > it. That one would have to be reclaimed asynchronously.
> > > >
> > > > The existing code we would call swap_page_trans_huge_swapped() with the SI it
> > > > obtained via _swap_info_get().
> > > >
> > > > I also don't see what should be left protecting the SI. It's not locked anymore,
> > > > the swapcounts are at 0. We don't hold the folio lock.
> > > >
> > > > try_to_unuse() will stop as soon as si->inuse_pages is at 0. Hm ...
> > >
> > > But, assuming the caller of free_swap_and_cache() acquires the PTL first, I
> > > think this all works out ok? While free_swap_and_cache() is running,
> > > try_to_unuse() will wait for the PTL. Or if try_to_unuse() runs first, then
> > > free_swap_and_cache() will never be called because the swap entry will have been
> > > removed from the PTE?
> >
> > But can't try_to_unuse() run, detect !si->inuse_pages and not even bother about
> > scanning any further page tables?
> >
> > But my head hurts from digging through that code.
>
> Yep, glad I'm not the only one that gets headaches from swapfile.c.
>
> > Let me try again:
> >
> > __swap_entry_free() might be the last user and result in "count == SWAP_HAS_CACHE".
> >
> > swapoff->try_to_unuse() will stop as soon as soon as si->inuse_pages==0.
> >
> >
> > So the question is: could someone reclaim the folio and turn si->inuse_pages==0,
> > before we completed swap_page_trans_huge_swapped().
> >
> > Imagine the following: 2 MiB folio in the swapcache. Only 2 subpages are still
> > references by swap entries.
> >
> > Process 1 still references subpage 0 via swap entry.
> > Process 2 still references subpage 1 via swap entry.
> >
> > Process 1 quits. Calls free_swap_and_cache().
> > -> count == SWAP_HAS_CACHE
> > [then, preempted in the hypervisor etc.]
> >
> > Process 2 quits. Calls free_swap_and_cache().
> > -> count == SWAP_HAS_CACHE
> >
> > Process 2 goes ahead, passes swap_page_trans_huge_swapped(), and calls
> > __try_to_reclaim_swap().
> >
> > __try_to_reclaim_swap()->folio_free_swap()->delete_from_swap_cache()->put_swap_folio()->
> > free_swap_slot()->swapcache_free_entries()->swap_entry_free()->swap_range_free()->
> > ...
> > WRITE_ONCE(si->inuse_pages, si->inuse_pages - nr_entries);
> >
> >
> > What stops swapoff to succeed after process 2 reclaimed the swap cache but
> > before process 1 finished its call to swap_page_trans_huge_swapped()?
>
> Assuming you are talking about anonymous memory, process 1 has the PTL while
> it's executing free_swap_and_cache(). try_to_unuse() iterates over every vma in
> every mm, and it swaps-in a page for every PTE that holds a swap entry for the
> device being swapoff'ed. It takes the PTL while converting the swap entry to
> present PTE - see unuse_pte(). Process 1 must have beaten try_to_unuse() to the
> particular pte, because if try_to_unuse() got there first, it would have
> converted it from a swap entry to present pte and process 1 would never even
> have called free_swap_and_cache(). So try_to_unuse() will eventually wait on the
> PTL until process 1 has released it after free_swap_and_cache() completes. Am I
> missing something? Because that part feels pretty clear to me.

Why should try_to_unuse() do *anything* if it already finds
si->inuse_pages == 0 because we (p1 } p2) just freed the swapentries and 
process 2 managed to free the last remaining swapcache entry?

I'm probably missing something important :)

try_to_unuse() really starts with

	if (!READ_ONCE(si->inuse_pages))
		goto success;

--
Cheers,

David / dhildenb