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 7A115EE49B7 for ; Wed, 11 Sep 2024 14:32:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9CDAA940043; Wed, 11 Sep 2024 10:32:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 97DF2940021; Wed, 11 Sep 2024 10:32:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 86C37940043; Wed, 11 Sep 2024 10:32:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 6A418940021 for ; Wed, 11 Sep 2024 10:32:18 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 2E9DF160F2A for ; Wed, 11 Sep 2024 14:32:18 +0000 (UTC) X-FDA: 82552697556.10.C72A480 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf24.hostedemail.com (Postfix) with ESMTP id 5D6E7180004 for ; Wed, 11 Sep 2024 14:32:15 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=none; spf=pass (imf24.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-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1726065082; a=rsa-sha256; cv=none; b=Ixx/GvhWdybS3b2vZo3lBTjx+4uv1dHsi9Ap/1z8uMfBjrXfRMpdMbnHAmmeazDDhuQ3Df 6NS5gZMOqQiyruJFgqFRWbDzpBgDPsRozoch4zbBuxriEANGvPQv9/o6i1s6S8t04NYlla GiDCFUzPY2Nf74FrhXGQPiej9zdO2n8= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=none; spf=pass (imf24.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=1726065082; 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=tABT4HBD/4GOzShNUsS5r4WNMjStiC88eOVOGvtmkR4=; b=dI144gTlulwsY28a55+W6wh8KsysmKYfeDInA/basNazMOqYoJjw7BFKrb1atWFT58uQTv 034APYwiIl/sWm64O/S56Pp33Dfg4vCNwFCAyMlzedKbESK3gN48cOXVVVliz2nWOtDq/3 TPuTs3xctTeR9utcjhbpXsoRbO/LmdM= 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 A5D611007; Wed, 11 Sep 2024 07:32:43 -0700 (PDT) Received: from [10.1.30.171] (XHFQ2J9959.cambridge.arm.com [10.1.30.171]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3E41F3F73B; Wed, 11 Sep 2024 07:32:13 -0700 (PDT) Message-ID: <80d64b74-ae09-4737-8fed-bdc328da71a5@arm.com> Date: Wed, 11 Sep 2024 15:32:11 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] mm: Drop unused set_pte_safe() Content-Language: en-GB To: Matthew Wilcox Cc: Anshuman Khandual , linux-mm@kvack.org, Andrew Morton , David Hildenbrand , "Mike Rapoport (IBM)" , linux-kernel@vger.kernel.org References: <20240910090409.374424-1-anshuman.khandual@arm.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Stat-Signature: 19kf877we5fquzhkr6a1ogq4d3xdhwam X-Rspamd-Queue-Id: 5D6E7180004 X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1726065135-807993 X-HE-Meta: U2FsdGVkX1+pDzE5z4j8TabRmBQz8PbMqWPKI9D65FZoeo14wlOzgKyyMzszPfnsICKbcqHiY0IY2lHGhCbyqI1xth1VZlHbr7HerjcX1p/1c3kdallEQnerjbiM0/l3PcpoV8dDefXCnlN9VqqbD60ZnX4acYag1/88nEaXzirrm+1WKsTTkVZmhSNLMPyiHpijjXACOLrJZ+bKD30/NYlbHixWWi3YUUAfJTt8re5707DJE9gFUB0CSnFXS8tBDhJbZ/fDuxkufFePHC5JKnlfOMGuQOAOhUAF/aFuMYHbRkImqmbQotcvh90iWTIq9q/kUMxc5cGizJR6jyNnlOmjV/FwzbfMb3tbILpBB3a9oW9OoU/Wjx4DvltOC1sv5sk/zRG1pLbziZS+IunFL9APyXMrpvazRQ9S7li/GgTQ96jRzeuail7r20SZW22e3yxmNxtMWK2XArtKrSXCqG+3cCpV+R4RjcBHK7QUrXuF692tq9yFegUOaO6prPy/BFMsvr0My9Itgu8SOMP0V1PX030sQn6sGnFlfiMtKgMXqxvWERdlh2P9J0bLx9hD5CUeyU+hJ2sj9SqPK7XPwL8F/iOIKMM5VrIsSKZvVtxqiSr796sHwB3US08CYsyHYTNA6OzHs4TRb6blcV7iqUy2ZzSd/ApzE6kkmCuuJrv2iKizPiZiwOugTOpISTQ9XFAm615s9SdKCwUWA+R2OStBEdwztS5j757D/u7GH+04+4Rzwdz1Ae6bPnacGo3LmPR3FWw3MvsMJGQNNqZt+2B/jKK8DnRKNI8CklYNgcThLLEi0lp+/KNe7lYZFPxjFPRYFGDcrAhb3tFxeKUv74NcAysL588uLOBxUtkoBYcGNMdG8te8PtHwcDzSD8E3sWKu7wDDGMeuX3U/9UDa8pB+p1gOdF/hAzssMeDFoInfzZrI+b4eY2iujBYKNCKhETozs+V26JV9qEWWAi7 DoTCaARt MRRghur811AEsBmQulii80W1f/afv2EnY0ceCelmHz56BK1brQotfNtdY4cRJUE/Nk/bC2RLlk/XaU3FiPY7Z4cSYYRt6ci0LRAgGZAL7xLWw5f6axPovs9wGTwXtckBVLdfUKLg+qdPibz34sCFKSyNffkx0+8ZI6JPybvaRqkeQCKxio2utzd78S1pdSU1cBGLA5Ib71PoxwC756wWbjlaNJl5P+jEZxCMP+gf7Z5cNCbP+QNQaTIDL/YAWr1wfhs+Pg0c36Thbm/JZfRXXq3v5EY3ppw0diZaY5CoUGWvA4ouKYxzYPLPvPirng1WMPz4UeNuPMwgCpu+bVKKJSBJs6Itv8GaBkkC66uajZSHam/aOgLKrqQuafCOdmmQ4DlUe5CCGRuCIvcCGYO+SYFEWMbxli9RVd5Sp 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 10/09/2024 18:20, Matthew Wilcox wrote: > On Tue, Sep 10, 2024 at 10:08:10AM +0100, Ryan Roberts wrote: >> On 10/09/2024 10:04, Anshuman Khandual wrote: >>> All set_pte_safe() usage have been dropped after the commit eccd906484d1 >>> ("x86/mm: Do not use set_{pud, pmd}_safe() when splitting a large page") >>> This just drops now unused helper set_pte_safe(). >> >> It would be good to add some comment here to mention that the macro was buggy >> due to doing direct dereferencing of the pte, and that if it were to be kept, it >> should have been updated to use a single call to ptep_get(). > > I'm not sure that the _macro_ would have been buggy in such a scenario. > If I understand correctly, the _caller_ would have been buggy: > > /* > * Use set_p*_safe(), and elide TLB flushing, when confident that *no* > * TLB flush will be required as a result of the "set". For example, use > * in scenarios where it is known ahead of time that the routine is > * setting non-present entries, or re-setting an existing entry to the > * same value. Otherwise, use the typical "set" helpers and flush the > * TLB. > */ > > so if *ptep changes between the two calls, that's the caller's bug, > right? I was referring to the fact that the ptep pointer is being directly dereferenced by the macro. It should really be using ptep_get(). See commit c33c794828f21 ("mm: ptep_get() conversion") for the explanation. So it really depends on your definition of a bug; arm64 doesn't use this helper so its not affected by the magic that arm64 does in its ptep_get(). So you'll never see incorrect behavior as a result (and I suspect in this specific case, even if arm64 did use it, you probably wouldn't notice a problem in practice). Perhaps it would have been better described as a code smell. And yes, it's my bug/code smell; I didn't catch this when doing the conversion; I guess Coccinelle has no type information so didn't highlight it, and I only used the compiler trick for arm64, which doesn't use this macro. > > Otherwise, set_pmd_safe() would be buggy ... arm64 doesn't do any magic in pmdp_get() like it does for ptep_get() (yet ;-) ). So direct dereference of a pmdp pointer is a less hard requirement. But there is still the issue of READ_ONCE() vs *pmdp. As I understand it, technically these should all be READ_ONCE() to ensure single-copy atomicity, as explained in commit 20a004e7b017c ("arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables"). Although, probably in practice it doesn't really break anything here since the PTL is held so the only racing writer is the page table walker writing access and dirty bits, which shouldn't get in the way of the tests done here. Thanks, Ryan > >> With that: >> >> Reviewed-by: Ryan Roberts >> >> Thanks, >> Ryan >> >>> >>> Cc: Andrew Morton >>> Cc: David Hildenbrand >>> Cc: Ryan Roberts >>> Cc: "Mike Rapoport (IBM)" >>> Cc: linux-mm@kvack.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Anshuman Khandual >>> --- >>> include/linux/pgtable.h | 6 ------ >>> 1 file changed, 6 deletions(-) >>> >>> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h >>> index 2a6a3cccfc36..aeabbf0db7c8 100644 >>> --- a/include/linux/pgtable.h >>> +++ b/include/linux/pgtable.h >>> @@ -1058,12 +1058,6 @@ static inline int pgd_same(pgd_t pgd_a, pgd_t pgd_b) >>> * same value. Otherwise, use the typical "set" helpers and flush the >>> * TLB. >>> */ >>> -#define set_pte_safe(ptep, pte) \ >>> -({ \ >>> - WARN_ON_ONCE(pte_present(*ptep) && !pte_same(*ptep, pte)); \ >>> - set_pte(ptep, pte); \ >>> -}) >>> - >>> #define set_pmd_safe(pmdp, pmd) \ >>> ({ \ >>> WARN_ON_ONCE(pmd_present(*pmdp) && !pmd_same(*pmdp, pmd)); \ >> >>