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 04709C48260 for ; Fri, 16 Feb 2024 12:53:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5B68D6B00AA; Fri, 16 Feb 2024 07:53:52 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5656C6B00AB; Fri, 16 Feb 2024 07:53:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 406AB6B00AC; Fri, 16 Feb 2024 07:53:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 29E686B00AA for ; Fri, 16 Feb 2024 07:53:52 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id B6ED51205FD for ; Fri, 16 Feb 2024 12:53:51 +0000 (UTC) X-FDA: 81797659062.06.6596F1C Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf20.hostedemail.com (Postfix) with ESMTP id BBCDD1C000D for ; Fri, 16 Feb 2024 12:53:49 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=none; spf=pass (imf20.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708088030; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=BJXdNuzo8P7/zeszA33GbZT3/aKVRR8dUrhRaauc/g4=; b=qvIcDTdVUyxRXfjl9jYSbMInk3Xn5rZpMSe8LxdYs8MCDOahYpLUW9ShSN3vb3DSYJiUnz XgIhh16uOBYtZI0gdMWFhrIjfPfjJXwDY89JKuoT3kSH+HCod9kCuEi9wyfBGOqIz7rU7L vWoE8Vsxbdb2G5SnkjAd7GvhXgcpicc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708088030; a=rsa-sha256; cv=none; b=UU5yBmQqNq9KgeJvxzw6AoO305jWJhqhxeNmF9Lvx+k44O3quLjkD9xLdfaCbH00NSChGv ulnZe6PiGSHTeLJonMoDfJHnOP+M49i43t6g9V1o0I5zuFPAldejJuo971iCwjZPTIE3Ls J4ncs2gR1bysw6fssLVg+y7jH43i4KA= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=none; spf=pass (imf20.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=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 55182DA7; Fri, 16 Feb 2024 04:54:29 -0800 (PST) Received: from [192.168.68.110] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5AEF53F766; Fri, 16 Feb 2024 04:53:44 -0800 (PST) Message-ID: <892caa6a-e4fe-4009-aa33-0570526961c5@arm.com> Date: Fri, 16 Feb 2024 12:53:43 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 12/18] arm64/mm: Wire up PTE_CONT for user mappings Content-Language: en-GB To: Catalin Marinas 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 References: <20240215103205.2607016-1-ryan.roberts@arm.com> <20240215103205.2607016-13-ryan.roberts@arm.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: BBCDD1C000D X-Rspam-User: X-Stat-Signature: ttt68umh8a458y8tyup97pbnuao1gowe X-Rspamd-Server: rspam03 X-HE-Tag: 1708088029-894816 X-HE-Meta: U2FsdGVkX1+FaCZhDRblFGI3+CdYsojvoHvrRiTv57gNX/Zx7UONP6w3FcIQAiM0tdA6eE428n/aoQ1MrutPAXddlj5YaMn3uy7CE/u16wlCFdFnwXtxRf83LI9djCAcNp/KsnfHT4XV7b5kQkKZF4N46Gnvn6FfUtWet+2xdpTkiewyP/zL56UUq+DPq7rWyH8guOfg5/IeEmcrhCUfMHwtQXYc7aYVdxmFOJSut2gmZBV0cdyKf7gQrksIQI0Ho5ThUI1u3pzZdF0GpnOb/a1tmLDljIRwVM2sU95Zr5Q3Bh9mAHS4KWvdaJ3aD3CBGPmN6lqBrc1hinmOpjQKnirA3Ye+CqrWjH7Z+F8tJg+RODKdIp4+SV4Pir/iEVovjKFypAjFdNmwfFRgAxlvwT7LXHYjgLAPMammwoYxJZQffMM2bIVAWP8pxtG/Vm+M2ukmRrX22t3AmSw+Bc3hMmmZNbgMW7DxMvSOGoGDXnAtQHFX589NVTBPk8TIlqnB+8upwbB2lLfgE+qjJx9D6Kpe2XgJvaX0PYRfSEI9pCB7Nd5g9BKArO2JSTIHLe8VsS2rrz4ajQMgWxupZ1foyp8x1oKs5S7al6GAkd2IKrgRvvHootITsM+tBGBfYWIpG8BFjKDaU52sFr1hM3hMJ9rUEJ1Iwnywz8D1Vr4LS01cg558xonjh+sqfXB5gQa+xz26/GGqFlx4Eaefjt12QwLoAfUXOXFzx3YLXl4jX5YCdccBQSVmyhzAP9lZdKjSptOUrZraAqw6beaKbpsGwfi46g56fvNL1NAkUA4xHUWC9M0s+DFa29NFOK2QGpxlxdlJpWG5H8Ic9yxJQNzQR2lXUqpvX0ej+edWOCDhbcnF4qQeoM75ks+FpXiYIkdAH6e548jjPhEo7QaTtt/Z7wkpp6I2tRxjOA2z2i/h6wrLi9eVsZeWU+OfiijTenBdylUuiytvMPo1CSheRl2 UzHIa3WG rsZk5c02Z95UqivQok8ndOg/YGc35Dha8hzMMcoP/7tmalSpgDCaPRXAVVhMn3dZpa6m+t5J1GygUXEMFE+R1NIzzh413H1FPBumH6hv+bRrEUBPoMVIupry7DWClTWFJOwutjKdmvt+CwZ9GqJIXFgBuVvK5aheUDfOVOi/gRuqypZ2EbEjgHDR6iKvBuOMXV7Vwr+5J+QyaXRm273ZK44K5J4ezSwCfawqTYYcezH/vVv4TwPL3cMGtfi/NBW+HG4cNwz/vNujIlnaU48YDnqZNhV6H6rcegFCuXGY4giyJ4w9m2Z4cTTQuaIwdgZYACQkI6du+Mx0bse1ZrFVZF3sQUUBCBYjAu/FuIQFExtyF0EIuv9WGFh06948WnhX3K+su 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: Hi Catalin, Thanks for the review! Comments below... On 16/02/2024 12:25, Catalin Marinas wrote: > 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. The problem is that the contpte_* symbols are called from the ptep_* inline functions. So where those inlines are called from modules, we need to make sure the contpte_* symbols are available. John Hubbard originally reported this problem against v1 and I enumerated all the drivers that call into the ptep_* inlines here: https://lore.kernel.org/linux-arm-kernel/b994ff89-1a1f-26ca-9479-b08c77f94be8@arm.com/#t So they definitely need to be exported. Perhaps we can tighten it to EXPORT_SYMBOL_GPL(), but I was being cautious as I didn't want to break anything out-of-tree. I'm not sure what the normal policy is? arm64 seems to use ~equal amounts of both. > >> +#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. Correct. The pte can be updated at any time, before after or during the reads. That was always the case. But now we have to cope with a whole contpte block being repainted while we are reading it. So we are just checking to make sure that all the ptes that we read from the contpte block are consistent with eachother and therefore we can trust that the access/dirty bits we gathered are consistent. > > 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: I already have a big block comment at the top, which was trying to explain it. Clearly not well enough though. I'll add more comments as a follow up patch when I get back from holiday. > > Reviewed-by: Catalin Marinas Thanks! >