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

[David Hildenbrand]

> > > 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.

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()?



> That just leaves shmem... I suspected there might be some serialization between
> shmem_unuse() (called from try_to_unuse()) and the shmem free_swap_and_cache()
> callsites, but I can't see it. Hmm...
>
> > Would performing the overall operation under lock_cluster_or_swap_info help? Not
> > so sure :(
>
> No - that function relies on being able to access the cluster from the array in
> the swap_info and lock it. And I think that array has the same lifetime as
> swap_map, so same problem. You'd need get_swap_device()/put_swap_device() and a
> bunch of refactoring for the internals not to take the locks, I guess. I think
> its doable, just not sure if neccessary...

Agreed.

--
Cheers,

David / dhildenb