On Wed, 24 Jun 2020 at 22:30, Paul E. McKenney <[email protected]> wrote: > > On Thu, Jun 25, 2020 at 03:38:03AM +0800, kernel test robot wrote: > > tree: > > https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git > > rcu/next > > head: 347acb93a34a6e4f312f8b9ec1afdb86d27858d2 > > commit: 347acb93a34a6e4f312f8b9ec1afdb86d27858d2 [35/35] rcu: Fixup noinstr > > warnings > > config: mips-allyesconfig (attached as .config) > > compiler: mips-linux-gcc (GCC) 9.3.0 > > reproduce (this is a W=1 build): > > wget > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > > ~/bin/make.cross > > chmod +x ~/bin/make.cross > > git checkout 347acb93a34a6e4f312f8b9ec1afdb86d27858d2 > > # save the attached .config to linux build tree > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross > > ARCH=mips > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot <[email protected]> > > > > All errors (new ones prefixed by >>): > > > > kernel/rcu/tree.c: In function 'rcu_dynticks_eqs_enter': > > >> kernel/rcu/tree.c:251:8: error: implicit declaration of function > > >> 'arch_atomic_add_return'; did you mean 'atomic_add_return'? > > >> [-Werror=implicit-function-declaration] > > 251 | seq = arch_atomic_add_return(RCU_DYNTICK_CTRL_CTR, > > &rdp->dynticks); > > | ^~~~~~~~~~~~~~~~~~~~~~ > > | atomic_add_return > > kernel/rcu/tree.c: In function 'rcu_dynticks_eqs_exit': > > >> kernel/rcu/tree.c:281:3: error: implicit declaration of function > > >> 'arch_atomic_andnot'; did you mean 'atomic_andnot'? > > >> [-Werror=implicit-function-declaration] > > 281 | arch_atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks); > > | ^~~~~~~~~~~~~~~~~~ > > | atomic_andnot > > kernel/rcu/tree.c: In function 'rcu_dynticks_curr_cpu_in_eqs': > > >> kernel/rcu/tree.c:314:11: error: implicit declaration of function > > >> 'arch_atomic_read'; did you mean 'atomic_read'? > > >> [-Werror=implicit-function-declaration] > > 314 | return !(arch_atomic_read(&rdp->dynticks) & > > RCU_DYNTICK_CTRL_CTR); > > | ^~~~~~~~~~~~~~~~ > > | atomic_read > > cc1: some warnings being treated as errors > > And architectures using the definitions in include/linux/atomic-fallback.h > don't like this patch much. MIPS defines everything in terms of > atomic_add_return_relaxed(), for which it provides inline assembly for > SMP-capable builds and a C-language code sequence otherwise. > > One way of handling this is as follows: > > ------------------------------------------------------------------------ > > diff --git a/include/linux/atomic-fallback.h b/include/linux/atomic-fallback.h > index 2c4927b..b7935857 100644 > --- a/include/linux/atomic-fallback.h > +++ b/include/linux/atomic-fallback.h > @@ -133,6 +133,7 @@ atomic_add_return(int i, atomic_t *v) > return ret; > } > #define atomic_add_return atomic_add_return > +#define arch_atomic_add_return atomic_add_return > #endif > > #endif /* atomic_add_return_relaxed */ > > ------------------------------------------------------------------------ > > And of course similar for arch_atomic_andnot() and arch_atomic_read(). > > Another way would be to define a noinstr_atomic_add_return() that > was defined something like this: > > ------------------------------------------------------------------------ > > #ifdef CONFIG_HAVE_ARCH_KCSAN > # define noinstr_atomic_add_return arch_atomic_add_return > #else > # define noinstr_atomic_add_return atomic_add_return > #endif
noinstr also needs to apply to KASAN & co, so this won't quite work. Every architecture that defines arch_atomic_* has #define ARCH_ATOMIC, so that could be used instead. > ------------------------------------------------------------------------ > > And again similarly for the others. > > Left to myself, I would take the second option just because it provably > leaves unaltered anything that isn't using the new API. That said, > there has to be a better Kconfig option to key this off of. > > Thoughts? I think 'arch_atomic_*' is already the noinstr variant, and your first suggestion of adding arch-defines to atomic-fallback.h seems cleaner, as it avoids introducing new interfaces. But that also depends on if it's a one-off, only for RCU, or if the use of 'arch_atomic' proliferates outside of arch/. My guess is that, unfortunately, other places will want 'arch_atomic' as well eventually. Thanks, -- Marco
