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 D071615B133 for ; Tue, 2 Jul 2024 12:36:46 +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=1719923809; cv=none; b=gGHQDfbD+eO9eEsOSjfAZeJvpiTtmbw+1hystZXWDqqmprpTco4d0n+D4bq2vDTphre3ROeOaqumCriXJc5FHmlsYG03+j9K6bwGaloeNvSd2MdSB9Ir/4SKy5lsGpiwlKNFHCNsxHW0Gvbknqnl4spkXj5TOvhZZmRdOR2wRSw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1719923809; c=relaxed/simple; bh=pfO0nG4Gnmn6alBJrTjUAgM/3BtOYt7KT8G1Qrd1Lvo=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=iwH4jp9CdX4FwYIZ0cKmnfdwopDfVo99jGfMPmdrdQ849xUG8pxwNZgOFTNdfDfqf3429ZcauEpx9PZUDpSrYr0iDqA44ZnAgNaTlvQTSkm5DF0NhfNuHpY5ezwERGNpxxLdUU9snlAiPyBwtJb9ap3LMhFHaNBMYgJhVKtc7iE= 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 3DDAD367; Tue, 2 Jul 2024 05:37:11 -0700 (PDT) Received: from [10.1.32.193] (XHFQ2J9959.cambridge.arm.com [10.1.32.193]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F22633F762; Tue, 2 Jul 2024 05:36:44 -0700 (PDT) Message-ID: <905e1319-7b20-4812-b052-8031a3c4dcf3@arm.com> Date: Tue, 2 Jul 2024 13:36:43 +0100 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: [v5 PATCH] arm64: mm: force write fault for atomic RMW instructions Content-Language: en-GB To: David Hildenbrand , Catalin Marinas , Yang Shi Cc: "Christoph Lameter (Ampere)" , will@kernel.org, anshuman.khandual@arm.com, scott@os.amperecomputing.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Jinjiang Tu References: <20240626191830.3819324-1-yang@os.amperecomputing.com> <773c8be7-eb73-010c-acea-1c2fefd65b84@gentwo.org> <200c5d06-c551-4847-adaf-287750e6aac4@os.amperecomputing.com> <3743d7e1-0b79-4eaf-82d5-d1ca29fe347d@arm.com> <34f2d8c6-50fa-4a8a-82a8-d417eb30ea70@redhat.com> From: Ryan Roberts In-Reply-To: <34f2d8c6-50fa-4a8a-82a8-d417eb30ea70@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 02/07/2024 12:22, David Hildenbrand wrote: > On 02.07.24 12:26, Ryan Roberts wrote: >> On 01/07/2024 20:43, Catalin Marinas wrote: >>> On Fri, Jun 28, 2024 at 11:20:43AM -0700, Yang Shi wrote: >>>> On 6/28/24 10:24 AM, Catalin Marinas wrote: >>>>> This patch does feel a bit like working around a non-optimal user choice >>>>> in kernel space. Who knows, madvise() may even be quicker if you do a >>>>> single call for a larger VA vs touching each page. >>>> >>>> IMHO, I don't think so. I viewed this patch to solve or workaround some ISA >>>> inefficiency in kernel. Two faults are not necessary if we know we are >>>> definitely going to write the memory very soon, right? >>> >>> I agree the Arm architecture behaviour is not ideal here and any >>> timelines for fixing it in hardware, if they do happen, are far into the >>> future. Purely from a kernel perspective, what I want though is make >>> sure that longer term (a) we don't create additional maintenance burden >>> and (b) we don't keep dead code around. >>> >>> Point (a) could be mitigated if the architecture is changed so that any >>> new atomic instructions added to this range would also come with >>> additional syndrome information so that we don't have to update the >>> decoding patterns. >>> >>> Point (b), however, depends on the OpenJDK and the kernel versions in >>> distros. Nick Gasson kindly provided some information on the OpenJDK >>> changes. The atomic_add(0) change happened in early 2022, about 5-6 >>> months after MADV_POPULATE_WRITE support was added to the kernel. What's >>> interesting is Ampere already contributed MADV_POPULATE_WRITE support to >>> OpenJDK a few months ago: >>> >>> https://github.com/openjdk/jdk/commit/a65a89522d2f24b1767e1c74f6689a22ea32ca6a >>> >>> The OpenJDK commit lacks explanation but what I gathered from the diff >>> is that this option is the preferred one in the presence of THP (which >>> most/all distros enable by default). If we merge your proposed kernel >>> patch, it will take time before it makes its way into distros. I'm >>> hoping that by that time, distros would have picked a new OpenJDK >>> version already that doesn't need the atomic_add(0) pattern. If that's >>> the case, we end up with some dead code in the kernel that's almost >>> never exercised. >>> >>> I don't follow OpenJDK development but I heard that updates are dragging >>> quite a lot. I can't tell whether people have picked up the >>> atomic_add(0) feature and whether, by the time a kernel patch would make >>> it into distros, they'd also move to the MADV_POPULATE_WRITE pattern. >>> >>> There's a point (c) as well on the overhead of reading the faulting >>> instruction. I hope that's negligible but I haven't measured it. >>> >> >> Just to add to this, I note the existing kernel behaviour is that if a write >> fault happens in a region that has a (RO) huge zero page mapped at PMD level, >> then the PMD is shattered, the PTE of the fault address is populated with a >> writable page and the remaining PTEs are populated with order-0 zero pages >> (read-only). > > That also recently popped up in [1]. CCing Jinjiang. Ever since I > replied there, I also thought some more about that handling in regard to the > huge zeropage. > >> >> This seems like odd behaviour to me. Surely it would be less effort and more >> aligned with the app's expectations to notice the huge zero page in the PMD, >> remove it, and install a THP, as would have been done if pmd_none() was true? I >> don't think there is a memory bloat argument here because, IIUC, with the >> current behaviour, khugepaged would eventually upgrade it to a THP anyway? > > One detail: depending on the setting of khugepaged_max_ptes_none. zeropages > are treated like pte_none. But in the common case, that setting is left alone. Ahh, got it. So in the common case, khugepaged won't actually collapse unless/until a bunch more write faults occur in the 2M region, and in that case there is a risk that changing this behaviour could lead to a memory bloat regression. > >> >> Changing to this new behaviour would only be a partial solution for your use >> case, since you would still have 2 faults. But it would remove the cost of the >> shattering and ensure you have a THP immediately after the write fault. But I >> can't think of a reason why this wouldn't be a generally useful change >> regardless? Any thoughts? > > The "let's read before we write" as used by QEMU migration code is the desire > to not waste memory by populating the zeropages. Deferring consuming memory > until really required. > >     /* >      * We read one byte of each page; this will preallocate page tables if >      * required and populate the shared zeropage on MAP_PRIVATE anonymous memory >      * where no page was populated yet. This might require adaption when >      * supporting other mappings, like shmem. >      */ So QEMU is concerned with preallocatiing page tables? I would have thought you could make that a lot more efficient with an explicit MADV_POPULATE_PGTABLE call? (i.e. 1 kernel call vs 1 call per 2M, allocate all the pages in one trip through the allocator, fewer pud/pmd lock/unlocks, etc). TBH I always assumed in the past the that huge zero page is only useful because its a placeholder for a real THP that would be populated on write. But that's obviously not the case at the moment. So other than a hack to preallocate the pgtables with only 1 fault per 2M, what other benefits does it have? > > > Without THP this works as expected. With THP this currently also works as > expected, but of course with the price [1] of not getting anon THP > immediately, which usually we don't care about. As you note, khugepaged might > fix this up later. > > If we disable the huge zeropage, we would get anon THPs when reading instead of > small zeropages. I wasn't aware of that behaviour either. Although that sounds like another reason why allocating a THP over the huge zero page on write fault should be the "more consistent" behaviour. > > As reply to [1], I suggested using preallcoation (using MADV_POPULATE_WRITE) > when we really care about that performance difference, which would also > avoid the huge zeropage completely, but it's also not quite optimal in some cases. I could imagine some cases could benefit from a MADV_POPULATE_WRITE_ON_FAULT, which would just mark the VMA so that any read fault is upgraded to write. > > > I don't really know what to do here: changing the handling for the huge zeropage > only unconditionally does not sound too wrong, but the change in behavior > might (or might not) be desired for some use cases. > > Reading from unpopulated memory can be a clear sign that really the shared zeropage > is desired (as for QEMU), and concurrent memory preallcoation/population should > ideally use MADV_POPULATE_WRITE. Maybe there are some details buried in [2] > regarding > the common use cases for the huge zeropage back than. The current huge zero page behavior on write fault sounds wonky to me. But I agree there are better and more complete solutions to the identified use cases. So unless something pops up where the change is a clear benefit, I guess better to be safe and leave as is. > > [1] https://lkml.kernel.org/r/740d7379-3e3d-4c8c-4350-6c496969db1f@huawei.com > [2] https://lwn.net/Articles/517465/ >