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 B9115C35FEA for ; Tue, 17 Sep 2024 07:14:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=ZOaOz4PXh6CKmoIij62EGMcw8JZ80tLFHlbr+TJamQs=; b=Q5ziJx25yCTjCoAJFI04vm8FtV kHz81ajvoAmN+N8SQk4VBxIx7//uzcueVCR80IhvKOiwo+nEdAF5WWQVrtwN7JGxz/qTjtK415sBj e5FI7NTE/Q/QtuvQRePCaRmKbADTwPoSOhYhHdz7uBVTdWhhBPDnltRef7m2Jb9clRCVht/Q4tSdx NnLMNV3qBx4MmGRzq+kJ/Jx8vJQjkYpJkUp8RYhH/78szo4bv1qFR5CzWBQgeE77QgPg0ix/r0mVd 34xk0oB2adTsRC8q7g7WfDKsav58VJ+iSuWp0pme7S+u97s5hJ2zZ6Z7ZfTL1/cOIo38dqOqk06Wx gVDLw2RQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1sqSPT-00000005Z5R-1ccW; Tue, 17 Sep 2024 07:14:03 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1sqSON-00000005Yqa-1iQs for linux-arm-kernel@lists.infradead.org; Tue, 17 Sep 2024 07:12:56 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id D7F365C0F76; Tue, 17 Sep 2024 07:12:48 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 264AAC4CEC6; Tue, 17 Sep 2024 07:12:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1726557172; bh=hgacHKFfwOa5El2HeDetZQZ0AkxJ6nRNh6t/Xie2Pm4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rbt1g37NEqrGSbdmS1XbUHYbqBqU8BT+ANrGbMK5ko2XPfVxK/AbAsiTK4WCHnX8I slITtFPqjcN3zcvVWRJOU5TY17zyHnNsXcsacXSJB3J7Q5yGt+Xnt9wK6QzAUyTLmS XDt+VDn/DIur7Uh5jwt4Sz/Zg+6/6ZvL3HezmTBfgyc0GEIupf1nr7SvTV7CrWDJWt sOOdSj13f4xKEkb4hYDqMhx8QNDFMNPVM+PvhAUDkXXJILKsxYT6sK9YEldGNQ8blX mjGH9gNVk/2uuFF+sd9UWKqw+oDH2ix6OOK8KhBybNaT5vr6HSYELs5obuU85NCz+m a70eJoZIVHogg== Date: Tue, 17 Sep 2024 08:12:46 +0100 From: Will Deacon To: cl@gentwo.org Cc: Thomas Gleixner , Catalin Marinas , Peter Zijlstra , Ingo Molnar , Waiman Long , Boqun Feng , Linus Torvalds , linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org Subject: Re: [PATCH v3] Avoid memory barrier in read_seqcount() through load acquire Message-ID: <20240917071246.GA27290@willie-the-truck> References: <20240912-seq_optimize-v3-1-8ee25e04dffa@gentwo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240912-seq_optimize-v3-1-8ee25e04dffa@gentwo.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240917_001255_609581_3C34D15B X-CRM114-Status: GOOD ( 19.33 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Christoph, On Thu, Sep 12, 2024 at 03:44:08PM -0700, Christoph Lameter via B4 Relay wrote: > diff --git a/arch/Kconfig b/arch/Kconfig > index 975dd22a2dbd..3c270f496231 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -1600,6 +1600,14 @@ config ARCH_HAS_KERNEL_FPU_SUPPORT > Architectures that select this option can run floating-point code in > the kernel, as described in Documentation/core-api/floating-point.rst. > > +config ARCH_HAS_ACQUIRE_RELEASE > + bool > + help > + Setting ARCH_HAS_ACQUIRE_RELEASE indicates that the architecture > + supports load acquire and release. Typically these are more effective > + than memory barriers. Code will prefer the use of load acquire and > + store release over memory barriers if this option is enabled. > + Unsurprisingly, I'd be in favour of making this unconditional rather than adding a new Kconfig option. Would that actually hurt any architectures where we care about the last few shreds of performance? > source "kernel/gcov/Kconfig" > > source "scripts/gcc-plugins/Kconfig" > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index a2f8ff354ca6..19e34fff145f 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -39,6 +39,7 @@ config ARM64 > select ARCH_HAS_PTE_DEVMAP > select ARCH_HAS_PTE_SPECIAL > select ARCH_HAS_HW_PTE_YOUNG > + select ARCH_HAS_ACQUIRE_RELEASE > select ARCH_HAS_SETUP_DMA_OPS > select ARCH_HAS_SET_DIRECT_MAP > select ARCH_HAS_SET_MEMORY > diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h > index d90d8ee29d81..a3fe9ee8edef 100644 > --- a/include/linux/seqlock.h > +++ b/include/linux/seqlock.h > @@ -23,6 +23,13 @@ > > #include > > +#ifdef CONFIG_ARCH_HAS_ACQUIRE_RELEASE > +# define USE_LOAD_ACQUIRE true > +# define USE_COND_LOAD_ACQUIRE !IS_ENABLED(CONFIG_PREEMPT_RT) > +#else > +# define USE_LOAD_ACQUIRE false > +# define USE_COND_LOAD_ACQUIRE false > +#endif > /* > * The seqlock seqcount_t interface does not prescribe a precise sequence of > * read begin/retry/end. For readers, typically there is a call to > @@ -132,6 +139,17 @@ static inline void seqcount_lockdep_reader_access(const seqcount_t *s) > #define seqcount_rwlock_init(s, lock) seqcount_LOCKNAME_init(s, lock, rwlock) > #define seqcount_mutex_init(s, lock) seqcount_LOCKNAME_init(s, lock, mutex) > > +static __always_inline unsigned __seqprop_load_sequence(const seqcount_t *s, bool acquire) > +{ > + if (!acquire || !USE_LOAD_ACQUIRE) > + return READ_ONCE(s->sequence); > + > + if (USE_COND_LOAD_ACQUIRE) > + return smp_cond_load_acquire((unsigned int *)&s->sequence, (s->sequence & 1) == 0); This looks wrong to me. The conditional expression passed to smp_cond_load_acquire() should be written in terms of 'VAL', otherwise you're introducing an additional non-atomic access to the sequence counter. Will