Re: linux-next: percpu tree build warning

[Ingo Molnar <mingo@elte.hu>]

(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