Re: [rcu:rcu/next 35/35] kernel/rcu/tree.c:251:8: error: implicit declaration of function 'arch_atomic_add_return'; did you mean

[Marco Elver]

On Wed, 24 Jun 2020 at 22:30, Paul E. McKenney <paulmck@xxxxxxxxxx> 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 <lkp@xxxxxxxxx>
> >
> > 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