From patchwork Thu Aug 1 08:16:57 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dev Jain X-Patchwork-Id: 13749962 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A3CEDC3DA4A for ; Thu, 1 Aug 2024 08:19:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: MIME-Version:Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References:List-Owner; bh=alG5mqDIhrlFZ8ZHILXT9MTipic/5YvDk8vbHhTbomE=; b=QctPr+Ko+n52nh0ecJc/bEh0C2 h7K0GtbZLzHUZf474Zr/tEw3umyS/9UkNsx4iVafl5V+4E8Pf1Ky+6pVgVhTvEtml5bqIpw4SaDOf zKrr6eNeqN+0RXYkjP8dWNi5CExeE4hrrIi3ehPxjsoFKuH5Qp5vAMSwbq6a4RpWzTBLJ7iK5Yuaz XYyiyEpQJJGk60vYvwLF8S1coQ2PVm2/hk3jc8jKa64mBfwsoPiMd1/xDMXEhg0eQWUvEpaDhGG1f LUy6QOudLef72zvYQwkmwIUruJKsBOVmve9YbnODV9ziIHo/eMTyf2bHPh1wYToKkOXnGShp8El95 nfSDlhqA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sZR1e-00000004JbU-2Ysh; Thu, 01 Aug 2024 08:19:06 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sZR1B-00000004JRM-0ghJ for linux-arm-kernel@lists.infradead.org; Thu, 01 Aug 2024 08:18:38 +0000 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 3A1051007; Thu, 1 Aug 2024 01:19:00 -0700 (PDT) Received: from e116581.blr.arm.com (e116581.arm.com [10.162.42.27]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id D855F3F766; Thu, 1 Aug 2024 01:18:25 -0700 (PDT) From: Dev Jain To: akpm@linux-foundation.org, david@redhat.com, willy@infradead.org Cc: ryan.roberts@arm.com, anshuman.khandual@arm.com, catalin.marinas@arm.com, cl@gentwo.org, vbabka@suse.cz, mhocko@suse.com, apopple@nvidia.com, osalvador@suse.de, baolin.wang@linux.alibaba.com, dave.hansen@linux.intel.com, will@kernel.org, baohua@kernel.org, ioworker0@gmail.com, gshan@redhat.com, mark.rutland@arm.com, kirill.shutemov@linux.intel.com, hughd@google.com, aneesh.kumar@kernel.org, yang@os.amperecomputing.com, peterx@redhat.com, broonie@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Dev Jain Subject: Race condition observed between page migration and page fault handling on arm64 machines Date: Thu, 1 Aug 2024 13:46:57 +0530 Message-Id: <20240801081657.1386743-1-dev.jain@arm.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240801_011837_312220_373EDA7F X-CRM114-Status: GOOD ( 22.19 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org I and Ryan had a discussion and we thought it would be best to get feedback from the community. The migration mm selftest currently fails on arm64 for shared anon mappings, due to the following race: Migration: Page fault: try_to_migrate_one(): handle_pte_fault(): 1. Nuke the PTE PTE has been deleted => do_pte_missing() 2. Mark the PTE for migration PTE has not been deleted but is just not "present" => do_swap_page() In the test, the parent thread migrates a single page between nodes, and the children threads read on that page. The race happens when the PTE has been nuked, and before it gets marked for migration, the reader faults and checks if the PTE is missing, and calls do_pte_missing() instead of the correct do_swap_page(); the latter implies that the reader ends up calling migration_entry_wait() to wait for PTEs to get corrected. The former path ends up following this: do_fault() -> do_read_fault() -> __do_fault() -> vma->vm_ops->fault, which is shmem_fault() -> shmem_get_folio_gfp() -> filemap_get_entry(), which then calls folio_try_get() and takes a reference on the folio which is under migration. Returning back, the reader blocks on folio_lock() since the migration path has the lock, and migration ends up failing in folio_migrate_mapping(), which expects a reference count of 2 on the folio. The following hack makes the race vanish: mm/rmap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) It's a hack because the invalidation is non-atomic, and pte_mkinvalid() is defined only on arm64. I could think of the following solutions: 1. Introduce (atomic)_pte_mkinvalid() for all arches and call the arch-API if defined, else call ptep_get_and_clear(). The theoretical race would still exist for other arches. 2. Prepare the migration swap entry and do an atomic exchange to fill the PTE (this currently seems the best option to me, but I have not tried it out). 3. In try_to_migrate_one(), before the nuking, freeze the refcount of the folio. This would solve the race, but then we will have to defer all the reference releases till later, and I don't know whether that's correct or feasible. 4. Since the extra refcount being taken in filemap_get_entry() is due to loading the folio from the pagecache, delete/invalidate the folio in the pagecache until migration is complete. I tried with some helpers present in mm/filemap.c to do that but I guess I am not aware of the subtleties, and I end up triggering a BUG() somewhere. 5. In do_fault(), under the if block, we are already checking whether this is just a temporary clearing of the PTE. We can take that out of the if block, but then that would be a performance bottleneck since we are taking the PTL? Thanks, Dev diff --git a/mm/rmap.c b/mm/rmap.c index e8fc5ecb59b2..bb10b3376595 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -2126,7 +2126,9 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, * transition on a cached TLB entry is written through * and traps if the PTE is unmapped. */ - pteval = ptep_get_and_clear(mm, address, pvmw.pte); + pteval = pte_mkinvalid(*(pvmw.pte)); + *(pvmw.pte) = pteval; set_tlb_ubc_flush_pending(mm, pteval, address); } else {