From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7BBA0C48BF6 for ; Mon, 4 Mar 2024 22:35:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C8F456B0083; Mon, 4 Mar 2024 17:35:03 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C18146B0088; Mon, 4 Mar 2024 17:35:03 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A91A26B008A; Mon, 4 Mar 2024 17:35:03 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 9440D6B0083 for ; Mon, 4 Mar 2024 17:35:03 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 5EAAF140D6A for ; Mon, 4 Mar 2024 22:35:03 +0000 (UTC) X-FDA: 81860813286.09.7F0C8EF Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf17.hostedemail.com (Postfix) with ESMTP id 6A57D4001B for ; Mon, 4 Mar 2024 22:35:01 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=none; spf=pass (imf17.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709591701; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HJiihZWyEUt93r6fvzQ/v0ebpFb9KXXzjnSYJFaTk6o=; b=uGAte1e7amy5IKcezRGZGDxgUG9xMhDctxDEXfei9jTQCVdIG3Qg+2GEPTxY1oV8rYGWaw vHfylr5DonxzC0UFDaqX5AxffaITfOkHEykxfHH/GI84HntBhghgylGcjs7aw1uHX1mXcp bS89UnC6z+t2jNi0QYmKcijnfkzTb2k= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709591701; a=rsa-sha256; cv=none; b=UsntNcnQibppt3lA8Wf1uBVrFJYUhRvpqWlqrE6NLC6LcfbVgtW7rN/RofncLUmVXvAhpp olGE2rEbKFQieMx3niWmkLKnsUW9oSrdCp3krRBdZZegOr6Px/GHMswSLr+VREEs9ChUKe CRnfCwFBs7hYjP9tp7H9B60jRvV5pjU= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=none; spf=pass (imf17.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0F9382F4; Mon, 4 Mar 2024 14:35:37 -0800 (PST) Received: from [10.57.68.92] (unknown [10.57.68.92]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 486603F73F; Mon, 4 Mar 2024 14:34:58 -0800 (PST) Message-ID: <3ae2da13-c33a-402f-9091-2c7328aea66a@arm.com> Date: Mon, 4 Mar 2024 22:34:56 +0000 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 , Andrew Morton , Matthew Wilcox , Huang Ying , Gao Xiang , Yu Zhao , Yang Shi , Michal Hocko , Kefeng Wang , Hugh Dickins 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> <6541e29b-f25a-48b8-a553-fd8febe85e5a@redhat.com> <2934125a-f2e2-417c-a9f9-3cb1e074a44f@redhat.com> <049818ca-e656-44e4-b336-934992c16028@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 In-Reply-To: <3d47ae7d-297a-441e-941c-5b2e34ba8759@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Stat-Signature: yre9x1tcsnzmkbd9mobgro14d8htxy4r X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 6A57D4001B X-Rspam-User: X-HE-Tag: 1709591701-534719 X-HE-Meta: U2FsdGVkX19sMx86NiIEk59PwxDHkY/eYEDscgoNmiogVHIYQk0HMxCkWUzNZzqa4ebXkXIzb/vzuaF23NAu/r4j66moa5hA1El6q2zwApnq7j9zurv40AIid3sz+Pz+DCj/YR0xd9CjOv9fgvCgIXoOee2X39WC074SVSrJnWsv+/jewmGz6U5Mh0gbCdkyjujuY8Zdxtmp1jwkQb13StkMIJk/bL67xOySj8RfWRd/+vh8pUVBr/Vi66txHBz1qfdz569oZbNzXSI9M4SN9kInm2DVtxWjo/KeNLghFHTtyYqKsZvaHKMgVjLhTgvkWtV0+tMGibR8kuCFVU7JN9/BrOna9rFEEHQKCmnTqoMFTtySEvvkckfW4wuIdk7hpNL1eH5TYbJHhMBXiugHOoINm5NySb9GzaXC3LwOpbJwy127q2YAdoO4Phou9d0hvRppk08OlsyGGjrqmfJUCQuhe1mqIsrZdMOQoqVI1PgbnVJFjGrmIBBCMHmJn6Q0yXH0zHZR9XgX9dH+I8aOccYfyEYhvDwJc5CYBUFHQvYb/ZhIZXOygjI4ffJfl8O9lGst6lTcZc3CsbQFfPCO9/Q33C6RtG0BGZFVsqxoo7/RoMWWVY6v3w28b6vc3TPD/FzsWJDZIoOGG++WiqiULalE41bGmfdTux0Rq6aGuP478NO1HtuxJtgZjinybv/6pKDJmXj6lgyzRH3rsULXayX+n4A8Og8MA4a17CQRZPL/VC6Vi1aJi9z+DoFVxKBPJecZD3chr446rcz/XnsoL+JLY3T7iw8erfOAJBIRG9xh8eCwfiwPCuHL3/23lbEqtMKZwmfVbUhuIYYCJTFwqqIdn7WRqd+3MHSxlF/EzyvkDStjkPxCSTsQ/UXmt1woEgdpFmymFwkfFyFk8SAGLa5TikV1y6cmoGiUjCWeZmGjGmdNwbZjYAKB0T7+N+Uxkk1trobOn/vFElSHhhH ijU6Gql/ vel3CTIuILXY0cTW+Sl1AU8IuyMrjvloyBg/Xbiurc4/FmBv+NLPa5S2/KSQy1ODBKEio4rEQm6WnkYtQNdvPG1pBSw0j2iIOxpFdWqofblXhb75tUlbCAV+z5dInNrEb/Jpj2a9MNodc/iofyaeTVAmyLqqQvs0+RIfa/g+LLMhGvmO5tp7cjqeST+E05TLvNaky3L2eBtI6RNDXk9tqHjP6AhG+FCiPtmM/ X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: + 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; >