Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1303377 > unrolled thread

linux-next: build failure after merge of the rcu tree

Back to article view | Back to linux.kernel

Contents

  linux-next: build failure after merge of the rcu tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-01-07 10:00 +0100
    Re: linux-next: build failure after merge of the rcu tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-07 19:30 +0100
      Re: linux-next: build failure after merge of the rcu tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-01-07 21:20 +0100
        Re: linux-next: build failure after merge of the rcu tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-07 22:00 +0100
          Re: linux-next: build failure after merge of the rcu tree Boqun Feng <boqun.feng@gmail.com> - 2016-01-08 02:40 +0100
            Re: linux-next: build failure after merge of the rcu tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-08 04:50 +0100
              Re: linux-next: build failure after merge of the rcu tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-01-08 05:10 +0100
                Re: linux-next: build failure after merge of the rcu tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-08 05:50 +0100
                  Re: linux-next: build failure after merge of the rcu tree Boqun Feng <boqun.feng@gmail.com> - 2016-01-08 06:00 +0100
                    Re: linux-next: build failure after merge of the rcu tree Tejun Heo <tj@kernel.org> - 2016-01-08 17:00 +0100
                      Re: linux-next: build failure after merge of the rcu tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-08 17:20 +0100
                    Re: linux-next: build failure after merge of the rcu tree Boqun Feng <boqun.feng@gmail.com> - 2016-01-08 17:00 +0100
                    Re: linux-next: build failure after merge of the rcu tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-08 17:00 +0100
            Re: linux-next: build failure after merge of the rcu tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-01-08 05:20 +0100

#1303377 — linux-next: build failure after merge of the rcu tree

Hi Paul,

[I found this a few days ago, but I think I forgot to send the email,
sorry.]

After merging the rcu tree, today's linux-next build (powerpc
allyesconfig) failed like this:

kernel/rcu/rcuperf.o:(.discard+0x0): multiple definition of `__pcpu_unique_srcu_ctl_srcu_array'
kernel/rcu/rcutorture.o:(.discard+0x0): first defined here

Caused by commit

  abcd7ec0808e ("rcutorture: Add RCU grace-period performance tests")

I have reverted that commit for today.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1303784

On Thu, Jan 07, 2016 at 07:57:25PM +1100, Stephen Rothwell wrote:
> Hi Paul,
> 
> [I found this a few days ago, but I think I forgot to send the email,
> sorry.]
> 
> After merging the rcu tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
> 
> kernel/rcu/rcuperf.o:(.discard+0x0): multiple definition of `__pcpu_unique_srcu_ctl_srcu_array'
> kernel/rcu/rcutorture.o:(.discard+0x0): first defined here
> 
> Caused by commit
> 
>   abcd7ec0808e ("rcutorture: Add RCU grace-period performance tests")
> 
> I have reverted that commit for today.

Hello, Stephen,

Very strange.  The "static" keyword does not mean anything here?
Easy enough to use different symbols in the two different files,
but this situation is not so good for information hiding.

Happy to update rcuperf.c to use a different name, but in the
immortal words of MSDOS, "Are you sure?" :-)

						Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1303866

Hi Paul,

On Thu, 7 Jan 2016 10:02:44 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> On Thu, Jan 07, 2016 at 07:57:25PM +1100, Stephen Rothwell wrote:
> > Hi Paul,
> > 
> > [I found this a few days ago, but I think I forgot to send the email,
> > sorry.]
> > 
> > After merging the rcu tree, today's linux-next build (powerpc
> > allyesconfig) failed like this:
> > 
> > kernel/rcu/rcuperf.o:(.discard+0x0): multiple definition of `__pcpu_unique_srcu_ctl_srcu_array'
> > kernel/rcu/rcutorture.o:(.discard+0x0): first defined here
> > 
> > Caused by commit
> > 
> >   abcd7ec0808e ("rcutorture: Add RCU grace-period performance tests")
> > 
> > I have reverted that commit for today.  
> 
> Hello, Stephen,
> 
> Very strange.  The "static" keyword does not mean anything here?
> Easy enough to use different symbols in the two different files,
> but this situation is not so good for information hiding.
> 
> Happy to update rcuperf.c to use a different name, but in the
> immortal words of MSDOS, "Are you sure?" :-)

I have no idea why it happens, but I do get the error above unless I
revert that commit.  So, yes, I am sure :-)

OK, I looked further and

DEFINE_STATIC_SRCU(srcu_ctl);

becomes this (NLs added for clarity):

static __attribute__((section(".discard"), unused)) char __pcpu_scope_srcu_ctl_srcu_array;
extern __attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
__attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
extern __attribute__((section(".data..percpu" ""))) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
__attribute__((section(".data..percpu" ""))) __attribute__((weak)) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
static struct srcu_struct srcu_ctl = {
	.
	.
};

So, the "static" is not very effective :-(

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[toc] | [prev] | [next] | [standalone]

#1303892

On Fri, Jan 08, 2016 at 07:19:32AM +1100, Stephen Rothwell wrote:
> Hi Paul,
> 
> On Thu, 7 Jan 2016 10:02:44 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >
> > On Thu, Jan 07, 2016 at 07:57:25PM +1100, Stephen Rothwell wrote:
> > > Hi Paul,
> > > 
> > > [I found this a few days ago, but I think I forgot to send the email,
> > > sorry.]
> > > 
> > > After merging the rcu tree, today's linux-next build (powerpc
> > > allyesconfig) failed like this:
> > > 
> > > kernel/rcu/rcuperf.o:(.discard+0x0): multiple definition of `__pcpu_unique_srcu_ctl_srcu_array'
> > > kernel/rcu/rcutorture.o:(.discard+0x0): first defined here
> > > 
> > > Caused by commit
> > > 
> > >   abcd7ec0808e ("rcutorture: Add RCU grace-period performance tests")
> > > 
> > > I have reverted that commit for today.  
> > 
> > Hello, Stephen,
> > 
> > Very strange.  The "static" keyword does not mean anything here?
> > Easy enough to use different symbols in the two different files,
> > but this situation is not so good for information hiding.
> > 
> > Happy to update rcuperf.c to use a different name, but in the
> > immortal words of MSDOS, "Are you sure?" :-)
> 
> I have no idea why it happens, but I do get the error above unless I
> revert that commit.  So, yes, I am sure :-)
> 
> OK, I looked further and
> 
> DEFINE_STATIC_SRCU(srcu_ctl);
> 
> becomes this (NLs added for clarity):
> 
> static __attribute__((section(".discard"), unused)) char __pcpu_scope_srcu_ctl_srcu_array;
> extern __attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
> __attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
> extern __attribute__((section(".data..percpu" ""))) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
> __attribute__((section(".data..percpu" ""))) __attribute__((weak)) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
> static struct srcu_struct srcu_ctl = {
> 	.
> 	.
> };
> 
> So, the "static" is not very effective :-(

Oddly enough, this appears to be toolchain dependent.  No idea why.

Here is a patch that I will be merging in.

							Thanx, Paul

------------------------------------------------------------------------

commit d81f900405de0dc6152692a2088258b8b35d740d
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Thu Jan 7 12:39:10 2016 -0800

    Merge with abcd7ec0808e (rcutorture: Add RCU grace-period performance tests)
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
index eef82a9460d8..4c8d99aa4f5e 100644
--- a/kernel/rcu/rcuperf.c
+++ b/kernel/rcu/rcuperf.c
@@ -188,8 +188,8 @@ static struct rcu_perf_ops rcu_bh_ops = {
  * Definitions for srcu perf testing.
  */
 
-DEFINE_STATIC_SRCU(srcu_ctl);
-static struct srcu_struct *srcu_ctlp = &srcu_ctl;
+DEFINE_STATIC_SRCU(srcu_ctl_perf);
+static struct srcu_struct *srcu_ctlp = &srcu_ctl_perf;
 
 static int srcu_perf_read_lock(void) __acquires(srcu_ctlp)
 {

[toc] | [prev] | [next] | [standalone]

#1304106

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jan 07, 2016 at 12:52:20PM -0800, Paul E. McKenney wrote:
> On Fri, Jan 08, 2016 at 07:19:32AM +1100, Stephen Rothwell wrote:
> > Hi Paul,
> > 
> > On Thu, 7 Jan 2016 10:02:44 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > On Thu, Jan 07, 2016 at 07:57:25PM +1100, Stephen Rothwell wrote:
> > > > Hi Paul,
> > > > 
> > > > [I found this a few days ago, but I think I forgot to send the email,
> > > > sorry.]
> > > > 
> > > > After merging the rcu tree, today's linux-next build (powerpc
> > > > allyesconfig) failed like this:
> > > > 
> > > > kernel/rcu/rcuperf.o:(.discard+0x0): multiple definition of `__pcpu_unique_srcu_ctl_srcu_array'
> > > > kernel/rcu/rcutorture.o:(.discard+0x0): first defined here
> > > > 
> > > > Caused by commit
> > > > 
> > > >   abcd7ec0808e ("rcutorture: Add RCU grace-period performance tests")
> > > > 
> > > > I have reverted that commit for today.  
> > > 
> > > Hello, Stephen,
> > > 
> > > Very strange.  The "static" keyword does not mean anything here?
> > > Easy enough to use different symbols in the two different files,
> > > but this situation is not so good for information hiding.
> > > 
> > > Happy to update rcuperf.c to use a different name, but in the
> > > immortal words of MSDOS, "Are you sure?" :-)
> > 
> > I have no idea why it happens, but I do get the error above unless I
> > revert that commit.  So, yes, I am sure :-)
> > 
> > OK, I looked further and
> > 
> > DEFINE_STATIC_SRCU(srcu_ctl);
> > 
> > becomes this (NLs added for clarity):
> > 
> > static __attribute__((section(".discard"), unused)) char __pcpu_scope_srcu_ctl_srcu_array;
> > extern __attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
> > __attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
> > extern __attribute__((section(".data..percpu" ""))) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
> > __attribute__((section(".data..percpu" ""))) __attribute__((weak)) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
> > static struct srcu_struct srcu_ctl = {
> > 	.
> > 	.
> > };
> > 
> > So, the "static" is not very effective :-(
> 
> Oddly enough, this appears to be toolchain dependent.  No idea why.
> 

Maybe the reason is because "static" doesn't work well with
DEFINE_PER_CPU sometimes?

The definition of __DEFINE_STATIC_SRCU is:

#define __DEFINE_SRCU(name, is_static)					\
	static DEFINE_PER_CPU(struct srcu_struct_array, name##_srcu_array);\
	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name)

whereas DEFINE_PER_CPU(which calls DEFINE_PER_CPU_SECTION) *could*
consists of *several* definitions:

#if defined(ARCH_NEEDS_WEAK_PER_CPU) || defined(CONFIG_DEBUG_FORCE_WEAK_PER_CPU)
...
#define DEFINE_PER_CPU_SECTION(type, name, sec)				\
	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
	__typeof__(type) name
#else
...
#define DEFINE_PER_CPU_SECTION(type, name, sec)				\
	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES			\
	__typeof__(type) name
#endif

So if ARCH_NEEDS_WEAK_PER_CPU=y or CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y,
the "static" keyword only has effects on the first definition i.e.
__pcpu_scope_##name.

Mind to check your config options, Stephen?


IOW, DEFINE_PER_CPU is not designed to work with "static", maybe we
should add STATIC_DEFINE_PER_CPU for that purpose?

Cc Tejun and Christoph for their opinions.

Regards,
Boqun

> Here is a patch that I will be merging in.
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit d81f900405de0dc6152692a2088258b8b35d740d
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Thu Jan 7 12:39:10 2016 -0800
> 
>     Merge with abcd7ec0808e (rcutorture: Add RCU grace-period performance tests)
>     
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
> index eef82a9460d8..4c8d99aa4f5e 100644
> --- a/kernel/rcu/rcuperf.c
> +++ b/kernel/rcu/rcuperf.c
> @@ -188,8 +188,8 @@ static struct rcu_perf_ops rcu_bh_ops = {
>   * Definitions for srcu perf testing.
>   */
>  
> -DEFINE_STATIC_SRCU(srcu_ctl);
> -static struct srcu_struct *srcu_ctlp = &srcu_ctl;
> +DEFINE_STATIC_SRCU(srcu_ctl_perf);
> +static struct srcu_struct *srcu_ctlp = &srcu_ctl_perf;
>  
>  static int srcu_perf_read_lock(void) __acquires(srcu_ctlp)
>  {
> 

[toc] | [prev] | [next] | [standalone]

#1304159

On Fri, Jan 08, 2016 at 09:37:04AM +0800, Boqun Feng wrote:
> On Thu, Jan 07, 2016 at 12:52:20PM -0800, Paul E. McKenney wrote:
> > On Fri, Jan 08, 2016 at 07:19:32AM +1100, Stephen Rothwell wrote:
> > > Hi Paul,
> > > 
> > > On Thu, 7 Jan 2016 10:02:44 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > On Thu, Jan 07, 2016 at 07:57:25PM +1100, Stephen Rothwell wrote:
> > > > > Hi Paul,
> > > > > 
> > > > > [I found this a few days ago, but I think I forgot to send the email,
> > > > > sorry.]
> > > > > 
> > > > > After merging the rcu tree, today's linux-next build (powerpc
> > > > > allyesconfig) failed like this:
> > > > > 
> > > > > kernel/rcu/rcuperf.o:(.discard+0x0): multiple definition of `__pcpu_unique_srcu_ctl_srcu_array'
> > > > > kernel/rcu/rcutorture.o:(.discard+0x0): first defined here
> > > > > 
> > > > > Caused by commit
> > > > > 
> > > > >   abcd7ec0808e ("rcutorture: Add RCU grace-period performance tests")
> > > > > 
> > > > > I have reverted that commit for today.  
> > > > 
> > > > Hello, Stephen,
> > > > 
> > > > Very strange.  The "static" keyword does not mean anything here?
> > > > Easy enough to use different symbols in the two different files,
> > > > but this situation is not so good for information hiding.
> > > > 
> > > > Happy to update rcuperf.c to use a different name, but in the
> > > > immortal words of MSDOS, "Are you sure?" :-)
> > > 
> > > I have no idea why it happens, but I do get the error above unless I
> > > revert that commit.  So, yes, I am sure :-)
> > > 
> > > OK, I looked further and
> > > 
> > > DEFINE_STATIC_SRCU(srcu_ctl);
> > > 
> > > becomes this (NLs added for clarity):
> > > 
> > > static __attribute__((section(".discard"), unused)) char __pcpu_scope_srcu_ctl_srcu_array;
> > > extern __attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
> > > __attribute__((section(".discard"), unused)) char __pcpu_unique_srcu_ctl_srcu_array;
> > > extern __attribute__((section(".data..percpu" ""))) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
> > > __attribute__((section(".data..percpu" ""))) __attribute__((weak)) __typeof__(struct srcu_struct_array) srcu_ctl_srcu_array;
> > > static struct srcu_struct srcu_ctl = {
> > > 	.
> > > 	.
> > > };
> > > 
> > > So, the "static" is not very effective :-(
> > 
> > Oddly enough, this appears to be toolchain dependent.  No idea why.
> > 
> 
> Maybe the reason is because "static" doesn't work well with
> DEFINE_PER_CPU sometimes?
> 
> The definition of __DEFINE_STATIC_SRCU is:
> 
> #define __DEFINE_SRCU(name, is_static)					\
> 	static DEFINE_PER_CPU(struct srcu_struct_array, name##_srcu_array);\
> 	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name)
> 
> whereas DEFINE_PER_CPU(which calls DEFINE_PER_CPU_SECTION) *could*
> consists of *several* definitions:
> 
> #if defined(ARCH_NEEDS_WEAK_PER_CPU) || defined(CONFIG_DEBUG_FORCE_WEAK_PER_CPU)
> ...
> #define DEFINE_PER_CPU_SECTION(type, name, sec)				\
> 	__PCPU_DUMMY_ATTRS char __pcpu_scope_##name;			\
> 	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
> 	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
> 	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
> 	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak			\
> 	__typeof__(type) name
> #else
> ...
> #define DEFINE_PER_CPU_SECTION(type, name, sec)				\
> 	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES			\
> 	__typeof__(type) name
> #endif
> 
> So if ARCH_NEEDS_WEAK_PER_CPU=y or CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y,
> the "static" keyword only has effects on the first definition i.e.
> __pcpu_scope_##name.
> 
> Mind to check your config options, Stephen?
> 
> 
> IOW, DEFINE_PER_CPU is not designed to work with "static", maybe we
> should add STATIC_DEFINE_PER_CPU for that purpose?

Indeed, I suspect that SRCU might not be the only thing that would like
static per-CPU variables.  ;-)

> Cc Tejun and Christoph for their opinions.
> 
> Regards,
> Boqun

							Thanx, Paul

> > Here is a patch that I will be merging in.
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit d81f900405de0dc6152692a2088258b8b35d740d
> > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Date:   Thu Jan 7 12:39:10 2016 -0800
> > 
> >     Merge with abcd7ec0808e (rcutorture: Add RCU grace-period performance tests)
> >     
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
> > index eef82a9460d8..4c8d99aa4f5e 100644
> > --- a/kernel/rcu/rcuperf.c
> > +++ b/kernel/rcu/rcuperf.c
> > @@ -188,8 +188,8 @@ static struct rcu_perf_ops rcu_bh_ops = {
> >   * Definitions for srcu perf testing.
> >   */
> >  
> > -DEFINE_STATIC_SRCU(srcu_ctl);
> > -static struct srcu_struct *srcu_ctlp = &srcu_ctl;
> > +DEFINE_STATIC_SRCU(srcu_ctl_perf);
> > +static struct srcu_struct *srcu_ctlp = &srcu_ctl_perf;
> >  
> >  static int srcu_perf_read_lock(void) __acquires(srcu_ctlp)
> >  {
> > 

[toc] | [prev] | [next] | [standalone]

#1304162

Hi Paul,

On Thu, 7 Jan 2016 19:41:57 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> Indeed, I suspect that SRCU might not be the only thing that would like
> static per-CPU variables.  ;-)

from include/linux/percpu-defs.h:

 * s390 and alpha modules require percpu variables to be defined as
 * weak to force the compiler to generate GOT based external
 * references for them.  This is necessary because percpu sections
 * will be located outside of the usually addressable area.
 *
 * This definition puts the following two extra restrictions when
 * defining percpu variables.
 *
 * 1. The symbol must be globally unique, even the static ones.
 * 2. Static percpu variables cannot be defined inside a function.
 *
 * Archs which need weak percpu definitions should define
 * ARCH_NEEDS_WEAK_PER_CPU in asm/percpu.h when necessary.
 *
 * To ensure that the generic code observes the above two
 * restrictions, if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is set weak
 * definition is used for all cases.

so they can be static, but must be globally unique.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[toc] | [prev] | [next] | [standalone]

#1304172

On Fri, Jan 08, 2016 at 03:08:50PM +1100, Stephen Rothwell wrote:
> Hi Paul,
> 
> On Thu, 7 Jan 2016 19:41:57 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >
> > Indeed, I suspect that SRCU might not be the only thing that would like
> > static per-CPU variables.  ;-)
> 
> from include/linux/percpu-defs.h:
> 
>  * s390 and alpha modules require percpu variables to be defined as
>  * weak to force the compiler to generate GOT based external
>  * references for them.  This is necessary because percpu sections
>  * will be located outside of the usually addressable area.
>  *
>  * This definition puts the following two extra restrictions when
>  * defining percpu variables.
>  *
>  * 1. The symbol must be globally unique, even the static ones.
>  * 2. Static percpu variables cannot be defined inside a function.
>  *
>  * Archs which need weak percpu definitions should define
>  * ARCH_NEEDS_WEAK_PER_CPU in asm/percpu.h when necessary.
>  *
>  * To ensure that the generic code observes the above two
>  * restrictions, if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is set weak
>  * definition is used for all cases.
> 
> so they can be static, but must be globally unique.

Thank you for the tutorial!  I have updated the patch to use a unique
name for the srcu_struct added by rcuperf.c.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1304176

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jan 07, 2016 at 08:48:35PM -0800, Paul E. McKenney wrote:
> On Fri, Jan 08, 2016 at 03:08:50PM +1100, Stephen Rothwell wrote:
> > Hi Paul,
> > 
> > On Thu, 7 Jan 2016 19:41:57 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > Indeed, I suspect that SRCU might not be the only thing that would like
> > > static per-CPU variables.  ;-)
> > 
> > from include/linux/percpu-defs.h:
> > 
> >  * s390 and alpha modules require percpu variables to be defined as
> >  * weak to force the compiler to generate GOT based external
> >  * references for them.  This is necessary because percpu sections
> >  * will be located outside of the usually addressable area.
> >  *
> >  * This definition puts the following two extra restrictions when
> >  * defining percpu variables.
> >  *
> >  * 1. The symbol must be globally unique, even the static ones.
> >  * 2. Static percpu variables cannot be defined inside a function.
> >  *
> >  * Archs which need weak percpu definitions should define
> >  * ARCH_NEEDS_WEAK_PER_CPU in asm/percpu.h when necessary.
> >  *
> >  * To ensure that the generic code observes the above two
> >  * restrictions, if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is set weak
> >  * definition is used for all cases.
> > 
> > so they can be static, but must be globally unique.

Interesting ;-) Thank you.

> 
> Thank you for the tutorial!  I have updated the patch to use a unique
> name for the srcu_struct added by rcuperf.c.
> 

Plus a comment for DEFINE_STATIC_SRCU saying srcu can be defined as
static but must have a unique name for this reason?

Regards,
Boqun

> 							Thanx, Paul
> 

[toc] | [prev] | [next] | [standalone]

#1304675

On Fri, Jan 08, 2016 at 07:53:41AM -0800, Paul E. McKenney wrote:
> commit 61822ff81839ee3c5b6094ce348944d972b87892
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Fri Jan 8 07:43:50 2016 -0800
> 
>     rcu: Document unique-name limitation for DEFINE_STATIC_SRCU()
>     
>     SRCU uses per-CPU variables, and DEFINE_STATIC_SRCU() uses a static
>     per-CPU variable.  However, per-CPU variables have significant
>     restrictions, for example, names of per-CPU variables must be globally
>     unique, even if declared static.  These restrictions carry over to
>     DEFINE_STATIC_SRCU(), and this commit therefore documents these
>     restrictions.
>     
>     Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>     Reported-by: kbuild test robot <fengguang.wu@intel.com>
>     Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index f5f80c5643ac..dc8eb63c6568 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -99,8 +99,23 @@ void process_srcu(struct work_struct *work);
>  	}
>  
>  /*
> - * define and init a srcu struct at build time.
> - * dont't call init_srcu_struct() nor cleanup_srcu_struct() on it.
> + * Define and initialize a srcu struct at build time.
> + * Do -not- call init_srcu_struct() nor cleanup_srcu_struct() on it.
> + *
> + * Note that although DEFINE_STATIC_SRCU() hides the name from other
> + * files, the per-CPU variable rules nevertheless require that the
> + * chosen name be globally unique.  These rules also prohibit use of
> + * DEFINE_STATIC_SRCU() within a function.  If these rules are too
> + * restrictive, declare the srcu_struct manually.  For example, in
> + * each file:
> + *
> + *	static struct srcu_struct my_srcu;
> + *
> + * Then, before the first use of each my_srcu, manually initialize it:
> + *
> + *	init_srcu_struct(&my_srcu);
> + *
> + * See include/linux/percpu-defs.h for the rules on per-CPU variables.

FWIW,

 Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks Stephen for posting the rules around static percpu definitions.
I wonder whether there is a better way to tell people what's going on.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1304703

On Fri, Jan 08, 2016 at 10:57:27AM -0500, Tejun Heo wrote:
> On Fri, Jan 08, 2016 at 07:53:41AM -0800, Paul E. McKenney wrote:
> > commit 61822ff81839ee3c5b6094ce348944d972b87892
> > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Date:   Fri Jan 8 07:43:50 2016 -0800
> > 
> >     rcu: Document unique-name limitation for DEFINE_STATIC_SRCU()
> >     
> >     SRCU uses per-CPU variables, and DEFINE_STATIC_SRCU() uses a static
> >     per-CPU variable.  However, per-CPU variables have significant
> >     restrictions, for example, names of per-CPU variables must be globally
> >     unique, even if declared static.  These restrictions carry over to
> >     DEFINE_STATIC_SRCU(), and this commit therefore documents these
> >     restrictions.
> >     
> >     Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> >     Reported-by: kbuild test robot <fengguang.wu@intel.com>
> >     Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index f5f80c5643ac..dc8eb63c6568 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -99,8 +99,23 @@ void process_srcu(struct work_struct *work);
> >  	}
> >  
> >  /*
> > - * define and init a srcu struct at build time.
> > - * dont't call init_srcu_struct() nor cleanup_srcu_struct() on it.
> > + * Define and initialize a srcu struct at build time.
> > + * Do -not- call init_srcu_struct() nor cleanup_srcu_struct() on it.
> > + *
> > + * Note that although DEFINE_STATIC_SRCU() hides the name from other
> > + * files, the per-CPU variable rules nevertheless require that the
> > + * chosen name be globally unique.  These rules also prohibit use of
> > + * DEFINE_STATIC_SRCU() within a function.  If these rules are too
> > + * restrictive, declare the srcu_struct manually.  For example, in
> > + * each file:
> > + *
> > + *	static struct srcu_struct my_srcu;
> > + *
> > + * Then, before the first use of each my_srcu, manually initialize it:
> > + *
> > + *	init_srcu_struct(&my_srcu);
> > + *
> > + * See include/linux/percpu-defs.h for the rules on per-CPU variables.
> 
> FWIW,
> 
>  Reviewed-by: Tejun Heo <tj@kernel.org>

Applied, thank you!

> Thanks Stephen for posting the rules around static percpu definitions.

Me too!  Hey, it was building fine with all of my configurations,
so I was really beginning to wonder if this was some sort of early
April Fools joke.  ;-)

> I wonder whether there is a better way to tell people what's going on.

I hope so.  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1304676

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 08, 2016 at 07:53:41AM -0800, Paul E. McKenney wrote:
> On Fri, Jan 08, 2016 at 12:54:53PM +0800, Boqun Feng wrote:
> > On Thu, Jan 07, 2016 at 08:48:35PM -0800, Paul E. McKenney wrote:
> > > On Fri, Jan 08, 2016 at 03:08:50PM +1100, Stephen Rothwell wrote:
> > > > Hi Paul,
> > > > 
> > > > On Thu, 7 Jan 2016 19:41:57 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > > >
> > > > > Indeed, I suspect that SRCU might not be the only thing that would like
> > > > > static per-CPU variables.  ;-)
> > > > 
> > > > from include/linux/percpu-defs.h:
> > > > 
> > > >  * s390 and alpha modules require percpu variables to be defined as
> > > >  * weak to force the compiler to generate GOT based external
> > > >  * references for them.  This is necessary because percpu sections
> > > >  * will be located outside of the usually addressable area.
> > > >  *
> > > >  * This definition puts the following two extra restrictions when
> > > >  * defining percpu variables.
> > > >  *
> > > >  * 1. The symbol must be globally unique, even the static ones.
> > > >  * 2. Static percpu variables cannot be defined inside a function.
> > > >  *
> > > >  * Archs which need weak percpu definitions should define
> > > >  * ARCH_NEEDS_WEAK_PER_CPU in asm/percpu.h when necessary.
> > > >  *
> > > >  * To ensure that the generic code observes the above two
> > > >  * restrictions, if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is set weak
> > > >  * definition is used for all cases.
> > > > 
> > > > so they can be static, but must be globally unique.
> > 
> > Interesting ;-) Thank you.
> > 
> > > Thank you for the tutorial!  I have updated the patch to use a unique
> > > name for the srcu_struct added by rcuperf.c.
> > 
> > Plus a comment for DEFINE_STATIC_SRCU saying srcu can be defined as
> > static but must have a unique name for this reason?
> 
> Yes, plus some words about how to work around the limitation.  How about
> the patch below?
> 

Looks very good to me ;-)

Regards,
Boqun

> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 61822ff81839ee3c5b6094ce348944d972b87892
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Fri Jan 8 07:43:50 2016 -0800
> 
>     rcu: Document unique-name limitation for DEFINE_STATIC_SRCU()
>     
>     SRCU uses per-CPU variables, and DEFINE_STATIC_SRCU() uses a static
>     per-CPU variable.  However, per-CPU variables have significant
>     restrictions, for example, names of per-CPU variables must be globally
>     unique, even if declared static.  These restrictions carry over to
>     DEFINE_STATIC_SRCU(), and this commit therefore documents these
>     restrictions.
>     
>     Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>     Reported-by: kbuild test robot <fengguang.wu@intel.com>
>     Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index f5f80c5643ac..dc8eb63c6568 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -99,8 +99,23 @@ void process_srcu(struct work_struct *work);
>  	}
>  
>  /*
> - * define and init a srcu struct at build time.
> - * dont't call init_srcu_struct() nor cleanup_srcu_struct() on it.
> + * Define and initialize a srcu struct at build time.
> + * Do -not- call init_srcu_struct() nor cleanup_srcu_struct() on it.
> + *
> + * Note that although DEFINE_STATIC_SRCU() hides the name from other
> + * files, the per-CPU variable rules nevertheless require that the
> + * chosen name be globally unique.  These rules also prohibit use of
> + * DEFINE_STATIC_SRCU() within a function.  If these rules are too
> + * restrictive, declare the srcu_struct manually.  For example, in
> + * each file:
> + *
> + *	static struct srcu_struct my_srcu;
> + *
> + * Then, before the first use of each my_srcu, manually initialize it:
> + *
> + *	init_srcu_struct(&my_srcu);
> + *
> + * See include/linux/percpu-defs.h for the rules on per-CPU variables.
>   */
>  #define __DEFINE_SRCU(name, is_static)					\
>  	static DEFINE_PER_CPU(struct srcu_struct_array, name##_srcu_array);\
> 

[toc] | [prev] | [next] | [standalone]

#1304678

On Fri, Jan 08, 2016 at 12:54:53PM +0800, Boqun Feng wrote:
> On Thu, Jan 07, 2016 at 08:48:35PM -0800, Paul E. McKenney wrote:
> > On Fri, Jan 08, 2016 at 03:08:50PM +1100, Stephen Rothwell wrote:
> > > Hi Paul,
> > > 
> > > On Thu, 7 Jan 2016 19:41:57 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > Indeed, I suspect that SRCU might not be the only thing that would like
> > > > static per-CPU variables.  ;-)
> > > 
> > > from include/linux/percpu-defs.h:
> > > 
> > >  * s390 and alpha modules require percpu variables to be defined as
> > >  * weak to force the compiler to generate GOT based external
> > >  * references for them.  This is necessary because percpu sections
> > >  * will be located outside of the usually addressable area.
> > >  *
> > >  * This definition puts the following two extra restrictions when
> > >  * defining percpu variables.
> > >  *
> > >  * 1. The symbol must be globally unique, even the static ones.
> > >  * 2. Static percpu variables cannot be defined inside a function.
> > >  *
> > >  * Archs which need weak percpu definitions should define
> > >  * ARCH_NEEDS_WEAK_PER_CPU in asm/percpu.h when necessary.
> > >  *
> > >  * To ensure that the generic code observes the above two
> > >  * restrictions, if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is set weak
> > >  * definition is used for all cases.
> > > 
> > > so they can be static, but must be globally unique.
> 
> Interesting ;-) Thank you.
> 
> > Thank you for the tutorial!  I have updated the patch to use a unique
> > name for the srcu_struct added by rcuperf.c.
> 
> Plus a comment for DEFINE_STATIC_SRCU saying srcu can be defined as
> static but must have a unique name for this reason?

Yes, plus some words about how to work around the limitation.  How about
the patch below?

							Thanx, Paul

------------------------------------------------------------------------

commit 61822ff81839ee3c5b6094ce348944d972b87892
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Fri Jan 8 07:43:50 2016 -0800

    rcu: Document unique-name limitation for DEFINE_STATIC_SRCU()
    
    SRCU uses per-CPU variables, and DEFINE_STATIC_SRCU() uses a static
    per-CPU variable.  However, per-CPU variables have significant
    restrictions, for example, names of per-CPU variables must be globally
    unique, even if declared static.  These restrictions carry over to
    DEFINE_STATIC_SRCU(), and this commit therefore documents these
    restrictions.
    
    Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
    Reported-by: kbuild test robot <fengguang.wu@intel.com>
    Suggested-by: Boqun Feng <boqun.feng@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index f5f80c5643ac..dc8eb63c6568 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -99,8 +99,23 @@ void process_srcu(struct work_struct *work);
 	}
 
 /*
- * define and init a srcu struct at build time.
- * dont't call init_srcu_struct() nor cleanup_srcu_struct() on it.
+ * Define and initialize a srcu struct at build time.
+ * Do -not- call init_srcu_struct() nor cleanup_srcu_struct() on it.
+ *
+ * Note that although DEFINE_STATIC_SRCU() hides the name from other
+ * files, the per-CPU variable rules nevertheless require that the
+ * chosen name be globally unique.  These rules also prohibit use of
+ * DEFINE_STATIC_SRCU() within a function.  If these rules are too
+ * restrictive, declare the srcu_struct manually.  For example, in
+ * each file:
+ *
+ *	static struct srcu_struct my_srcu;
+ *
+ * Then, before the first use of each my_srcu, manually initialize it:
+ *
+ *	init_srcu_struct(&my_srcu);
+ *
+ * See include/linux/percpu-defs.h for the rules on per-CPU variables.
  */
 #define __DEFINE_SRCU(name, is_static)					\
 	static DEFINE_PER_CPU(struct srcu_struct_array, name##_srcu_array);\

[toc] | [prev] | [next] | [standalone]

#1304163

Hi Boqun,

On Fri, 8 Jan 2016 09:37:04 +0800 Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Mind to check your config options, Stephen?

This was an allyesconfig build, so I assume that
CONFIG_DEBUG_FORCE_WEAK_PER_CPU was set (sorry, I don;t have
the .config file at the moment).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web