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 155A3C48260 for ; Fri, 16 Feb 2024 12:25:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 992668D0007; Fri, 16 Feb 2024 07:25:50 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 941C28D0006; Fri, 16 Feb 2024 07:25:50 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8089E8D0007; Fri, 16 Feb 2024 07:25:50 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 6DCFA8D0006 for ; Fri, 16 Feb 2024 07:25:50 -0500 (EST) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 30FCD4013B for ; Fri, 16 Feb 2024 12:25:50 +0000 (UTC) X-FDA: 81797588460.20.4B3EFB6 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf10.hostedemail.com (Postfix) with ESMTP id 8198EC001D for ; Fri, 16 Feb 2024 12:25:48 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=none; spf=pass (imf10.hostedemail.com: domain of cmarinas@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cmarinas@kernel.org; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none) ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708086348; 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: in-reply-to:in-reply-to:references:references; bh=N1j6xa/DEYDK9Xa48dRWHFL+VGsg2PnWGyeXNci4jq8=; b=HdveGwL1eZhwHLwuE8c+lCo0NO8qKrToPAsBVrZkMCPUtLtz6emLltdSkAoLM6nl9aQHCH XPj6a5ERgjm6XrbXbcqxxku+JKBOtVvFnK8mcOZyFCj9KYRo+gI0tr+gDJqg3xebc7yH1R 8/5pHPhEfCFaYmb02y0Wn2gsNQZEoC8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708086348; a=rsa-sha256; cv=none; b=jeBtgnPZp3lAbcDObK3nJZFLuJ3nbAlH+OO354KyGsrMxmPcty4NUQki3gwQ54OF9HbDpG ITe7Zc5NsXSaAWuh+8H+PM4BsDSjqY7CG1VHI3R2KsfIMNomZee2a6HHWOfHwA6RSYGEv5 fMV2j0JqCJOgFgUWUaMIJ8BlemQVdBM= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=none; spf=pass (imf10.hostedemail.com: domain of cmarinas@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cmarinas@kernel.org; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none) 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-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240215103205.2607016-13-ryan.roberts@arm.com> X-Rspamd-Queue-Id: 8198EC001D X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: m3arbdeooxb6xsbwzwoaiu15z3ypj659 X-HE-Tag: 1708086348-971919 X-HE-Meta: U2FsdGVkX1/El58I19zrWptolYK0gSpMUizFoPtYfq5iv6SmBNSEwQl2l19ZRMPfkpf8PMJhjzVZGvDKT01WJhguWPQua/zwiE4lTj5MbJ7SvMuR+K7aGIoXDjCDw+cE9Y141oH9Vo5IlpjkftgeSCp1YYaXCHLLveez0dSQvfnLT+OG8aV8BgzzP5rMOQALeDdVOj1/soz/Wcpzq18FM9s4B1zFETtldHmfHafDJ4KwwZ+TLQKG8dKHCAjJ4WiAXnANtYb9c/DOtbptib6BORzkxUkLtQKA+Lb5xMWJGliY0ndr3qlCRDTjxeBUZ92DxRtYS5Ypm3kd8RcEMf76eJwjogWnM+jI78W6UB4BH06KbI1sRMvaKmo3IUp9DGZ2XRJQjHwpQ5C8mKl47KSiYF9Pm7fPJy4B2IokYuKBWrJVs6Kim6e2otpQUZCebMwE62XGKkM9gcz3FeSXNhQFc8zElyH+apiJPUOeTYtwn6Sk68l9gIjgbGPpORz0bRYstWs0gK/RFb2b1fDMT1gAakso8l7LNJbWTwvC3zVzK3vrzKcvan4iXQxfZYixQIjfwW3v5eyJFgh01J76Z+/oAQ0xMBLRXAsGBX0F870mFY5X+YjOs1haJXKfyXGCCDybuTO/tannefQdaoVpwjVbVjPZuA5tuvzfmqOO1dVN+J9pvE3szztO8FWsFnrGOR5P+DrECQ6YF6ldjVuuRAsBJ4+69prmj4XzpetviSoukL6Sbf70i4tunmJZkif86RPipukJ1knp2JrB5PAiVtuoPw283PePL6ZSMwOU/lernfOGBFAFUc8Ef1ojkUeQ66Q7OPdgfk8F1sNtk1GGsRMjP49cvOfb/v0dsaRuaZILomepkIs3IJ+rHpdOpMu1nUXFXlTGayX+JQgj+yqrrljdVvqg8NLih1hVStePn8AAUrozHcVIxMtcgk9pxT8qMtOXySLrqniqIBqsWOyUz+u A36j7F/A Pnvle55dqkgoYrSZLWDliv3YTS67naPm5R0obPynE5PV18UgDBG2m6KMwA8SkhQCS6JuAryWwJHd6MF1DUPCH7xeOG9P5EAlHM/zlyHdg8x8k7NT1P9XuVvutZa7+XYPeYRd3k6CMaf2NmeWFFyxTOHupz3FXCbaX4vSdqFbNajFJkT5449hj3l4TD8y6E00df4DGfU8HMYapsEDsTEv3dzFwT/E8XTHWEJEcxK3+xE/DaHSJZGOdhutKucCP8Dh0aXmspZl6vnZ95sZJeS6Y/ss/Q0GPyx6ba3eKd18d0GtGvgbZNojC66nNwbHW1/2Xkglrc/zW5cAt9tlWRMV3Lze+Zu+P4PGCKaUvhfHMh2nczLptS1z9FOIOb/J0ynNjlAQmBAJgKtY4bHNlXA4UFwKbjOHRm/MRLFbVsSsJ8HesORvDusnq7NqB9ezpowcuBJKDi+umYvhp8RKbSN3FDLsSX5tyEKnlFxiaSQ+upGtchUE= 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: 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