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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux