Re: linux-next: percpu tree build warning

Ingo Molnar
2009-11-25T11:59:16Z

(Cc:-ed Linus and Andrew)

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Wed, 25 Nov 2009 09:20:04 pm Ingo Molnar 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.
>
> No, we've been through this.
>
> sparse annotations replace the per_cpu prefix now per-cpu vars can be
> used withn other than per-cpu ops (ie. their address can be usefully
> taken).
>
> The prefix crutch predated sparse. And it was certainly never
> supposed to let people write confusing and crap code like this.

What's confusing and crap about the code below? I dont think it is
confusing, nor crap:

int arch_install_hw_breakpoint(struct perf_event *bp)
{
struct arch_hw_breakpoint *info = counter_arch_bp(bp);
unsigned long *dr7;
int i;

for (i = 0; i < HBP_NUM; i++) {
struct perf_event **slot = &__get_cpu_var(bp_per_reg[i]);

if (!*slot) {
*slot = bp;
break;
}
}

if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
return -EBUSY;

set_debugreg(info->address, i);
__get_cpu_var(cpu_debugreg[i]) = info->address;

dr7 = &__get_cpu_var(dr7);
*dr7 |= encode_dr7(i, info->len, info->type);

set_debugreg(*dr7, 7);

return 0;
}

This is basically equivalent to:

pid = task->pid;

the 'dr7' in the local scope is clearly different from the
__get_cpu_var(dr7) variable.

percpu variables are basically in a special struct. It's not like you
can _ever_ access 'dr7' the percpu variable like that - it _always_ has
to go via a proper percpu wrapper construct. So this change is
needlessly obtrusive.

Really, guys, while the workaround is easy (a rename), this might be
going a bit too far. I already think that the recently introduced
limitation to name local percpu symbols globally sucked - but i'm not
sure whether this new rule of not allowing such clear and clean looking
code is acceptable.

Percpu variables now pollute _both_ the global and the local namespace -
i dont think you can have it both ways.

Ingo