linux-next: percpu tree build warning

linux-next: percpu tree build warning

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Hi all,

Today's linux-next build (x86_64 allmodconfig) produced this warning:

arch/x86/kernel/hw_breakpoint.c: In function 'arch_install_hw_breakpoint':
arch/x86/kernel/hw_breakpoint.c:121: warning: assignment from incompatible pointer type
arch/x86/kernel/hw_breakpoint.c: In function 'arch_uninstall_hw_breakpoint':
arch/x86/kernel/hw_breakpoint.c:156: warning: assignment from incompatible pointer type

I am not sure where this one came from ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next: percpu tree build warning

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi all,
> 
> Today's linux-next build (x86_64 allmodconfig) produced this warning:
> 
> arch/x86/kernel/hw_breakpoint.c: In function 'arch_install_hw_breakpoint':
> arch/x86/kernel/hw_breakpoint.c:121: warning: assignment from incompatible pointer type
> arch/x86/kernel/hw_breakpoint.c: In function 'arch_uninstall_hw_breakpoint':
> arch/x86/kernel/hw_breakpoint.c:156: warning: assignment from incompatible pointer type
> 
> I am not sure where this one came from ...

the code does:

DEFINE_PER_CPU(unsigned long, dr7);

int arch_install_hw_breakpoint(struct perf_event *bp)
{
...
        unsigned long *dr7;
...
        dr7 = &__get_cpu_var(dr7);
...
}

Tejun, is it perhaps a problem of the percpu code getting confused 
between the local and file scope 'dr7' variable shadowing each other?

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.

	Ingo

Re: linux-next: percpu tree build warning

[Rusty Russell]

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.

Rusty.

Re: linux-next: percpu tree build warning

[Ingo Molnar]

(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

Re: linux-next: percpu tree build warning

[Tejun Heo]

Hello,

11/25/2009 08:58 PM, Ingo Molnar wrote:
> 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.

The only problem is that now the addresses can and need be handled as
value.  If we keep the prefix, we just end up with one set of
accessors which prepend prefix to the symbol string and another set
which are basically the same but lack any protection (we already have
them - this_cpu accessors).  The current for-next tree is sort of
caught up inbetween.  Once sparse warning cleanup sweep is complete,
all static specific accessors will be dropped.

> 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.

I agree static local symbol requiring global uniqueness truly sucks
but this is a completely different issue.  This is making percpu
variables behave more sanely and the fallouts are few and linux-next
warning check is enough to detect the few.

Thanks.

-- 
tejun

Re: linux-next: percpu tree build warning

[Tejun Heo]

Hello,

11/25/2009 07:50 PM, Ingo Molnar wrote:
> Tejun, is it perhaps a problem of the percpu code getting confused 
> between the local and file scope 'dr7' variable shadowing each other?

Yes.

> 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.
Prefixing kind of works but simply breaks when the addresses are
allowed to be handled as values.  We have had the exactly same problem
and solution for years now - iomem.  Percpu memory isn't different
from iomem at all.  Once the conversion is complete and annotations
and code are upstream, it won't be painful at all.

Thanks.

-- 
tejun

Re: linux-next: percpu tree build warning

[Ingo Molnar]

* 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

Re: linux-next: percpu tree build warning

[Tejun Heo]

Hello,

11/25/2009 10:40 PM, Ingo Molnar wrote:
> 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);

My whole argument can be compressed into "don't name a global symbol
dr7, no matter what it is".

The key problem is the artificial difference between static and
dynamic percpu variable accessors.  The old way of prefixing from
accessors only works for symbol literals, so either we need another
identical set for dynamic ones without auto-prefixing or we end up
doing the repetition you mentioned above in much uglier way.

Option 1:

	this_cpu_static_OP(dr7, ARG);
	this_cpu_dynamic_OP(*allocated_ptr, ARG);
	this_cpu_dynamic_OP(per_cpu_var(dr7), ARG);

Options 2:

	this_cpu_OP(per_cpu_var(dr7), ARG);

BTW, option 2 is what we've been doing before the change.  It's just
ugly and the prefix no longer provides much protection because users
outside of percpu code have to use per_cpu_var() which never was
supposed to go outside of percpu internal code.  All it ends up doing
is providing false sense of address space isolation when there is
none.

DEFINE_*(NAME) defines a global symbol NAME in all other definition
macros.  DEFINE_PER_CPU() does so too.

Thanks.

-- 
tejun

Re: linux-next: percpu tree build warning

[Rusty Russell]

On Thu, 26 Nov 2009 12:10:58 am Ingo Molnar wrote:
> 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(dr7);  [[Fixed -- RR]]

The entire point of Tejun's per-cpu work is that &dr7 is now valid.  A per-cpu
pointer as if it were allocated by the dynamic per-cpu allocator.

Your arguments are fine, but out-of-date.

Rusty.

Re: linux-next: percpu tree build warning

[Ingo Molnar]

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

> On Thu, 26 Nov 2009 12:10:58 am Ingo Molnar wrote:
> > 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(dr7);  [[Fixed -- RR]]
> 
> The entire point of Tejun's per-cpu work is that &dr7 is now valid. A 
> per-cpu pointer as if it were allocated by the dynamic per-cpu 
> allocator.
>
> Your arguments are fine, but out-of-date.

But allowing &dr7 is outright dangerous - and not particularly clean 
either.

Nothing tells us that it's a percpu variable and it blends into the 
regular namespace while most of the operators on it are special 
(__get_cpu_var(), per_cpu(), __this_cpu(), etc.).

What if someone writes &dr7 in preemptible code? It's dangerous to do it 
and a quick review wont catch the mistake. Seeing &per_cpu_dr7 in 
clearly preemptible code does raise alarms on the other hand.

So i think it should be valid to take the address of it and unify the 
static and dynamic percpu space ... if it's prefixed properly: what's 
wrong with &per_cpu_dr7?

Either make it fully blend in or keep it separate - but dont do it 
half-ways, that's only causing confusion down the road. Furthermore, i 
think per cpu logic and code is tricky enough to be documented clearly, 
every time it's used. We have bugs with such code again and again, and 
disproportionately so.

So AFAICS this change is going in exactly the wrong direction, makes the 
percpu code less readable and more obstructed and more inconsistent, and 
does it for all the wrong reasons and is causing some collateral damage 
as well.

	Ingo

Re: linux-next: percpu tree build warning

[Tejun Heo]

Hello,

11/27/2009 02:41 PM, Ingo Molnar wrote:
> But allowing &dr7 is outright dangerous - and not particularly clean 
> either.
> 
> Nothing tells us that it's a percpu variable and it blends into the 
> regular namespace while most of the operators on it are special 
> (__get_cpu_var(), per_cpu(), __this_cpu(), etc.).
> 
> What if someone writes &dr7 in preemptible code? It's dangerous to do it 
> and a quick review wont catch the mistake. Seeing &per_cpu_dr7 in 
> clearly preemptible code does raise alarms on the other hand.
>
> So i think it should be valid to take the address of it and unify the 
> static and dynamic percpu space ... if it's prefixed properly: what's 
> wrong with &per_cpu_dr7?

DEFINE_PER_CPU(unsigned long, reg0);
DEFINE_PER_CPU(unsigned long, reg1);

static void my_fn(void)
{
	unsigned long reg0 = per_cpu_var(reg0);
	unsigned long reg1 = per_cpu_var(reg1);
	unsigned long *p = &per_cpu_var(reg0);

	// blah blah

	if (some cond)
		p = &reg1;	// oops meant &per_cpu_var(reg1)

	// blah blah

	this_cpu_inc(p);
}

It's more dangerous to depend on the pseudo namespace created by
prefixing.  Let's add __percpu sparse annotations.  It will be more
flexible and safer.

Thanks.

-- 
tejun

Re: linux-next: percpu tree build warning

[Ingo Molnar]

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

> Hello,
> 
> 11/27/2009 02:41 PM, Ingo Molnar wrote:
> > But allowing &dr7 is outright dangerous - and not particularly clean 
> > either.
> > 
> > Nothing tells us that it's a percpu variable and it blends into the 
> > regular namespace while most of the operators on it are special 
> > (__get_cpu_var(), per_cpu(), __this_cpu(), etc.).
> > 
> > What if someone writes &dr7 in preemptible code? It's dangerous to do it 
> > and a quick review wont catch the mistake. Seeing &per_cpu_dr7 in 
> > clearly preemptible code does raise alarms on the other hand.
> >
> > So i think it should be valid to take the address of it and unify the 
> > static and dynamic percpu space ... if it's prefixed properly: what's 
> > wrong with &per_cpu_dr7?
> 
> DEFINE_PER_CPU(unsigned long, reg0);
> DEFINE_PER_CPU(unsigned long, reg1);
> 
> static void my_fn(void)
> {
> 	unsigned long reg0 = per_cpu_var(reg0);
> 	unsigned long reg1 = per_cpu_var(reg1);
> 	unsigned long *p = &per_cpu_var(reg0);
> 
> 	// blah blah
> 
> 	if (some cond)
> 		p = &reg1;	// oops meant &per_cpu_var(reg1)
> 
> 	// blah blah
> 
> 	this_cpu_inc(p);

At least to me a typo like this would stick out like a sore thumb during 
review.

I'd recognize &reg1 as a stack local variable immediately, and when i 
see it being used in this_cpu_inc() i'd go 'huh' immediately.

OTOH, the two examples of confusion i gave you in my previous mail would 
be far less obvious. The 'visual distance' to a percpu variable 
definition is greater (it's at least file scope in 95% of the cases), so 
i wouldnt be able to 'see' which the percpu variables are, from a code 
context.

Anyway, YMMV.

Thanks,

	Ingo

Re: linux-next: percpu tree build warning

[Tejun Heo]

Hello, Ingo.
> At least to me a typo like this would stick out like a sore thumb during 
> review.

Yeah, maybe, but it still shows why reusing the same name for global
and local variables behind compiler's back is a bad idea.

> I'd recognize &reg1 as a stack local variable immediately, and when i 
> see it being used in this_cpu_inc() i'd go 'huh' immediately.
> 
> OTOH, the two examples of confusion i gave you in my previous mail would 
> be far less obvious. The 'visual distance' to a percpu variable 
> definition is greater (it's at least file scope in 95% of the cases), so 
> i wouldnt be able to 'see' which the percpu variables are, from a code 
> context.

With proper __percpu annotations (which we desparately need for
dynamic percpu pointers anyway) the 'visual distance' should remain
fine in most cases, I think.

If we can manage the separate namespace thing without adding confusion
regarding different types of accessors and the actually non-existing
but yet visible differences between static and dynamic percpu
variables, I think it would be good.  But it costs us quite a bit and
__percpu sparse annotation has almost complete coverage over the issue
including the visible queue telling that something is percpu.  So,
given that, to me __percpu seems like a much better way to do it.

Thanks.

-- 
tejun

Re: linux-next: percpu tree build warning

[Tejun Heo]

11/27/2009 03:31 PM, Tejun Heo wrote:
...
> __percpu sparse annotation has almost complete coverage over the issue
> including the visible queue telling that something is percpu.  So,
                        ^^^^^
			 cue, of course.  dang workqueues.

-- 
tejun

Re: linux-next: percpu tree build warning

[Rusty Russell]

On Fri, 27 Nov 2009 04:11:28 pm Ingo Molnar wrote:
> 
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > On Thu, 26 Nov 2009 12:10:58 am Ingo Molnar wrote:
> > > 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(dr7);  [[Fixed -- RR]]
> > 
> > The entire point of Tejun's per-cpu work is that &dr7 is now valid. A 
> > per-cpu pointer as if it were allocated by the dynamic per-cpu 
> > allocator.
> >
> > Your arguments are fine, but out-of-date.
> 
> But allowing &dr7 is outright dangerous - and not particularly clean 
> either.

That's foolish.  We can now have generic per-cpu function for counters
and the like.

> Nothing tells us that it's a percpu variable

__percpu.  Again, I'm explaining what you should already know before sending
email about this stuff.

> and it blends into the 
> regular namespace while most of the operators on it are special 
> (__get_cpu_var(), per_cpu(), __this_cpu(), etc.).

OK, you convince Linus to change __user vars to use a prefix.  Then I'll
agree that per_cpu_## is more kernely.

Stupidest debate ever.
Rusty.

Re: linux-next: percpu tree build warning

[Ingo Molnar]

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

> On Fri, 27 Nov 2009 04:11:28 pm Ingo Molnar wrote:
> > 
> > * Rusty Russell <rusty@rustcorp.com.au> wrote:
> > 
> > > On Thu, 26 Nov 2009 12:10:58 am Ingo Molnar wrote:
> > > > 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(dr7);  [[Fixed -- RR]]
> > > 
> > > The entire point of Tejun's per-cpu work is that &dr7 is now valid. A 
> > > per-cpu pointer as if it were allocated by the dynamic per-cpu 
> > > allocator.
> > >
> > > Your arguments are fine, but out-of-date.
> > 
> > But allowing &dr7 is outright dangerous - and not particularly clean 
> > either.
> 
> That's foolish.  We can now have generic per-cpu function for counters 
> and the like.

So your argument in favor of what i see as at least a mild form of type 
obfusaction is that ... even more obfuscation is upcoming?

I think percpu usage should be spelled out clear and loud. We should not 
pretend they are 'usual' C variables, because they are not. They are 
defined in a special way, they are used via special operators. I sure 
want to make sure that taking an address of one of them:

	ptr = &dr7;

... looks special too.

Just look at the two 'fixes' i quoted in this discussion:

 28b4e0d: x86: Rename global percpu symbol dr7 to cpu_dr7
 11e6635: kernel/hw_breakpoint.c: Fix local/global shadowing

They actually 'solved' the shadowing by renaming the variables to ... 
cpu_. Think about it: the 'I am percpu' prefix came right back - it's 
just now present in a more volatile form and the default usage is 
slightly more dangerous!

I guess i'm a bit more sensitive to percpu complications than you 
because i've seen my fair share of bugs in the scheduler (and 
preemptible/non-preemptible code) related to percpu code (a fair share 
of it introduced by yourself ;-), so the last thing i'd like to see is 
changes that are hiding its nature.

I _use_ percpu code, i dont just write the facilities ;-)

> [...] Again, I'm explaining what you should already know before 
> sending email about this stuff.
> [...]
> Stupidest debate ever.

What i am making is a somewhat subtle technical argument and making any 
progress on it needs at least a minimal form of a working debate. I do 
not claim i am right, but still you are dismissing my arguments in a 
rather nasty way.

... alas, i dont care _that_ much about this and i dont think my 
concerns deserved your ad hominem attacks so i see no point in further 
participating in this thread.

Thanks,

	Ingo

Re: linux-next: percpu tree build warning

[Rusty Russell]

On Sun, 29 Nov 2009 05:10:19 pm Ingo Molnar wrote:
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
> > [...] Again, I'm explaining what you should already know before 
> > sending email about this stuff.
> > [...]
> > Stupidest debate ever.
> 
> What i am making is a somewhat subtle technical argument and making any 
> progress on it needs at least a minimal form of a working debate. I do 
> not claim i am right, but still you are dismissing my arguments in a 
> rather nasty way.

You're right, I was overly abrupt.  Apologies.

You have a point: the compile-time restrictions on per-cpu misuse was nice.  I know, I wrote it :)  But as we start to pass more per-cpu pointers around, it breaks down.  So the new sparse annotations are a better fit, and cover more.

The separate namespace was an unintended side-effect, and not one I'm fond
of.  Unfortunately it became more attractive now static-scope per-cpu vars
are banned for some platforms.  But having two names meaning different things
is bad for tags, grep and casual readers (your example is trivial enough that
we don't care, I agree).

> ... alas, i dont care _that_ much about this and i dont think my 
> concerns deserved your ad hominem attacks so i see no point in further 
> participating in this thread.

I just reviewed what I sent.  I don't think my attacks were ad-hominem.

There are very few people on this list who gracefully accept when they are wrong.  The rest of us try to come grasp at some alternate criticism instead, flailing around attacking minor surrounding issue to save face.[1]

This behavior is unbecoming and frankly embarrassing, but at least it usually
motivates people to review the patches!  And, code being what it is, they find some nit to pick and feel better about their prior mistake.

Unfortunately, this debate *is* stupid because neither of us are really going to do anything about it: ie. it did not cause either of us to review code.  It is also stupid because I know I'm not telling you anything you can't think of yourself.  Finally, it's stupid because it's all been said before in commit messages and in previous posts.

Hope that clarifies,
Rusty.
[1] If anyone cared, I'm sure they could find excruciating lkml archive
    examples of me doing this.

[PATCH] x86: rename global percpu symbol dr7 to cpu_dr7

[Tejun Heo]

Percpu symbols now occupy the same namespace as other global symbols
and as such short global symbols without subsystem prefix tend to
collide with local variables.  dr7 percpu variable used by x86 was hit
by this.  Rename it to cpu_dr7.  The rename also makes it more
consistent with its fellow cpu_debugreg percpu variable.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Ingo Molnar <mingo@elte.hu>
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 arch/x86/include/asm/debugreg.h |    4 ++--
 arch/x86/kernel/hw_breakpoint.c |   10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
index fdabd84..8240f76 100644
--- a/arch/x86/include/asm/debugreg.h
+++ b/arch/x86/include/asm/debugreg.h
@@ -75,7 +75,7 @@
  */
 #ifdef __KERNEL__
 
-DECLARE_PER_CPU(unsigned long, dr7);
+DECLARE_PER_CPU(unsigned long, cpu_dr7);
 
 static inline void hw_breakpoint_disable(void)
 {
@@ -91,7 +91,7 @@ static inline void hw_breakpoint_disable(void)
 
 static inline int hw_breakpoint_active(void)
 {
-	return __get_cpu_var(dr7) & DR_GLOBAL_ENABLE_MASK;
+	return __get_cpu_var(cpu_dr7) & DR_GLOBAL_ENABLE_MASK;
 }
 
 extern void aout_dump_debugregs(struct user *dump);
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 4d267fb..92ea5aa 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -46,8 +46,8 @@
 #include <asm/debugreg.h>
 
 /* Per cpu debug control register value */
-DEFINE_PER_CPU(unsigned long, dr7);
-EXPORT_PER_CPU_SYMBOL(dr7);
+DEFINE_PER_CPU(unsigned long, cpu_dr7);
+EXPORT_PER_CPU_SYMBOL(cpu_dr7);
 
 /* Per cpu debug address registers values */
 static DEFINE_PER_CPU(unsigned long, cpu_debugreg[HBP_NUM]);
@@ -118,7 +118,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 	set_debugreg(info->address, i);
 	__get_cpu_var(cpu_debugreg[i]) = info->address;
 
-	dr7 = &__get_cpu_var(dr7);
+	dr7 = &__get_cpu_var(cpu_dr7);
 	*dr7 |= encode_dr7(i, info->len, info->type);
 
 	set_debugreg(*dr7, 7);
@@ -153,7 +153,7 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
 		return;
 
-	dr7 = &__get_cpu_var(dr7);
+	dr7 = &__get_cpu_var(cpu_dr7);
 	*dr7 &= ~encode_dr7(i, info->len, info->type);
 
 	set_debugreg(*dr7, 7);
@@ -437,7 +437,7 @@ void hw_breakpoint_restore(void)
 	set_debugreg(__get_cpu_var(cpu_debugreg[2]), 2);
 	set_debugreg(__get_cpu_var(cpu_debugreg[3]), 3);
 	set_debugreg(current->thread.debugreg6, 6);
-	set_debugreg(__get_cpu_var(dr7), 7);
+	set_debugreg(__get_cpu_var(cpu_dr7), 7);
 }
 EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
 

linux-next: percpu tree build warning

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

Hi all,

Today's linux-next build (powerpc ppc64_defconfig) produced this warning:

drivers/net/loopback.c: In function 'loopback_get_stats':
drivers/net/loopback.c:109: warning: initialization discards qualifiers from pointer target type

The line in question is:

	lb_stats = per_cpu_ptr(pcpu_lstats, i);

Where "lb_stats" and "pcpu_lstats" are both "const struct pcpu_lstats
*".  I guess this is because of __verify_pcpu_ptr().

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next: percpu tree build warning

[Christoph Lameter]

On Thu, 12 Nov 2009, Stephen Rothwell wrote:

> Hi all,
>
> Today's linux-next build (powerpc ppc64_defconfig) produced this warning:
>
> drivers/net/loopback.c: In function 'loopback_get_stats':
> drivers/net/loopback.c:109: warning: initialization discards qualifiers from pointer target type
>
> The line in question is:
>
> 	lb_stats = per_cpu_ptr(pcpu_lstats, i);
>
> Where "lb_stats" and "pcpu_lstats" are both "const struct pcpu_lstats
> *".  I guess this is because of __verify_pcpu_ptr().

hmm... pcpu_lstats needs to be correctly qualified I guess.