Re: linux-next: percpu tree build warning

Ingo Molnar
2009-11-25T13:41:42Z

* Tejun Heo <tj@kernel.org> wrote:

> > If yes then that needs to be fixed in the percpu tree. per-cpu
> > variables used to have a __per_cpu prefix and that should be
> > maintained - the two namespaces are obviously separate on the
> > logical space, so they should never overlap in the implementational
> > space either.
>
> If all we ever have are static variables, the prefix may be fine but
> with dynamic percpu variables now basically being the same first class
> citizen but prefix just doesn't cut it. It just ends up adding more
> confusion. The transition will be a bit painful (but not too much,
> how many of these reports have we had? Only several) but after that
> it's just plain local/global symbol collision the compiler would have
> no problem warning about. It behaves exactly like other global
> symbols.
>
> Percpu symbols and variables belong to a different address space than
> normal symbols. Unfortunately, C doesn't have support for such thing.

That argument does not parse for me. Obviously no sane programming
language should allow shadowed variables which are used in the same way
- it's way too easy to use the wrong one.

But we have _no_ real shadowing here - it's a pure artifact of how the
percpu symbol space is mapped back into C - and the collision (which
does not exist in the program space) is created where none existed
before.

In other words: you are solving a problem that does not exist - you
cannot mix up a local C variable and a percpu variable. The two spaces
are clearly separated via definition and APIs. A C variable is defined
via:

unsigned long *dr7;

and is used via:

dr7

While a percpu variable is defined and used in completely different
ways:

DEFINE_PER_CPU(unsigned long, dr7);

and is used via:

__get_cpu_var(cpu_dr7);

It's analogous as if we had a 'struct percpu' C structure, and
dereferenced it via:

cpu->dr7.

Note that we dont require it to be renamed to cpu->cpu_dr7.

And look at your own 'cleanup' patch - it changes the percpu name to
'cpu_dr7'. That results in nonsensical repetition:

dr7 = &__get_cpu_var(cpu_dr7);

I already said it's a percpu variable, via the __get_cpu_var()
primitive. Why do i have to type cpu_ again to express this, hm?

These kinds of messy interactions between clearly disjunct name spaces
are bad IMO. And i dont see how dynamic percpu variables change this in
any way - none of the above is a dynamic percpu variable.

I've applied your patch to not hold things up in linux-next (shadowing
is dangerous) but i dont see how your arguments add up.

Ingo