Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <202006250300.ic32FsdY%lkp@intel.com> <20200624203025.GJ9247@paulmck-ThinkPad-P72>
In-Reply-To: <20200624203025.GJ9247@paulmck-ThinkPad-P72>
From:   Marco Elver <elver@google.com>
Date:   Thu, 25 Jun 2020 11:55:13 +0200
Message-ID: <CANpmjNO5uBSZj0gHy0t+O2VhD+UjG58+zON0AFX8i7MNSO5a6Q@mail.gmail.com>
Subject: Re: [rcu:rcu/next 35/35] kernel/rcu/tree.c:251:8: error: implicit
 declaration of function 'arch_atomic_add_return'; did you mean
To:     "Paul E. McKenney" <paulmck@kernel.org>
Cc:     kernel test robot <lkp@intel.com>,
        Peter Zijlstra <peterz@infradead.org>, kbuild-all@lists.01.org,
        LKML <linux-kernel@vger.kernel.org>,
        Will Deacon <will@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 24 Jun 2020 at 22:30, Paul E. McKenney <paulmck@kernel.org> 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@intel.com>
> >
> > 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