initialization of static per-cpu variables

initialization of static per-cpu variables

[Vegard Nossum]

Hi,

I encountered this comment in kernel/softirq.c:

/* Some compilers disobey section attribute on statics when not
   initialized -- RR */
static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };

So I assume it's the combination of static and whatever section
DFINE_PER_CPU puts the variable in which is the problem.

However, there's a LOT of these "static DEFINE_PER_CPU" without any
initializer in the rest of the code, e.g.:

$ g 'static DEFINE_PER_CPU' kernel/sched.c
static DEFINE_PER_CPU(struct sched_entity, init_sched_entity);
static DEFINE_PER_CPU(struct cfs_rq, init_cfs_rq) ____cacheline_aligned_in_smp;
static DEFINE_PER_CPU(struct sched_rt_entity, init_sched_rt_entity);
static DEFINE_PER_CPU(struct rt_rq, init_rt_rq) ____cacheline_aligned_in_smp;
static DEFINE_PER_CPU_SHARED_ALIGNED(struct rq, runqueues);
static DEFINE_PER_CPU(unsigned long long, time_offset);
static DEFINE_PER_CPU(unsigned long long, prev_cpu_time);
static DEFINE_PER_CPU(spinlock_t, aggregate_lock);
static DEFINE_PER_CPU(struct sched_domain, cpu_domains);
static DEFINE_PER_CPU(struct sched_group, sched_group_cpus);
static DEFINE_PER_CPU(struct sched_domain, core_domains);
static DEFINE_PER_CPU(struct sched_group, sched_group_core);
static DEFINE_PER_CPU(struct sched_domain, phys_domains);
static DEFINE_PER_CPU(struct sched_group, sched_group_phys);
static DEFINE_PER_CPU(struct sched_domain, node_domains);
static DEFINE_PER_CPU(struct sched_domain, allnodes_domains);
static DEFINE_PER_CPU(struct sched_group, sched_group_allnodes);

The comment seems to be ancient and I don't know who wrote it, so I'm
making a guess that it's Rusty Russel (Cced).

So which do we do, delete the comment (on the grounds that it is
invalid) or fix these other declarations (there's a lot of them)?

Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: initialization of static per-cpu variables

[Rusty Russell]

On Thursday 22 May 2008 04:28:02 Vegard Nossum wrote:
> Hi,
>
> I encountered this comment in kernel/softirq.c:
>
> /* Some compilers disobey section attribute on statics when not
>    initialized -- RR */
> static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
> static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };
>
> So I assume it's the combination of static and whatever section
> DFINE_PER_CPU puts the variable in which is the problem.
>
> However, there's a LOT of these "static DEFINE_PER_CPU" without any
> initializer in the rest of the code, e.g.:

Yep, it was an old toolchain used by Sparc: DaveM found this one.  As you say, 
it's ancient: I'm happy to queue a cleanup patch now everyone is on a modern 
compiler.

Thanks,
Rusty.

Re: initialization of static per-cpu variables

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Thu, 22 May 2008 18:20:06 +1000

> On Thursday 22 May 2008 04:28:02 Vegard Nossum wrote:
> > Hi,
> >
> > I encountered this comment in kernel/softirq.c:
> >
> > /* Some compilers disobey section attribute on statics when not
> >    initialized -- RR */
> > static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
> > static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };
> >
> > So I assume it's the combination of static and whatever section
> > DFINE_PER_CPU puts the variable in which is the problem.
> >
> > However, there's a LOT of these "static DEFINE_PER_CPU" without any
> > initializer in the rest of the code, e.g.:
> 
> Yep, it was an old toolchain used by Sparc: DaveM found this one.  As you say, 
> it's ancient: I'm happy to queue a cleanup patch now everyone is on a modern 
> compiler.

Yes, that workaround certainly can die now.

Re: initialization of static per-cpu variables

[Adrian Bunk]

On Thu, May 22, 2008 at 06:20:06PM +1000, Rusty Russell wrote:
> On Thursday 22 May 2008 04:28:02 Vegard Nossum wrote:
> > Hi,
> >
> > I encountered this comment in kernel/softirq.c:
> >
> > /* Some compilers disobey section attribute on statics when not
> >    initialized -- RR */
> > static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
> > static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };
> >
> > So I assume it's the combination of static and whatever section
> > DFINE_PER_CPU puts the variable in which is the problem.
> >
> > However, there's a LOT of these "static DEFINE_PER_CPU" without any
> > initializer in the rest of the code, e.g.:
> 
> Yep, it was an old toolchain used by Sparc: DaveM found this one.  As you say, 
> it's ancient: I'm happy to queue a cleanup patch now everyone is on a modern 
> compiler.

The commit says:

    [PATCH] softirq.c per_cpu fix
    
    GCC3.1 apparently gets confused about uninitialized sections


We do still support gcc 3.2 (which is the same as 3.1 except for a C++ 
ABI change) as a compiler for the kernel.


> Thanks,
> Rusty.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: initialization of static per-cpu variables

[Vegard Nossum]

On Fri, May 23, 2008 at 4:29 PM, Adrian Bunk <bunk@kernel.org> wrote:
> On Thu, May 22, 2008 at 06:20:06PM +1000, Rusty Russell wrote:
>> On Thursday 22 May 2008 04:28:02 Vegard Nossum wrote:
>> > Hi,
>> >
>> > I encountered this comment in kernel/softirq.c:
>> >
>> > /* Some compilers disobey section attribute on statics when not
>> >    initialized -- RR */
>> > static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
>> > static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };
>> >
>> > So I assume it's the combination of static and whatever section
>> > DFINE_PER_CPU puts the variable in which is the problem.
>> >
>> > However, there's a LOT of these "static DEFINE_PER_CPU" without any
>> > initializer in the rest of the code, e.g.:
>>
>> Yep, it was an old toolchain used by Sparc: DaveM found this one.  As you say,
>> it's ancient: I'm happy to queue a cleanup patch now everyone is on a modern
>> compiler.
>
> The commit says:
>
>    [PATCH] softirq.c per_cpu fix
>
>    GCC3.1 apparently gets confused about uninitialized sections
>
>
> We do still support gcc 3.2 (which is the same as 3.1 except for a C++
> ABI change) as a compiler for the kernel.

Ah, thanks for looking that up!

The question is then whether 3.1/3.2 will _ever_ do the right thing,
when you consider the number of places in the kernel that *don't*
initialize static per-cpu variables. (Or maybe: What makes this
variable so special that it breaks the kernel?)

Maybe it was fixed by accident in the kernel at some point later (e.g.
by change of the linker script) or maybe the commit message/original
reason was wrong and it was really something else?

Or maybe it is time to deprecate gcc 3.1/3.2 support?

Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: initialization of static per-cpu variables

[Rusty Russell]

On Saturday 24 May 2008 00:29:04 Adrian Bunk wrote:
> On Thu, May 22, 2008 at 06:20:06PM +1000, Rusty Russell wrote:
> > Yep, it was an old toolchain used by Sparc: DaveM found this one.  As you
> > say, it's ancient: I'm happy to queue a cleanup patch now everyone is on
> > a modern compiler.
>
> The commit says:
>
>     GCC3.1 apparently gets confused about uninitialized sections
>
> We do still support gcc 3.2 (which is the same as 3.1 except for a C++
> ABI change) as a compiler for the kernel.

Adrian, that's a little silly.  There are obviously bug fixes in 3.2 over 
3.1.0.  Noone has complained about the introduction of multiple other cases 
which would screw things up if they experienced this bug.

Finally, it's a sparc64 problem and DaveM acked already.  That's half the 
userbase!

Cheers,
Rusty.

Re: initialization of static per-cpu variables

[Adrian Bunk]

On Sun, May 25, 2008 at 08:35:24PM +1000, Rusty Russell wrote:
> On Saturday 24 May 2008 00:29:04 Adrian Bunk wrote:
> > On Thu, May 22, 2008 at 06:20:06PM +1000, Rusty Russell wrote:
> > > Yep, it was an old toolchain used by Sparc: DaveM found this one.  As you
> > > say, it's ancient: I'm happy to queue a cleanup patch now everyone is on
> > > a modern compiler.
> >
> > The commit says:
> >
> >     GCC3.1 apparently gets confused about uninitialized sections
> >
> > We do still support gcc 3.2 (which is the same as 3.1 except for a C++
> > ABI change) as a compiler for the kernel.
> 
> Adrian, that's a little silly.  There are obviously bug fixes in 3.2 over 
> 3.1.0.

I've checked the announcements of 3.1.1 and 3.2, and at least for me 
nothing looked like it would fix this bug.

> Noone has complained about the introduction of multiple other cases 
> which would screw things up if they experienced this bug.

I doubt there is any serious userbase for gcc 3.2 left.

But your "now everyone is on a modern compiler" does not match what we 
announce as supported compiler versions for the kernel.

If you have a good reason for pushing the minimum required gcc version 
for compiling the kernel to 3.3 or 3.4 [1] you have my full support, but 
as long as we officially support gcc 3.2 we should try to break as few 
as possible - especially since it will take time until anyone will run 
into any breakage.

> Finally, it's a sparc64 problem and DaveM acked already.

Was it a sparc64 problem or a generic problem DaveM happened to run into 
on sparc?

> That's half the userbase!

sparc64 isn't that unpopular...

> Cheers,
> Rusty.

Sorry if I'm sounding overly pedantic, but I want that what we try as 
good as possible that what we announce as being supported also works 
(even if this results in several workarounds shipped).

cu
Adrian

[1] gcc 3.4 still has a serious userbase at least in ARM country, so you
    won't be able to drop support for it

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: initialization of static per-cpu variables

[Rusty Russell]

On Monday 02 June 2008 17:59:37 Adrian Bunk wrote:
> On Sun, May 25, 2008 at 08:35:24PM +1000, Rusty Russell wrote:
> > Finally, it's a sparc64 problem and DaveM acked already.
>
> Was it a sparc64 problem or a generic problem DaveM happened to run into
> on sparc?

Yes, it was a sparc specific problem.

Rusty.