Messages in this thread | | | Date | Wed, 25 Nov 2009 12:58:56 +0100 | From | Ingo Molnar <> | Subject | Re: linux-next: percpu tree build warning |
| |
(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
| |