From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 80EF935948; Thu, 19 Dec 2024 18:31:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734633081; cv=none; b=chldrT35acE6TyXM9FfGoEPkp3WcaeJ1tLADK0aMNSXTIlEXcZ4+XzZ8g9qNBmCIv/K/kHsSe2atVEAID9DVYiEAEsQ9OmsJ3rkbwEsXc27CDQ/cgp6LLgXvO/nDyYqzdEOtZY1GONVvnbxuMGiiSaRDbl6zQhsaFQBwcRv7Z2Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734633081; c=relaxed/simple; bh=LupmFiqnew+PoWEdgjS5fCZXDq669vUXqEwg87c19BU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=sG8xyjLqurMAjgP+YtlOxHDEIFqVoQd3ExD6cIk+EUlfEqMw7Bk8YzlqdgCxL/0H+vjxjnPIHJggnWqEwt8eVzd9CxjDgbEhFFxKLxxSxGPIPJf8Hj1qR+BjFV3qon2jlZLWb3xZvWKhf/Aa48Ruax9j+hiC1PHpHDOTGmn2JJg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=JUcO1lFm; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="JUcO1lFm" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 69575C4CECE; Thu, 19 Dec 2024 18:31:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734633081; bh=LupmFiqnew+PoWEdgjS5fCZXDq669vUXqEwg87c19BU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JUcO1lFmPxq16F6j8CagAPSYhkIocjIo+6iHFj4b4x5ZOg0Z/YQ2K7OgLRLkzi6xr Fx25qQEEZUbsEFeJzVAV0pJmUZAGsZqLOnh2M29c6AWxQAsezW53ZUO/Do1JiGXuPa toDmYRpXSJuoO6KknjF4BxPX8OeA3TAdv6I5JfHn/wI6CkUBVJoOZba+LCT/BNEKAn D2V5SdDFQ8o34aw2LaHOiILhuZ2hm3gvlRq0UVbLbxYEJbZulhCaqcPTyXj42inqvb CYQbD4QrQEXJcSg0M6ZWutdI4BY9Ig8d7vKKVk+ub7r3eUUz7jl9V70LFJiGOmCF4r l+ulCG3suBgCw== Date: Thu, 19 Dec 2024 18:31:15 +0000 From: Will Deacon To: "Paul E. McKenney" Cc: Peter Zijlstra , Daniel Xu , mingo@redhat.com, longman@redhat.com, boqun.feng@gmail.com, linux-kernel@vger.kernel.org, linux-toolchains@vger.kernel.org Subject: Re: [PATCH] seqlock: Use WRITE_ONCE() when updating sequence Message-ID: <20241219183115.GA25652@willie-the-truck> References: <20241218103000.GK11133@noisy.programming.kicks-ass.net> <20241218162325.GH2354@noisy.programming.kicks-ass.net> <20241218162934.GJ12500@noisy.programming.kicks-ass.net> <9b6d585a-c17f-4dd1-9518-b28ac2dfd855@paulmck-laptop> <20241218171241.GN2354@noisy.programming.kicks-ass.net> <20241219163454.GE24724@willie-the-truck> <7f7ac0e2-8103-426e-aade-5a37246e8a98@paulmck-laptop> <20241219175802.GB25477@willie-the-truck> <3c748145-5184-4e98-a1f2-079f85cf12b8@paulmck-laptop> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3c748145-5184-4e98-a1f2-079f85cf12b8@paulmck-laptop> User-Agent: Mutt/1.10.1 (2018-07-13) On Thu, Dec 19, 2024 at 10:06:23AM -0800, Paul E. McKenney wrote: > On Thu, Dec 19, 2024 at 05:58:03PM +0000, Will Deacon wrote: > > On Thu, Dec 19, 2024 at 09:53:48AM -0800, Paul E. McKenney wrote: > > > On Thu, Dec 19, 2024 at 04:34:55PM +0000, Will Deacon wrote: > > > > On Wed, Dec 18, 2024 at 06:12:41PM +0100, Peter Zijlstra wrote: > > > > > diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c > > > > > index 39cb8d0ae348..8fb7cd75fe62 100644 > > > > > --- a/arch/s390/kernel/idle.c > > > > > +++ b/arch/s390/kernel/idle.c > > > > > @@ -45,7 +45,7 @@ void account_idle_time_irq(void) > > > > > > > > > > /* Account time spent with enabled wait psw loaded as idle time. */ > > > > > WRITE_ONCE(idle->idle_time, READ_ONCE(idle->idle_time) + idle_time); > > > > > - WRITE_ONCE(idle->idle_count, READ_ONCE(idle->idle_count) + 1); > > > > > + INC_ONC(idle->idle_count); > > > > > > > > (nit: drive-by typo) > > > > > > Heh! I was clearly building with GCC. ;-) > > > > And not for S390 :p > > True enough! ;-) > > > > > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > > > > > index c9b58188ec61..77c253e29758 100644 > > > > > --- a/include/linux/compiler-gcc.h > > > > > +++ b/include/linux/compiler-gcc.h > > > > > @@ -137,3 +137,8 @@ > > > > > #if GCC_VERSION < 90100 > > > > > #undef __alloc_size__ > > > > > #endif > > > > > + > > > > > +/* > > > > > + * GCC can't properly optimize the real one with volatile on. > > > > > + */ > > > > > +#define INC_ONCE(v) (v)++ > > > > > > > > I think I'm missing what we're trying to do here, but how is this a > > > > safe fallback? I'd have thought we'd need the whole READ_ONCE() + > > > > WRITE_ONCE() sequence if the compiler doesn't give us what we need... > > > > > > > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > > > > > index efd43df3a99a..b1b13dac1b9e 100644 > > > > > --- a/include/linux/compiler.h > > > > > +++ b/include/linux/compiler.h > > > > > @@ -8,6 +8,10 @@ > > > > > > > > > > #ifdef __KERNEL__ > > > > > > > > > > +#ifndef INC_ONCE > > > > > +#define INC_ONCE(v) (*(volatile typeof(v) *)&(v))++ > > > > > +#endif > > > > > > > > ... but I'm honestly not sure what this is aiming to elide. Is this about > > > > having the compiler emit an increment instruction with memory operands > > > > on architectures that support it? If so, what about architectures that > > > > _don't_ support that? We still need to guarantee that the load/store > > > > instructions are single-copy atomic for the type in that case. > > > > > > Architectures whose increment instructions lack memory operands can still > > > load, increment, then store. So yes, if two of these run concurrently, > > > you can lose counts when incrementing normal memory. (Of course, with > > > device memory, you get what you get.) > > > > ... but can we guarantee that those load and store instructions won't > > now be torn? > > The volatile type qualifier must take care of that, or device drivers > no longer work. > > Or are you worried about the C-language ++ fallback? In that case, > as I understand it, this fallback is only there until GCC learns to > generate a increment-to-memory instruction for x86. As I understand it, both the clang and the GCC implementations in the proposed diff are written using the C-language ++ operator, no? The GCC one doesn't have 'volatile' at all: #define INC_ONCE(v) (v)++ so I don't understand why it's a correct replacement for e.g.: WRITE_ONCE(idle->idle_time, READ_ONCE(idle->idle_time) + idle_time); If it was, then we wouldn't need any of these macros in the first place! The clang version does have 'volatile': #define INC_ONCE(v) (*(volatile typeof(v) *)&(v))++ but then I don't understand why this is enough for the compiler to generate an 'inc' with memory operands on x86 if it cannot do that for the READ_ONCE + WRITE_ONCE implementation. If somehow the above is more "optimisable" then I worry about the atomicity being preserved as well. So, basically, I don't understand what's going on here :) I think I was expecting to see arch-specific implementations of INC_ONCE() written in assembly where they have the relevant instructions but that doesn't seem to be the case. Will