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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0CAA5C4332F for ; Mon, 3 Oct 2022 13:32:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229795AbiJCNcR (ORCPT ); Mon, 3 Oct 2022 09:32:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43660 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229523AbiJCNcO (ORCPT ); Mon, 3 Oct 2022 09:32:14 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 467CA2B1A5; Mon, 3 Oct 2022 06:32:13 -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 E6037B81110; Mon, 3 Oct 2022 13:32:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6D48CC433C1; Mon, 3 Oct 2022 13:32:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1664803930; bh=asOE+hzrVNCgdrI5kF2gbU4H889vs9ZPQScrahLaA0k=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=B0MKI91QDpSNZg9ZJQCGEB/tSOdojVhY+Mipv8QDeNlHzYd34hBSk1D32mIh4FAWe dRSSNLBoONrbyhxt9rfwxsLugDrj0wC40pb/msIIuiLSCYqiXxUXfga0YKdoJsLKXr VZ/GP6S8rw3zNbK6IYGGzQ5yyGgfysd32PF2wDGs6EkFrcpoFWYmB+niITHBCm6kdw 6QpaWp4ZoYN1o0bkSmiN4BFgsfHL2PDFabDcU7SY7t4WPBdBkihaps0SrZeSRRiuz2 klzOQekpanGFpewJ2e/5QjuSs12VEr1ZuZofxTQTXbFByXs0GQni3BGiK99j92P5h5 vgTjxLBRzf1kw== Received: by paulmck-ThinkPad-P17-Gen-1.home (Postfix, from userid 1000) id 0C6DF5C0641; Mon, 3 Oct 2022 06:32:10 -0700 (PDT) Date: Mon, 3 Oct 2022 06:32:10 -0700 From: "Paul E. McKenney" To: Frederic Weisbecker Cc: rcu@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com, rostedt@goodmis.org, Thomas Gleixner , John Ogness , Petr Mladek Subject: Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety Message-ID: <20221003133210.GZ4196@paulmck-ThinkPad-P17-Gen-1> Reply-To: paulmck@kernel.org References: <20220929180714.GA2874192@paulmck-ThinkPad-P17-Gen-1> <20220929180731.2875722-3-paulmck@kernel.org> <20221002220619.GA298433@lothringen> <20221002235103.GW4196@paulmck-ThinkPad-P17-Gen-1> <20221003101331.GA304186@lothringen> <20221003115718.GY4196@paulmck-ThinkPad-P17-Gen-1> <20221003123721.GA304426@lothringen> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221003123721.GA304426@lothringen> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 03, 2022 at 02:37:21PM +0200, Frederic Weisbecker wrote: > On Mon, Oct 03, 2022 at 04:57:18AM -0700, Paul E. McKenney wrote: > > On Mon, Oct 03, 2022 at 12:13:31PM +0200, Frederic Weisbecker wrote: > > > On Sun, Oct 02, 2022 at 04:51:03PM -0700, Paul E. McKenney wrote: > > > > On Mon, Oct 03, 2022 at 12:06:19AM +0200, Frederic Weisbecker wrote: > > > > > On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote: > > > > > > This commit adds runtime checks to verify that a given srcu_struct uses > > > > > > consistent NMI-safe (or not) read-side primitives on a per-CPU basis. > > > > > > > > > > > > Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/ > > > > > > > > > > > > Signed-off-by: Paul E. McKenney > > > > > > Cc: Thomas Gleixner > > > > > > Cc: John Ogness > > > > > > Cc: Petr Mladek > > > > > > --- > > > > > > include/linux/srcu.h | 4 ++-- > > > > > > include/linux/srcutiny.h | 4 ++-- > > > > > > include/linux/srcutree.h | 9 +++++++-- > > > > > > kernel/rcu/srcutree.c | 38 ++++++++++++++++++++++++++++++++------ > > > > > > 4 files changed, 43 insertions(+), 12 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > > > > > index 2cc8321c0c86..565f60d57484 100644 > > > > > > --- a/include/linux/srcu.h > > > > > > +++ b/include/linux/srcu.h > > > > > > @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp > > > > > > int retval; > > > > > > > > > > > > if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE)) > > > > > > - retval = __srcu_read_lock_nmisafe(ssp); > > > > > > + retval = __srcu_read_lock_nmisafe(ssp, true); > > > > > > else > > > > > > retval = __srcu_read_lock(ssp); > > > > > > > > > > Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ? > > > > > > > > You are asking why there is no "true" argument to __srcu_read_lock()? > > > > That is because it checks unconditionally. > > > > > > It checks unconditionally but it always assumes not to be called as nmisafe. > > > > > > For example on x86/arm64/loongarch, the same ssp used with both srcu_read_lock() and > > > srcu_read_lock_nmisafe() won't report an issue. But on powerpc it will. > > > > > > My point is that strong archs should warn as well on behalf of others, to detect > > > mistakes early. > > > > Good point, especially given that x86_64 and arm64 are a rather large > > fraction of the uses. Not critically urgent, but definitely nice to have. > > No indeed. > > > > > Did you by chance have a suggestion for a nice way to accomplish this? > > This could be like this: > > enum srcu_nmi_flags { > SRCU_NMI_UNKNOWN = 0x0, > SRCU_NMI_UNSAFE = 0x1, > SRCU_NMI_SAFE = 0x2 > }; > > #ifdef CONFIG_NEED_SRCU_NMI_SAFE > static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags) > { > int idx; > struct srcu_data *sdp = raw_cpu_ptr(ssp->sda); > > idx = READ_ONCE(ssp->srcu_idx) & 0x1; > atomic_long_inc(&sdp->srcu_lock_count[idx]); > smp_mb__after_atomic(); /* B */ /* Avoid leaking the critical section. */ > > srcu_check_nmi_safety(ssp, flags); > > return idx; > } > #else > static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags) > { > srcu_check_nmi_safety(ssp, flags); > return __srcu_read_lock(ssp); > } > #endif > > static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) > { > return __srcu_read_lock_nmisafe(ssp, SRCU_NMI_SAFE); > } > > // An __srcu_read_lock() caller in kernel/rcu/tasks.h must be > // taken care of as well > static inline int srcu_read_lock(struct srcu_struct *ssp) > { > srcu_check_nmi_safety(ssp, SRCU_NMI_UNSAFE); > return __srcu_read_lock(ssp); > } > > And then you can call __srcu_read_lock_nmisafe(ssp, SRCU_NMI_UNKNOWN) from > initializers of gp. Not bad at all! Would you like to send a patch? I do not consider this to be something for the current merge window even if the rest goes in because printk() is used heavily and because it is easy to get access to powerpc and presumably also riscv systems. But as you say, it would be very good to have longer term for the case where srcu_read_lock_nmisafe() is used for some more obscure purpose. Thanx, Paul