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 740D8156F39; Wed, 18 Dec 2024 19:15:06 +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=1734549307; cv=none; b=s66FGDyAvk5NTpnH3lUfJiQwAI5+ml9WYTefqosPzAZKvEJH0klX82tiN8iXaxcpwUu9nk3QBK6W6Rs4diw/UWoo4HImYtIrUkj/Ebj8otyJHMtEfsMEUGzkTnOjVTJ6Kq6sWF78kyiUpNNOaJ/cNj6TasVWlWuC4RJvNRsQDJI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734549307; c=relaxed/simple; bh=TueU+/lcl+pUmTzI86r5LZLPC/pRIyOKFnXqvJY1vgE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BTBv/H9ov2of1MkvkBUnrqp0SVttmySzjQusVhiir3Q3e36sGbleDreJyECE8zGar50eP2YsqC3YMldqWRMnQFEfLUAGi7u+mKD8F7SUyIWh67M7ODJ+XYtSpD7cmvwL/e9gkCcdDReesbUDZUZN9dq7gTxN9TYkTdxacjF3IZ8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=mUimw5La; 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="mUimw5La" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 15FDCC4CECD; Wed, 18 Dec 2024 19:15:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734549306; bh=TueU+/lcl+pUmTzI86r5LZLPC/pRIyOKFnXqvJY1vgE=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=mUimw5LaURkPRv5Ll2K233tMz9yloN/gXZ9BmUxBpsOdq/piN/xvvYxoCcQieKlvt CvGt5UenSld+Ve7D9pRMpv+3N3jIVRpWpdV5MM4m62hvnakkkMmJ4F/lNKLzDz0hnh f1Q3cX4th8MfY1qY2nIazHHdkF/Qu41tpvKcDI0k8TsRWy5bpLCa/HgRzBeDA1Wx+K NaBmboNNaLmWjoX5d3eawPxOGauNjvKT1HHzgHnMiAFVQ4lBWnfRKCGlESD2+ZsWgK 6TAFNOf+AkCv3VpvR+cRNlQfAuSnvvN5ntC4p1730Bu/Oo7LOmMllflQT0PnFA4zFG yJSDMNp7nMw6A== Received: by paulmck-ThinkPad-P17-Gen-1.home (Postfix, from userid 1000) id ADDF7CE0C4A; Wed, 18 Dec 2024 11:15:05 -0800 (PST) Date: Wed, 18 Dec 2024 11:15:05 -0800 From: "Paul E. McKenney" To: Peter Zijlstra Cc: Daniel Xu , mingo@redhat.com, will@kernel.org, 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: <1d026056-5fa3-47e0-94cc-5212128ae00a@paulmck-laptop> Reply-To: paulmck@kernel.org References: <0eaea03ecc9df536649763cfecda356fc38b6938.1734477414.git.dxu@dxuuu.xyz> <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> 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: <20241218171241.GN2354@noisy.programming.kicks-ass.net> On Wed, Dec 18, 2024 at 06:12:41PM +0100, Peter Zijlstra wrote: > > +linux-toolchains > > On Wed, Dec 18, 2024 at 08:59:47AM -0800, Paul E. McKenney wrote: > > > > Perhaps something like: (*(volatile unsigned int *)&s->sequence)++; ? > > > I'd have to check what the compiler makes of that. > > > > > > /me mucks about with godbolt for a bit... > > > > > > GCC doesn't optimize that, but Clang does. > > > > > > I would still very much refrain from making this change until both > > > compilers can generate sane code for it. > > > > Is GCC on track to do this, or do we need to encourage them? > > I have no clue; probably wise to offer encouragement. Hopefully your +linux-toolchain is a start. > > Just to make sure I understand, your proposal is to create an INC_ONCE() > > or similar, and add it once compiler support is there? Seems reasonable > > to me, just checking. > > I suppose we can work in parallel, add INC_ONCE() now, but not have a > proper definition for GCC. This passes an rcutorture smoke test, so feel free to add: Reviewed-by: Paul E. McKenney > --- > arch/riscv/kvm/vmid.c | 2 +- > arch/s390/kernel/idle.c | 2 +- > drivers/md/dm-vdo/indexer/index-session.c | 2 +- > fs/xfs/libxfs/xfs_iext_tree.c | 2 +- > include/linux/compiler-gcc.h | 5 +++++ > include/linux/compiler.h | 4 ++++ > include/linux/rcupdate_trace.h | 2 +- > include/linux/srcutiny.h | 2 +- > io_uring/io_uring.c | 2 +- > kernel/rcu/srcutree.c | 4 ++-- > kernel/rcu/tree_plugin.h | 2 +- > kernel/sched/fair.c | 2 +- > mm/kfence/kfence_test.c | 2 +- > security/apparmor/apparmorfs.c | 2 +- > 14 files changed, 22 insertions(+), 13 deletions(-) > > diff --git a/arch/riscv/kvm/vmid.c b/arch/riscv/kvm/vmid.c > index ddc98714ce8e..805a5acf669c 100644 > --- a/arch/riscv/kvm/vmid.c > +++ b/arch/riscv/kvm/vmid.c > @@ -90,7 +90,7 @@ void kvm_riscv_gstage_vmid_update(struct kvm_vcpu *vcpu) > > /* First user of a new VMID version? */ > if (unlikely(vmid_next == 0)) { > - WRITE_ONCE(vmid_version, READ_ONCE(vmid_version) + 1); > + INC_ONCE(vmid_version); > vmid_next = 1; > > /* > 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); > account_idle_time(cputime_to_nsecs(idle_time)); > } > > diff --git a/drivers/md/dm-vdo/indexer/index-session.c b/drivers/md/dm-vdo/indexer/index-session.c > index aee0914d604a..c5a7dee9dc66 100644 > --- a/drivers/md/dm-vdo/indexer/index-session.c > +++ b/drivers/md/dm-vdo/indexer/index-session.c > @@ -152,7 +152,7 @@ static void enter_callback_stage(struct uds_request *request) > > static inline void count_once(u64 *count_ptr) > { > - WRITE_ONCE(*count_ptr, READ_ONCE(*count_ptr) + 1); > + INC_ONCE(*count_ptr); > } > > static void update_session_stats(struct uds_request *request) > diff --git a/fs/xfs/libxfs/xfs_iext_tree.c b/fs/xfs/libxfs/xfs_iext_tree.c > index 8796f2b3e534..a1fcd4cf2424 100644 > --- a/fs/xfs/libxfs/xfs_iext_tree.c > +++ b/fs/xfs/libxfs/xfs_iext_tree.c > @@ -626,7 +626,7 @@ xfs_iext_realloc_root( > */ > static inline void xfs_iext_inc_seq(struct xfs_ifork *ifp) > { > - WRITE_ONCE(ifp->if_seq, READ_ONCE(ifp->if_seq) + 1); > + INC_ONCE(ifp->if_seq); > } > > void > 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)++ > 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 > + > /* > * Note: DISABLE_BRANCH_PROFILING can be used by special lowlevel code > * to disable branch tracing on a per file basis. > diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h > index e6c44eb428ab..adb12e7304da 100644 > --- a/include/linux/rcupdate_trace.h > +++ b/include/linux/rcupdate_trace.h > @@ -50,7 +50,7 @@ static inline void rcu_read_lock_trace(void) > { > struct task_struct *t = current; > > - WRITE_ONCE(t->trc_reader_nesting, READ_ONCE(t->trc_reader_nesting) + 1); > + INC_ONCE(t->trc_reader_nesting); > barrier(); > if (IS_ENABLED(CONFIG_TASKS_TRACE_RCU_READ_MB) && > t->trc_reader_special.b.need_mb) > diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h > index 1321da803274..bd4362323733 100644 > --- a/include/linux/srcutiny.h > +++ b/include/linux/srcutiny.h > @@ -66,7 +66,7 @@ static inline int __srcu_read_lock(struct srcu_struct *ssp) > > preempt_disable(); // Needed for PREEMPT_AUTO > idx = ((READ_ONCE(ssp->srcu_idx) + 1) & 0x2) >> 1; > - WRITE_ONCE(ssp->srcu_lock_nesting[idx], READ_ONCE(ssp->srcu_lock_nesting[idx]) + 1); > + INC_ONCE(ssp->srcu_lock_nesting[idx]); > preempt_enable(); > return idx; > } > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > index 06ff41484e29..ef3d4871e775 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c > @@ -373,7 +373,7 @@ static void io_account_cq_overflow(struct io_ring_ctx *ctx) > { > struct io_rings *r = ctx->rings; > > - WRITE_ONCE(r->cq_overflow, READ_ONCE(r->cq_overflow) + 1); > + INC_ONCE(r->cq_overflow); > ctx->cq_extra--; > } > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 5e2e53464794..a812af81dbff 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -656,7 +656,7 @@ static unsigned long srcu_get_delay(struct srcu_struct *ssp) > jbase += j - gpstart; > if (!jbase) { > ASSERT_EXCLUSIVE_WRITER(sup->srcu_n_exp_nodelay); > - WRITE_ONCE(sup->srcu_n_exp_nodelay, READ_ONCE(sup->srcu_n_exp_nodelay) + 1); > + INC_ONCE(sup->srcu_n_exp_nodelay); > if (READ_ONCE(sup->srcu_n_exp_nodelay) > srcu_max_nodelay_phase) > jbase = 1; > } > @@ -1856,7 +1856,7 @@ static void process_srcu(struct work_struct *work) > j = jiffies; > if (READ_ONCE(sup->reschedule_jiffies) == j) { > ASSERT_EXCLUSIVE_WRITER(sup->reschedule_count); > - WRITE_ONCE(sup->reschedule_count, READ_ONCE(sup->reschedule_count) + 1); > + INC_ONCE(sup->reschedule_count); > if (READ_ONCE(sup->reschedule_count) > srcu_max_nodelay) > curdelay = 1; > } else { > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > index 3927ea5f7955..51f525089c27 100644 > --- a/kernel/rcu/tree_plugin.h > +++ b/kernel/rcu/tree_plugin.h > @@ -387,7 +387,7 @@ static int rcu_preempt_blocked_readers_cgp(struct rcu_node *rnp) > > static void rcu_preempt_read_enter(void) > { > - WRITE_ONCE(current->rcu_read_lock_nesting, READ_ONCE(current->rcu_read_lock_nesting) + 1); > + INC_ONCE(current->rcu_read_lock_nesting); > } > > static int rcu_preempt_read_exit(void) > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index f5329672815b..f0927407abbe 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -3242,7 +3242,7 @@ static void reset_ptenuma_scan(struct task_struct *p) > * statistical sampling. Use READ_ONCE/WRITE_ONCE, which are not > * expensive, to avoid any form of compiler optimizations: > */ > - WRITE_ONCE(p->mm->numa_scan_seq, READ_ONCE(p->mm->numa_scan_seq) + 1); > + INC_ONCE(p->mm->numa_scan_seq); > p->mm->numa_scan_offset = 0; > } > > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index f65fb182466d..e0bf31b1875e 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -517,7 +517,7 @@ static void test_kmalloc_aligned_oob_write(struct kunit *test) > * fault immediately after it. > */ > expect.addr = buf + size; > - WRITE_ONCE(*expect.addr, READ_ONCE(*expect.addr) + 1); > + INC_ONCE(*expect.addr); > KUNIT_EXPECT_FALSE(test, report_available()); > test_free(buf); > KUNIT_EXPECT_TRUE(test, report_matches(&expect)); > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 2c0185ebc900..63f5c8387764 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -596,7 +596,7 @@ static __poll_t ns_revision_poll(struct file *file, poll_table *pt) > > void __aa_bump_ns_revision(struct aa_ns *ns) > { > - WRITE_ONCE(ns->revision, READ_ONCE(ns->revision) + 1); > + INC_ONCE(ns->revision); > wake_up_interruptible(&ns->wait); > } >