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 847F7C433F5 for ; Tue, 22 Mar 2022 21:07:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=i7sMr5cBrRknUSFhji8OouVowjEyh6K670rHGSF8dTI=; b=gaKRa/rIn/8MzW CPrBCDBhqfpR87BriDWFphDRQbu21o+SAt//prmKPVgNK602IhsTtXDgW77rYghC+91ZRe9KJ3gYZ f2sMXUFBSZPqvYBIBVsuKaRR9neQbvdXNfuoopdK9UiU0A74zzBAYFm7+in/jq03reknnpsGvxMZV vX3qCJG+tVE2YnRwyer31VTtUxaR8IVqWAIX8WBq+4z8UrQeJd+XDErMtyqGzDF96us1o/mb2m3ic 2/YqlRUPiEWHXGQK5+rVPOWctgDMFh3CIYsd5fl/BNUj5ZEFX4/6mmQfXU/IBO5PpQ9/LI4+vxehF LAHabylSon9Udp6UMixw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWliV-00CAgF-Q1; Tue, 22 Mar 2022 21:06:59 +0000 Received: from mail-pl1-x636.google.com ([2607:f8b0:4864:20::636]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWliS-00CAfT-Qh for linux-riscv@lists.infradead.org; Tue, 22 Mar 2022 21:06:58 +0000 Received: by mail-pl1-x636.google.com with SMTP id i11so15849725plr.1 for ; Tue, 22 Mar 2022 14:06:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=VqKe7hI7FJvdE8Xfvk2XUbpj3DCkNeOgE/n2lG3V58M=; b=l6Lj5tunNDJYVXRRFlmjLK9y2r5Jtsm6eTf4WyR4vCW9d1v3e8JWl3ivVczi5PMKsp GIR1Sg6+0/bKw/9nBJUQcybRUIvkvFjdJJ0H0oR7fHFEyu+XupEKYEm3OQQKjDn7iyj4 bNbaob5ScTg/vx318Dqxax8e6cOPbpYL7m1deZjm6slVSnoWf5Ur5VGgVNHRCj/wYRXl FZkBtOUZMTDLIAjt6SX0LD19WvVYwc6XpjnWcEAjC12kt/wlb9ILb3PgmRJw5Vi2J0YA n2oNcjFY7X3/mjlom/Y0MchhJrHJt4RxsRJWIcPs/y3pdKU0A9kr1SA7zWjclEkJd+OK 17sw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=VqKe7hI7FJvdE8Xfvk2XUbpj3DCkNeOgE/n2lG3V58M=; b=pR6Dc8AAvf3yik4TFMQrtzHtNxO4mLe4exnSpWUfCLtThbkI2hhHDiwMNuayCBMWAi /YbJI4FHLUIK67sVx6h2Z6DNBXfI0JYkC6PTA9Y18a7rtoB5Nw9gRaCFpp1JHrk6dJXy oPgzeEpbhdKIqe5MRj3EyOqC2e2Cd+jsWZFIizP6NGimaHhOwvtM9j5lHFwi5wDh5VlQ bhXxAjjPMYsEtAyzvKZF/H+X29ueSKw9DpL0gpkLdYMlJW/gSVLJgS4M1Zr7llxkn1oI bzAaphBZU1YTMJ5WEEtqPauoUjvoNReUgnlJ8ERwI4K1qsVzvlRvd8iT9cmY9Qnkzcgk 8BDw== X-Gm-Message-State: AOAM530CZAYTMLqhvGsDHCF42pVT7R5pKyXmUz+5ubqoOi03WB8N8ezs UimOFHc26uUDpoMoZTyK2PU= X-Google-Smtp-Source: ABdhPJyu21bjBOSUqHlZN0IdeZqf053AvJ2Yx8yrrk0KLxSnLOPWMTq9b2/meIZSuiNdlmy9jXWyOg== X-Received: by 2002:a17:902:d643:b0:153:97c3:c8e5 with SMTP id y3-20020a170902d64300b0015397c3c8e5mr20078217plh.76.1647983215902; Tue, 22 Mar 2022 14:06:55 -0700 (PDT) Received: from localhost ([2409:10:24a0:4700:e8ad:216a:2a9d:6d0c]) by smtp.gmail.com with ESMTPSA id k12-20020a056a00134c00b004fabc8fa42esm3345559pfu.87.2022.03.22.14.06.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Mar 2022 14:06:54 -0700 (PDT) Date: Wed, 23 Mar 2022 06:06:52 +0900 From: Stafford Horne To: Waiman Long Cc: guoren@kernel.org, palmer@dabbelt.com, arnd@arndb.de, boqun.feng@gmail.com, peterz@infradead.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, linux-csky@vger.kernel.org, openrisc@lists.librecores.org, Palmer Dabbelt , linux-riscv@lists.infradead.org Subject: Re: [OpenRISC] [PATCH V2 1/5] asm-generic: ticket-lock: New generic ticket-based spinlock Message-ID: References: <20220319035457.2214979-1-guoren@kernel.org> <20220319035457.2214979-2-guoren@kernel.org> <54d6221d-0c4f-9329-042d-4f74c4ea288f@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <54d6221d-0c4f-9329-042d-4f74c4ea288f@redhat.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220322_140656_903449_E816EB35 X-CRM114-Status: GOOD ( 32.92 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Tue, Mar 22, 2022 at 11:54:37AM -0400, Waiman Long wrote: > On 3/21/22 23:10, Stafford Horne wrote: > > Hello, > > > > There is a problem with this patch on Big Endian machines, see below. > > > > On Sat, Mar 19, 2022 at 11:54:53AM +0800, guoren@kernel.org wrote: > > > From: Peter Zijlstra > > > > > > This is a simple, fair spinlock. Specifically it doesn't have all the > > > subtle memory model dependencies that qspinlock has, which makes it more > > > suitable for simple systems as it is more likely to be correct. > > > > > > [Palmer: commit text] > > > Signed-off-by: Palmer Dabbelt > > > > > > -- > > > > > > I have specifically not included Peter's SOB on this, as he sent his > > > original patch > > > > > > without one. > > > --- > > > include/asm-generic/spinlock.h | 11 +++- > > > include/asm-generic/spinlock_types.h | 15 +++++ > > > include/asm-generic/ticket-lock-types.h | 11 ++++ > > > include/asm-generic/ticket-lock.h | 86 +++++++++++++++++++++++++ > > > 4 files changed, 120 insertions(+), 3 deletions(-) > > > create mode 100644 include/asm-generic/spinlock_types.h > > > create mode 100644 include/asm-generic/ticket-lock-types.h > > > create mode 100644 include/asm-generic/ticket-lock.h > > > > > > diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h > > > new file mode 100644 > > > index 000000000000..59373de3e32a > > > --- /dev/null > > > +++ b/include/asm-generic/ticket-lock.h > > ... > > > > > +static __always_inline void ticket_unlock(arch_spinlock_t *lock) > > > +{ > > > + u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN); > > As mentioned, this patch series breaks SMP on OpenRISC. I traced it to this > > line. The above `__is_defined(__BIG_ENDIAN)` does not return 1 as expected > > even on BIG_ENDIAN machines. This works: > > > > > > diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h > > index 59373de3e32a..52b5dc9ffdba 100644 > > --- a/include/asm-generic/ticket-lock.h > > +++ b/include/asm-generic/ticket-lock.h > > @@ -26,6 +26,7 @@ > > #define __ASM_GENERIC_TICKET_LOCK_H > > #include > > +#include > > #include > > static __always_inline void ticket_lock(arch_spinlock_t *lock) > > @@ -51,7 +52,7 @@ static __always_inline bool ticket_trylock(arch_spinlock_t *lock) > > static __always_inline void ticket_unlock(arch_spinlock_t *lock) > > { > > - u16 *ptr = (u16 *)lock + __is_defined(__BIG_ENDIAN); > > + u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN); > > u32 val = atomic_read(lock); > > smp_store_release(ptr, (u16)val + 1); > > > > > > > + u32 val = atomic_read(lock); > > > + > > > + smp_store_release(ptr, (u16)val + 1); > > > +} > > > + > > __BIG_ENDIAN is defined in . I believe that if you include > , the second hunk is not really needed and vice versa. I thought so too, but it doesn't seem to work. I think __is_defined is not doing what we think in this context. It looks like __is_defined works when a macro is defined as 1, in this case we have __BIG_ENDIAN 4321. With just the first hunk, we can see we still get 0 for the lock offset as per below: diff --git a/include/asm-generic/ticket-lock.h b/include/asm-generic/ticket-lock.h index 59373de3e32a..769561fb6997 100644 --- a/include/asm-generic/ticket-lock.h +++ b/include/asm-generic/ticket-lock.h @@ -26,6 +26,7 @@ #define __ASM_GENERIC_TICKET_LOCK_H #include +#include #include static __always_inline void ticket_lock(arch_spinlock_t *lock) -- make ARCH=openrisc simple_smp_defconfig make ARCH=openrisc CROSS_COMPILE=or1k-linux- kernel/locking/spinlock.i grep -C3 'lock +' kernel/locking/spinlock.i static inline __attribute__((__gnu_inline__)) __attribute__((__unused__)) __attribute__((__no_instrument_function__)) __attribute__((__always_inline__)) void ticket_unlock(arch_spinlock_t *lock) { u16 *ptr = (u16 *)lock + 0; u32 val = atomic_read(lock); -Stafford _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv