Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Oct 2022 15:27:18 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     John Ogness <john.ogness@linutronix.de>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@fb.com,
        rostedt@goodmis.org, tglx@linutronix.de, pmladek@suse.com
Subject: Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API
Message-ID: <20221020222718.GA5600@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220921144620.GA1200846@paulmck-ThinkPad-P17-Gen-1>
 <20220929180714.GA2874192@paulmck-ThinkPad-P17-Gen-1>
 <87k04x4e0r.fsf@jogness.linutronix.de>
 <20221018152418.GR5600@paulmck-ThinkPad-P17-Gen-1>
 <87ilkh0y52.fsf@jogness.linutronix.de>
 <20221018185936.GX5600@paulmck-ThinkPad-P17-Gen-1>
 <20221018215721.GA1716567@paulmck-ThinkPad-P17-Gen-1>
 <87pmeoawwe.fsf@jogness.linutronix.de>
 <20221019191418.GF5600@paulmck-ThinkPad-P17-Gen-1>
 <20221019220537.GA1234896@lothringen>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221019220537.GA1234896@lothringen>
Precedence: bulk

On Thu, Oct 20, 2022 at 12:05:37AM +0200, Frederic Weisbecker wrote:
> On Wed, Oct 19, 2022 at 12:14:18PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 19, 2022 at 01:19:53PM +0206, John Ogness wrote:
> > > On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > And the v6.1-rc1 stack is now at srcunmisafe.2022.10.18b.
> > > 
> > > Thanks!
> > > 
> > > I guess the kernel test robot will catch this, but if you checkout
> > > commit 79c95dc428ad ("arch/x86: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
> > > Kconfig option") and build for x86_64, you will get:
> > > 
> > > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_gp_start_if_needed':
> > > srcutree.c:(.text+0x133a): undefined reference to `__srcu_read_lock_nmisafe'
> > > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1490): undefined reference to `__srcu_read_unlock_nmisafe'
> > > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_barrier':
> > > srcutree.c:(.text+0x1b03): undefined reference to `__srcu_read_lock_nmisafe'
> > > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1b38): undefined reference to `__srcu_read_unlock_nmisafe'
> > > 
> > > Note that this error is fixed with a later commit:
> > > 
> > > commit c2d158a284ab ("srcu: Debug NMI safety even on archs that don't
> > > require it").
> > > 
> > > This does not affect what I am working on, so feel free to take care of
> > > it whenever it fits your schedule.
> > 
> > Good catch, thank you!
> > 
> > It looks like the first two hunks in include/linux/srcu.h from that
> > later commit need to be in that earlier commit.
> > 
> > Frederic, does this make sense, or am I off in the weeds?
> 
> Actually you need to do that earlier, in
> 6584822b1be1 ("srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()")
> 
> This way you don't only fix x86 bisectability but also the one of all the other safe archs.
> 
> And it's not just the first two hunks, you also need to include
> the removal of the srcutiny.h/srcutree.h definitions.
> 
> So namely you need to apply the following to 6584822b1be1. You might
> meet some minor retro-conflicts (the chknmisafe parameter didn't exist yet),
> but that's pretty much it:

Thank you both!

I have an untested but allegedly fixed branch on -rcu on branch
srcunmisafe.2022.10.20a.

							Thanx, Paul

> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 565f60d57484..f0814ffca34b 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -52,8 +52,6 @@ int init_srcu_struct(struct srcu_struct *ssp);
>  #else
>  /* Dummy definition for things like notifiers.  Actual use gets link error. */
>  struct srcu_struct { };
> -int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
> -void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
>  #endif
>  
>  void call_srcu(struct srcu_struct *ssp, struct rcu_head *head,
> @@ -66,6 +64,20 @@ unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
>  unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
>  bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
>  
> +#ifdef CONFIG_NEED_SRCU_NMI_SAFE
> +int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
> +void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
> +#else
> +static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
> +{
> +	return __srcu_read_lock(ssp);
> +}
> +static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> +{
> +	__srcu_read_unlock(ssp, idx);
> +}
> +#endif /* CONFIG_NEED_SRCU_NMI_SAFE */
> +
>  #ifdef CONFIG_SRCU
>  void srcu_init(void);
>  #else /* #ifdef CONFIG_SRCU */
> diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> index f890301f123d..f3a4d65b91ef 100644
> --- a/include/linux/srcutiny.h
> +++ b/include/linux/srcutiny.h
> @@ -89,16 +89,4 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
>  		 data_race(READ_ONCE(ssp->srcu_idx)),
>  		 data_race(READ_ONCE(ssp->srcu_idx_max)));
>  }
> -
> -static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
> -{
> -	BUG();
> -	return 0;
> -}
> -
> -static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
> -{
> -	BUG();
> -}
> -
>  #endif
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index 35ffdedf86cc..c689a81752c9 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -159,7 +155,4 @@ void synchronize_srcu_expedited(struct srcu_struct *ssp);
>  void srcu_barrier(struct srcu_struct *ssp);
>  void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf);
>  
> -int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
> -void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
> -
>  #endif
> 
>