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 [email protected]
http://www.canb.auug.org.au/~sfr/
* Stephen Rothwell <[email protected]> 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
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.
(Cc:-ed Linus and Andrew)
* Rusty Russell <[email protected]> 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
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
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
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 <[email protected]>
Cc: Ingo Molnar <[email protected]>
Reported-by: Stephen Rothwell <[email protected]>
---
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);
* Tejun Heo <[email protected]> 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
Commit-ID: 28b4e0d86acf59ae3bc422921138a4958458326e
Gitweb: http://git.kernel.org/tip/28b4e0d86acf59ae3bc422921138a4958458326e
Author: Tejun Heo <[email protected]>
AuthorDate: Wed, 25 Nov 2009 22:24:44 +0900
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 25 Nov 2009 14:30:04 +0100
x86: Rename global percpu symbol dr7 to cpu_dr7
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 <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Linus Torvalds <[email protected]>,
Cc: Andrew Morton <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Reported-by: Stephen Rothwell <[email protected]>
---
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);
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
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.
* Rusty Russell <[email protected]> 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
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 = ®1; // 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
* Tejun Heo <[email protected]> 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 = ®1; // 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 ®1 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
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 ®1 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
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
On Fri, 27 Nov 2009 04:11:28 pm Ingo Molnar wrote:
>
> * Rusty Russell <[email protected]> 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.
* Rusty Russell <[email protected]> wrote:
> On Fri, 27 Nov 2009 04:11:28 pm Ingo Molnar wrote:
> >
> > * Rusty Russell <[email protected]> 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
On Sun, 29 Nov 2009 05:10:19 pm Ingo Molnar wrote:
> * Rusty Russell <[email protected]> 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.