Received-SPF: pass (google.com: domain of linux-kernel+bounces-91365-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <3ae2da13-c33a-402f-9091-2c7328aea66a@arm.com>
Date: Mon, 4 Mar 2024 22:34:56 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/4] mm: swap: Remove CLUSTER_FLAG_HUGE from
 swap_cluster_info:flags
Content-Language: en-GB
To: David Hildenbrand <david@redhat.com>,
 Andrew Morton <akpm@linux-foundation.org>,
 Matthew Wilcox <willy@infradead.org>, Huang Ying <ying.huang@intel.com>,
 Gao Xiang <xiang@kernel.org>, Yu Zhao <yuzhao@google.com>,
 Yang Shi <shy828301@gmail.com>, Michal Hocko <mhocko@suse.com>,
 Kefeng Wang <wangkefeng.wang@huawei.com>, Hugh Dickins <hughd@google.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org
References: <20231025144546.577640-1-ryan.roberts@arm.com>
 <20231025144546.577640-2-ryan.roberts@arm.com>
 <d108bd79-086b-4564-838b-d41afa055137@redhat.com>
 <6541e29b-f25a-48b8-a553-fd8febe85e5a@redhat.com>
 <ee760679-7e3c-4a35-ad53-ca98b598ead5@arm.com>
 <ba9101ae-a618-4afc-9515-a61f25376390@arm.com>
 <2934125a-f2e2-417c-a9f9-3cb1e074a44f@redhat.com>
 <049818ca-e656-44e4-b336-934992c16028@arm.com>
 <d2fbfdd0-ad61-4fe2-a976-4dac7427bfc9@redhat.com>
 <b642c7ff-c452-4066-ac12-dbf05e215cb9@arm.com>
 <949b6c22-d737-4060-9ca1-a69d8e986d90@redhat.com>
 <9ed743a7-0c5d-49d9-b8b2-d58364df1f5f@arm.com>
 <65a66eb9-41f8-4790-8db2-0c70ea15979f@redhat.com>
 <6cfc022a-0c7a-4fe6-aaa4-3d28aeacc982@arm.com>
 <3d47ae7d-297a-441e-941c-5b2e34ba8759@redhat.com>
From: Ryan Roberts <ryan.roberts@arm.com>
In-Reply-To: <3d47ae7d-297a-441e-941c-5b2e34ba8759@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

+ Hugh

On 04/03/2024 22:02, David Hildenbrand wrote:
> 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?

Yeah ok. For some reason I thought unuse_mm() was iterating over all mms and so
the `while (READ_ONCE(si->inuse_pages))` was only evaluated after iterating over
every mm. Oops.

So yes, I agree with you; I think this is broken. And I'm a bit worried this
could be a can of worms; By the same logic, I think folio_free_swap(),
swp_swapcount() and probably others are broken in the same way.

I wonder if we are missing something here? I've added Hugh - I see he has a lot
of commits in this area, perhaps he has some advice?

Thanks,
Ryan


> 
> I'm probably missing something important :)
> 
> try_to_unuse() really starts with
> 
>     if (!READ_ONCE(si->inuse_pages))
>         goto success;
>