From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 3B14F2C859 for ; Fri, 20 Dec 2024 04:37:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734669478; cv=none; b=BG5mRY9T4pg06g8PC+kGF/y31HPhXq55IuJd8YqSNf3cnVWLXtHEi9foybxLNOKZgRxsimMRUYv0Q6Rm4bxazeXKh8a3HBtPauHUOB86fM8lhpRF6htokVGPSZBFIRs5ip5BhdY6kKfd9f9ePDKURjCgr7NyesgSofDO24n6ZCg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734669478; c=relaxed/simple; bh=gQOdekNHbIAhCpzzpzszW4q3tzIhhEbT1F8vMGdu6VY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=R677tEoj8KKS2JjqpTDWEa6mypR7XJSSSYFn1Uz+/qAZb/D+ZbSLpTbrBqsUAK4ISthjGptYnoTkjpPPw396nqclm+FOKoCDZ9WNflVleNqyemmDIvsg9C/lAve78Hea34uYb6L/mL22qA0jw4c6HlIbdxzlqHZ2loOF5eDpjDw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=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 39A151480; Thu, 19 Dec 2024 20:38:22 -0800 (PST) Received: from [10.162.42.20] (K4MQJ0H1H2.blr.arm.com [10.162.42.20]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BD5EA3F720; Thu, 19 Dec 2024 20:37:50 -0800 (PST) Message-ID: Date: Fri, 20 Dec 2024 10:07:47 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] mm: migration :shared anonymous migration test is failing To: Baolin Wang , Donet Tom , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Cc: Ritesh Harjani , "Aneesh Kumar K . V" , Zi Yan , David Hildenbrand , shuah Khan References: <20241219124717.4907-1-donettom@linux.ibm.com> <36f9ab13-e057-40a0-8d0b-9939df056fc6@linux.ibm.com> <4d76321e-7905-46e6-8105-f09afde516ff@linux.alibaba.com> Content-Language: en-US From: Dev Jain In-Reply-To: <4d76321e-7905-46e6-8105-f09afde516ff@linux.alibaba.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 20/12/24 9:02 am, Baolin Wang wrote: > > > On 2024/12/20 11:12, Donet Tom wrote: >> >> On 12/20/24 08:01, Baolin Wang wrote: >>> >>> >>> On 2024/12/19 20:47, Donet Tom wrote: >>>> The migration selftest is currently failing for shared anonymous >>>> mappings due to a race condition. >>>> >>>> During migration, the source folio's PTE is unmapped by nuking the >>>> PTE, flushing the TLB,and then marking the page for migration >>>> (by creating the swap entries). The issue arises when, immediately >>>> after the PTE is nuked and the TLB is flushed, but before the page >>>> is marked for migration, another thread accesses the page. This >>>> triggers a page fault, and the page fault handler invokes >>>> do_pte_missing() instead of do_swap_page(), as the page is not yet >>>> marked for migration. >>>> >>>> In the fault handling path, do_pte_missing() calls __do_fault() >>>> ->shmem_fault() -> shmem_get_folio_gfp() -> filemap_get_entry(). >>>> This eventually calls folio_try_get(), incrementing the reference >>>> count of the folio undergoing migration. The thread then blocks >>>> on folio_lock(), as the migration path holds the lock. This >>>> results in the migration failing in __migrate_folio(), which expects >>>> the folio's reference count to be 2. However, the reference count is >>>> incremented by the fault handler, leading to the failure. >>>> >>>> The issue arises because, after nuking the PTE and before marking the >>>> page for migration, the page is accessed. To address this, we have >>>> updated the logic to first nuke the PTE, then mark the page for >>>> migration, and only then flush the TLB. With this patch, If the >>>> page is >>>> accessed immediately after nuking the PTE, the TLB entry is still >>>> valid, so no fault occurs. After marking the page for migration, >>> >>> IMO, I don't think this assumption is correct. At this point, the >>> TLB entry might also be evicted, so a page fault could still occur. >>> It's just a matter of probability. >> In this patch, we mark the page for migration before flushing the TLB. >> This ensures that if someone accesses the page after the TLB flush, >> the page fault will occur and in the page fault handler will wait for >> the >> migration to complete. So migration will not fail >> >> Without this patch, if someone accesses the page after the TLB flush >> but before it is marked for migration, the migration will fail. > > Actually my concern is the same as David's (I did not see David's > reply before sending my comments), which is that your patch does not > "rules out all cases". I like this solution but really the proper solution for this one was to atomically set the migration entry IMHO. > >>> Additionally, IIUC, if another thread is accessing the shmem folio >>> causing the migration to fail, I think this is expected, and >>> migration failure is not a vital issue? >>> >> In my case, the shmem migration test is always failing, >> even after retries. Would it be correct to consider this >> as expected behavior? > > IMHO I think your test case is too aggressive and unlikely to occur in > real-world scenarios. Additionally, as I mentioned, migration failure > is not a vital issue in the system, and some temporary refcnt can also > lead to migration failure if you want to create such test cases. So > personally, I don't think it is worthy doing. Agreed, AFAIR the test case starts faulting exactly on those pages which we want to migrate, making this a very artificial scenario.