Received: by 2002:ac2:464d:0:0:0:0:0 with SMTP id s13csp3274771lfo; Mon, 23 May 2022 00:27:36 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzrk1FbqWYCdBN9whMxnSJ291gXJSAfQHAdxxDS3pNqzI2QYe2Sr0GWc9I9UwksBNeni1Wi X-Received: by 2002:a17:902:c952:b0:162:14b6:3179 with SMTP id i18-20020a170902c95200b0016214b63179mr7261407pla.7.1653290856378; Mon, 23 May 2022 00:27:36 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1653290856; cv=none; d=google.com; s=arc-20160816; b=0UxC+LUsQfv1gz7VgXO6niXxCnXZ0dFNb0WJ3kfTE80aELO0zM76ZtapzRGkyoNxbd dlDsU20/derdKfPuKpR0Z9B2usz9yVO8lPsSXIb6fAg96kHBo8Uj8HW2mmSiWW+WUXIX +TWqDt99KIfMr0tPDlKUmovV40Unqi9QRvf+gBMbrWcOs5965udo0NeILCc014R0Tf76 yI/3I70XWJkWtjKQT+FftgeUI+Y9xuDer/KHi+9O1Ayv5qz8h9sTNwbn90i6YCQ9JtzW 0R2pndjudh7nT82LtRb+MdgfymtPmxtjbDhEmyyjmeitsiD1kOGtz6iR6803M/2Ww1DF 6ifQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=v7P3OdRr7CwwPDd5xVMOtizC8wrS4KMX6TkDN70MltQ=; b=PpLqe/P83bRHELbrm1lH43s35BgSk9ntkMoLJbQg8redHVUglD/m0R+iMBSd8P7VZT sjtyetvsLWFz109fQwwkx9d+gG/0CgdDE50QJjeRc3nuxvRlJaVi5wBCvOLd4EcSY77t vZiOKmw2TET5smf6fBaSysazDFwiSakltYa4USFrOlnlyh6LZ+JgrgDf+bj3I1b+vsqS btdrqo0X5gfnbMWUvvQb0SvJDlEutw8sXZfOwlWUPAP+YE/l+OiJIJ2hbsjaFWMztTyy VTguRpvc9yZRtwWeBoFa+DvhNlO8JmWoojlX8YbK4IFvA2JX+hxxqhtvlrQhp/5xzKmS M2Rw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=karEZOrV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net. [2620:137:e000::1:18]) by mx.google.com with ESMTPS id t12-20020a17090340cc00b00156c0b0a42asi8546618pld.113.2022.05.23.00.27.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 May 2022 00:27:36 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=karEZOrV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 0A4CF4B1FB; Sun, 22 May 2022 23:40:50 -0700 (PDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233796AbiEVNNQ (ORCPT + 99 others); Sun, 22 May 2022 09:13:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46184 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229937AbiEVNNO (ORCPT ); Sun, 22 May 2022 09:13:14 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3780839B84; Sun, 22 May 2022 06:13:12 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id A063DB80BE9; Sun, 22 May 2022 13:13:10 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3BC29C385AA; Sun, 22 May 2022 13:13:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1653225189; bh=8Ni5c6Xwwig839tx+SLYNI2uAV2C3Tn7rzb0ubCC4rA=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=karEZOrVgKgEytAenz7Bf/6RrIZZPB+4IcNTvWZy1mdVVrGHIuuJwmPXjhUJ7C4wm qUaq+Gd1g9DH7Kh7I5ZCmlt5NY9NbZYd/GZXLt37EQxGjM8wfkk9ud1ppizlvDfJ05 RkXq6vAPZYas2zOWziYSXJbX9y9mduKjbmfG/9JOz00HMAhbw8uk+hajuElu/gOvng OPYbQUtBWNW7d/EBZ/JebhCIsHUYQl19awBnkje0hgNzjg2hKEeD7zODUOHlC9TAm5 nDaHDLaRlMZJYZBFaHn4JWTx0nypXKAtkkcnkoDV7EAkRB2zyGqgvoaHYLqTDMfOCi /L3dYo5GXzk3g== Received: by mail-vs1-f53.google.com with SMTP id h4so1481196vsr.13; Sun, 22 May 2022 06:13:09 -0700 (PDT) X-Gm-Message-State: AOAM5337BM7pH879Dyhsw/cMK3wRswbKQy/uiksj3if0OG25rl+hARIe /n5eC4B6KdW6vSxbUIBjzQrCU9Z9yPFP5L/WH1I= X-Received: by 2002:a67:e046:0:b0:335:d1a2:6192 with SMTP id n6-20020a67e046000000b00335d1a26192mr7142077vsl.8.1653225188112; Sun, 22 May 2022 06:13:08 -0700 (PDT) MIME-Version: 1.0 References: <20220505035526.2974382-6-guoren@kernel.org> In-Reply-To: From: Guo Ren Date: Sun, 22 May 2022 21:12:56 +0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH V4 5/5] riscv: atomic: Optimize LRSC-pairs atomic ops with .aqrl annotation To: Palmer Dabbelt Cc: Andrea Parri , Arnd Bergmann , Mark Rutland , Will Deacon , Peter Zijlstra , Boqun Feng , Daniel Lustig , linux-arch , Linux Kernel Mailing List , linux-riscv , Guo Ren Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, May 22, 2022 at 4:46 AM Palmer Dabbelt wrote: > > On Wed, 04 May 2022 20:55:26 PDT (-0700), guoren@kernel.org wrote: > > From: Guo Ren > > > > The current implementation is the same with 8e86f0b409a4 > > ("arm64: atomics: fix use of acquire + release for full barrier > > semantics"). RISC-V could combine acquire and release into the SC > > instructions and it could reduce a fence instruction to gain better > > performance. Here is related descriptio from RISC-V ISA 10.2 > > Load-Reserved/Store-Conditional Instructions: > > > > - .aq: The LR/SC sequence can be given acquire semantics by > > setting the aq bit on the LR instruction. > > - .rl: The LR/SC sequence can be given release semantics by > > setting the rl bit on the SC instruction. > > - .aqrl: Setting the aq bit on the LR instruction, and setting > > both the aq and the rl bit on the SC instruction makes > > the LR/SC sequence sequentially consistent, meaning that > > it cannot be reordered with earlier or later memory > > operations from the same hart. > > > > Software should not set the rl bit on an LR instruction unless > > the aq bit is also set, nor should software set the aq bit on an > > SC instruction unless the rl bit is also set. LR.rl and SC.aq > > instructions are not guaranteed to provide any stronger ordering > > than those with both bits clear, but may result in lower > > performance. > > > > The only difference is when sc.w/d.aqrl failed, it would cause .aq > > effect than before. But it's okay for sematic because overlap > > address LR couldn't beyond relating SC. > > IIUC that's not accurate, or at least wasn't in 2018. The ISA tends to > drift around a bit, so it's possible things have changed since then. > 5ce6c1f3535f ("riscv/atomic: Strengthen implementations with fences") Thx for explaining, that why I suspected with the sentence in the comment: "which do not give full-ordering with .aqrl" > describes the issue more specifically, that's when we added these > fences. There have certainly been complains that these fences are too > heavyweight for the HW to go fast, but IIUC it's the best option we have Yeah, it would reduce the performance on D1 and our next-generation processor has optimized fence performance a lot. > given the current set of memory model primitives we implement in the > ISA (ie, there's more in RVWMO but no way to encode that). > > The others all look good, though, and as these are really all > independent cleanups I'm going to go ahead and put those three on > for-next. > > There's also a bunch of checkpatch errors. The ones about "*" seem > spurious, but the alignment ones aren't. Here's my fixups: Thx for the fixup. > > diff --git a/arch/riscv/include/asm/atomic.h b/arch/riscv/include/asm/atomic.h > index 34f757dfc8f2..0bde499fa8bc 100644 > --- a/arch/riscv/include/asm/atomic.h > +++ b/arch/riscv/include/asm/atomic.h > @@ -86,9 +86,9 @@ ATOMIC_OPS(xor, xor, i) > * versions, while the logical ops only have fetch versions. > */ > #define ATOMIC_FETCH_OP(op, asm_op, I, asm_type, c_type, prefix) \ > -static __always_inline \ > -c_type arch_atomic##prefix##_fetch_##op##_relaxed(c_type i, \ > - atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_fetch_##op##_relaxed(c_type i, \ > + atomic##prefix##_t *v) \ > { \ > register c_type ret; \ > __asm__ __volatile__ ( \ > @@ -98,9 +98,9 @@ c_type arch_atomic##prefix##_fetch_##op##_relaxed(c_type i, \ > : "memory"); \ > return ret; \ > } \ > -static __always_inline \ > -c_type arch_atomic##prefix##_fetch_##op##_acquire(c_type i, \ > - atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_fetch_##op##_acquire(c_type i, \ > + atomic##prefix##_t *v) \ > { \ > register c_type ret; \ > __asm__ __volatile__ ( \ > @@ -110,9 +110,9 @@ c_type arch_atomic##prefix##_fetch_##op##_acquire(c_type i, \ > : "memory"); \ > return ret; \ > } \ > -static __always_inline \ > -c_type arch_atomic##prefix##_fetch_##op##_release(c_type i, \ > - atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_fetch_##op##_release(c_type i, \ > + atomic##prefix##_t *v) \ > { \ > register c_type ret; \ > __asm__ __volatile__ ( \ > @@ -122,8 +122,8 @@ c_type arch_atomic##prefix##_fetch_##op##_release(c_type i, \ > : "memory"); \ > return ret; \ > } \ > -static __always_inline \ > -c_type arch_atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v) \ > { \ > register c_type ret; \ > __asm__ __volatile__ ( \ > @@ -135,28 +135,28 @@ c_type arch_atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v) \ > } > > #define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_type, c_type, prefix) \ > -static __always_inline \ > -c_type arch_atomic##prefix##_##op##_return_relaxed(c_type i, \ > - atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_##op##_return_relaxed(c_type i, \ > + atomic##prefix##_t *v) \ > { \ > - return arch_atomic##prefix##_fetch_##op##_relaxed(i, v) c_op I; \ > + return arch_atomic##prefix##_fetch_##op##_relaxed(i, v) c_op I; \ > } \ > -static __always_inline \ > -c_type arch_atomic##prefix##_##op##_return_acquire(c_type i, \ > - atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_##op##_return_acquire(c_type i, \ > + atomic##prefix##_t *v) \ > { \ > - return arch_atomic##prefix##_fetch_##op##_acquire(i, v) c_op I; \ > + return arch_atomic##prefix##_fetch_##op##_acquire(i, v) c_op I; \ > } \ > -static __always_inline \ > -c_type arch_atomic##prefix##_##op##_return_release(c_type i, \ > - atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_##op##_return_release(c_type i, \ > + atomic##prefix##_t *v) \ > { \ > - return arch_atomic##prefix##_fetch_##op##_release(i, v) c_op I; \ > + return arch_atomic##prefix##_fetch_##op##_release(i, v) c_op I; \ > } \ > -static __always_inline \ > -c_type arch_atomic##prefix##_##op##_return(c_type i, atomic##prefix##_t *v) \ > +static __always_inline c_type \ > +arch_atomic##prefix##_##op##_return(c_type i, atomic##prefix##_t *v) \ > { \ > - return arch_atomic##prefix##_fetch_##op(i, v) c_op I; \ > + return arch_atomic##prefix##_fetch_##op(i, v) c_op I; \ > } > > #ifdef CONFIG_GENERIC_ATOMIC64 > > > > > > Signed-off-by: Guo Ren > > Signed-off-by: Guo Ren > > Cc: Palmer Dabbelt > > Cc: Mark Rutland > > Cc: Dan Lustig > > Cc: Andrea Parri > > --- > > arch/riscv/include/asm/atomic.h | 24 ++++++++---------------- > > arch/riscv/include/asm/cmpxchg.h | 6 ++---- > > 2 files changed, 10 insertions(+), 20 deletions(-) > > > > diff --git a/arch/riscv/include/asm/atomic.h b/arch/riscv/include/asm/atomic.h > > index 34f757dfc8f2..aef8aa9ac4f4 100644 > > --- a/arch/riscv/include/asm/atomic.h > > +++ b/arch/riscv/include/asm/atomic.h > > @@ -269,9 +269,8 @@ static __always_inline int arch_atomic_fetch_add_unless(atomic_t *v, int a, int > > "0: lr.w %[p], %[c]\n" > > " beq %[p], %[u], 1f\n" > > " add %[rc], %[p], %[a]\n" > > - " sc.w.rl %[rc], %[rc], %[c]\n" > > + " sc.w.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : [a]"r" (a), [u]"r" (u) > > @@ -290,9 +289,8 @@ static __always_inline s64 arch_atomic64_fetch_add_unless(atomic64_t *v, s64 a, > > "0: lr.d %[p], %[c]\n" > > " beq %[p], %[u], 1f\n" > > " add %[rc], %[p], %[a]\n" > > - " sc.d.rl %[rc], %[rc], %[c]\n" > > + " sc.d.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : [a]"r" (a), [u]"r" (u) > > @@ -382,9 +380,8 @@ static __always_inline bool arch_atomic_inc_unless_negative(atomic_t *v) > > "0: lr.w %[p], %[c]\n" > > " bltz %[p], 1f\n" > > " addi %[rc], %[p], 1\n" > > - " sc.w.rl %[rc], %[rc], %[c]\n" > > + " sc.w.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : > > @@ -402,9 +399,8 @@ static __always_inline bool arch_atomic_dec_unless_positive(atomic_t *v) > > "0: lr.w %[p], %[c]\n" > > " bgtz %[p], 1f\n" > > " addi %[rc], %[p], -1\n" > > - " sc.w.rl %[rc], %[rc], %[c]\n" > > + " sc.w.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : > > @@ -422,9 +418,8 @@ static __always_inline int arch_atomic_dec_if_positive(atomic_t *v) > > "0: lr.w %[p], %[c]\n" > > " addi %[rc], %[p], -1\n" > > " bltz %[rc], 1f\n" > > - " sc.w.rl %[rc], %[rc], %[c]\n" > > + " sc.w.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : > > @@ -444,9 +439,8 @@ static __always_inline bool arch_atomic64_inc_unless_negative(atomic64_t *v) > > "0: lr.d %[p], %[c]\n" > > " bltz %[p], 1f\n" > > " addi %[rc], %[p], 1\n" > > - " sc.d.rl %[rc], %[rc], %[c]\n" > > + " sc.d.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : > > @@ -465,9 +459,8 @@ static __always_inline bool arch_atomic64_dec_unless_positive(atomic64_t *v) > > "0: lr.d %[p], %[c]\n" > > " bgtz %[p], 1f\n" > > " addi %[rc], %[p], -1\n" > > - " sc.d.rl %[rc], %[rc], %[c]\n" > > + " sc.d.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : > > @@ -486,9 +479,8 @@ static __always_inline s64 arch_atomic64_dec_if_positive(atomic64_t *v) > > "0: lr.d %[p], %[c]\n" > > " addi %[rc], %[p], -1\n" > > " bltz %[rc], 1f\n" > > - " sc.d.rl %[rc], %[rc], %[c]\n" > > + " sc.d.aqrl %[rc], %[rc], %[c]\n" > > " bnez %[rc], 0b\n" > > - " fence rw, rw\n" > > "1:\n" > > : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter) > > : > > diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h > > index 1af8db92250b..9269fceb86e0 100644 > > --- a/arch/riscv/include/asm/cmpxchg.h > > +++ b/arch/riscv/include/asm/cmpxchg.h > > @@ -307,9 +307,8 @@ > > __asm__ __volatile__ ( \ > > "0: lr.w %0, %2\n" \ > > " bne %0, %z3, 1f\n" \ > > - " sc.w.rl %1, %z4, %2\n" \ > > + " sc.w.aqrl %1, %z4, %2\n" \ > > " bnez %1, 0b\n" \ > > - " fence rw, rw\n" \ > > "1:\n" \ > > : "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr) \ > > : "rJ" ((long)__old), "rJ" (__new) \ > > @@ -319,9 +318,8 @@ > > __asm__ __volatile__ ( \ > > "0: lr.d %0, %2\n" \ > > " bne %0, %z3, 1f\n" \ > > - " sc.d.rl %1, %z4, %2\n" \ > > + " sc.d.aqrl %1, %z4, %2\n" \ > > " bnez %1, 0b\n" \ > > - " fence rw, rw\n" \ > > "1:\n" \ > > : "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr) \ > > : "rJ" (__old), "rJ" (__new) \ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/