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 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 A3793C48260 for ; Fri, 16 Feb 2024 12:26:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=UZCa21xcruTisGudvM56ZvZ+kzeOfaqiuE3mwJO1m3E=; b=0so+iBR8Myk8ty PYm31GZom2nIEbos0AKyLsk25/160dgthIsicOMcPH81bZLWOp92Y7XeOt4ZKhjGspyhqrQRQ6eTS EEqxZ0cTnPrsrONYineeiusUOVHZfyJdrNFQeyipSwMCQfRBzLgCoiRBFuocicW+Dv0mEIcjWMZ0N bbr/OZwFZ6dG9Civw9a1S3VFyah42Q+8PbILxw9RZoFM2r0ORwCAlmOJzcbej1R0PfA9NefjEpZnh Yq08qvrQDJ0cUANsVR85Qk2+ABSjVOMeMB4qk2e6kXew/pchP5g2JuQTeETdeEmVT1pcZ1duMVQZ4 b8iSkEmJA+q+BYrSP3BQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1raxHr-00000002HIV-16fC; Fri, 16 Feb 2024 12:25:51 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1raxHo-00000002HHS-0UF1 for linux-arm-kernel@lists.infradead.org; Fri, 16 Feb 2024 12:25:49 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 64A1F61F6F; Fri, 16 Feb 2024 12:25:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id F0177C433C7; Fri, 16 Feb 2024 12:25:41 +0000 (UTC) Date: Fri, 16 Feb 2024 12:25:39 +0000 From: Catalin Marinas To: Ryan Roberts Cc: Will Deacon , Ard Biesheuvel , Marc Zyngier , James Morse , Andrey Ryabinin , Andrew Morton , Matthew Wilcox , Mark Rutland , David Hildenbrand , Kefeng Wang , John Hubbard , Zi Yan , Barry Song <21cnbao@gmail.com>, Alistair Popple , Yang Shi , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , "H. Peter Anvin" , linux-arm-kernel@lists.infradead.org, x86@kernel.org, linuxppc-dev@lists.ozlabs.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 12/18] arm64/mm: Wire up PTE_CONT for user mappings Message-ID: References: <20240215103205.2607016-1-ryan.roberts@arm.com> <20240215103205.2607016-13-ryan.roberts@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240215103205.2607016-13-ryan.roberts@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240216_042548_264601_E869929D X-CRM114-Status: GOOD ( 31.48 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Feb 15, 2024 at 10:31:59AM +0000, Ryan Roberts wrote: > arch/arm64/mm/contpte.c | 285 +++++++++++++++++++++++++++++++ Nitpick: I think most symbols in contpte.c can be EXPORT_SYMBOL_GPL(). We don't expect them to be used by random out of tree modules. In fact, do we expect them to end up in modules at all? Most seem to be called from the core mm code. > +#define ptep_get_lockless ptep_get_lockless > +static inline pte_t ptep_get_lockless(pte_t *ptep) > +{ > + pte_t pte = __ptep_get(ptep); > + > + if (likely(!pte_valid_cont(pte))) > + return pte; > + > + return contpte_ptep_get_lockless(ptep); > +} [...] > +pte_t contpte_ptep_get_lockless(pte_t *orig_ptep) > +{ > + /* > + * Gather access/dirty bits, which may be populated in any of the ptes > + * of the contig range. We may not be holding the PTL, so any contiguous > + * range may be unfolded/modified/refolded under our feet. Therefore we > + * ensure we read a _consistent_ contpte range by checking that all ptes > + * in the range are valid and have CONT_PTE set, that all pfns are > + * contiguous and that all pgprots are the same (ignoring access/dirty). > + * If we find a pte that is not consistent, then we must be racing with > + * an update so start again. If the target pte does not have CONT_PTE > + * set then that is considered consistent on its own because it is not > + * part of a contpte range. > +*/ I can't get my head around this lockless API. Maybe it works fine (and may have been discussed already) but we should document what the races are, why it works, what the memory ordering requirements are. For example, the generic (well, x86 PAE) ptep_get_lockless() only needs to ensure that the low/high 32 bits of a pte are consistent and there are some ordering rules on how these are updated. Does the arm64 implementation only need to be correct w.r.t. the access/dirty bits? Since we can read orig_ptep atomically, I assume the only other updates from unfolding would set the dirty/access bits. > + > + pgprot_t orig_prot; > + unsigned long pfn; > + pte_t orig_pte; > + pgprot_t prot; > + pte_t *ptep; > + pte_t pte; > + int i; > + > +retry: > + orig_pte = __ptep_get(orig_ptep); > + > + if (!pte_valid_cont(orig_pte)) > + return orig_pte; > + > + orig_prot = pte_pgprot(pte_mkold(pte_mkclean(orig_pte))); > + ptep = contpte_align_down(orig_ptep); > + pfn = pte_pfn(orig_pte) - (orig_ptep - ptep); > + > + for (i = 0; i < CONT_PTES; i++, ptep++, pfn++) { > + pte = __ptep_get(ptep); > + prot = pte_pgprot(pte_mkold(pte_mkclean(pte))); We don't have any ordering guarantees in how the ptes in this range are read or written in the contpte_set_ptes() and the fold/unfold functions. We might not need them given all the other checks below but it's worth adding a comment. > + > + if (!pte_valid_cont(pte) || > + pte_pfn(pte) != pfn || > + pgprot_val(prot) != pgprot_val(orig_prot)) > + goto retry; I think this also needs some comment. I get the !pte_valid_cont() check to attempt retrying when racing with unfolding. Are the other checks needed to detect re-folding with different protection or pfn? > + > + if (pte_dirty(pte)) > + orig_pte = pte_mkdirty(orig_pte); > + > + if (pte_young(pte)) > + orig_pte = pte_mkyoung(orig_pte); > + } After writing the comments above, I think I figured out that the whole point of this loop is to check that the ptes in the contig range are still consistent and the only variation allowed is the dirty/young state to be passed to the orig_pte returned. The original pte may have been updated by the time this loop finishes but I don't think it matters, it wouldn't be any different than reading a single pte and returning it while it is being updated. If you can make this easier to parse (in a few years time) with an additional patch adding some more comments, that would be great. For this patch: Reviewed-by: Catalin Marinas -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel