Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
On Thu, 2010-04-15 at 19:15 +0200, Frederic Weisbecker wrote: that looks rather ugly. Why not do a raw: this_cpu_inc(lockdep_stats.redundant_hardirqs_on); which basically open-codes debug_atomic_inc(), but without the warning? Because that would open a race against interrupts that might touch lockdep_stats.redundant_hardirqs_on too. How so, its a pure per-cpu variable right? so either the increment happens before the interrupts hits, or after, in either case there should not be a race with interrupts. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
On Fri, Apr 16, 2010 at 12:38:43PM +0200, Peter Zijlstra wrote: On Thu, 2010-04-15 at 19:15 +0200, Frederic Weisbecker wrote: that looks rather ugly. Why not do a raw: this_cpu_inc(lockdep_stats.redundant_hardirqs_on); which basically open-codes debug_atomic_inc(), but without the warning? Because that would open a race against interrupts that might touch lockdep_stats.redundant_hardirqs_on too. How so, its a pure per-cpu variable right? so either the increment happens before the interrupts hits, or after, in either case there should not be a race with interrupts. In x86 yeah, I guess the compiler simply loads the address and does an inc directly, which is atomic wrt interrupts. But what about another arch that would need an intermediate load of the value: load val, reg add reg, 1 ---interrupt here store reg, val ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
From: Ingo Molnar mi...@elte.hu Date: Thu, 15 Apr 2010 08:49:40 +0200 Btw., WARN_ON trapping on PowerPC is clearly a PowerPC bug - there's a good reason we have WARN_ON versus BUG_ON - it should be fixed. I disagree, an implementation should be allowed to use the most efficient implementation possible for both interfaces. I would be using traps for both on sparc64 if that were really feasible on sparc64 (and actually with gcc-4.5's asm goto it might actually be now) The WARN and BUG macros, when implemented without traps, have serious implications for overall code size and register pressure. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
* David Miller da...@davemloft.net wrote: From: Ingo Molnar mi...@elte.hu Date: Thu, 15 Apr 2010 08:49:40 +0200 Btw., WARN_ON trapping on PowerPC is clearly a PowerPC bug - there's a good reason we have WARN_ON versus BUG_ON - it should be fixed. I disagree, an implementation should be allowed to use the most efficient implementation possible for both interfaces. It trades robustness for slightly better space/code efficiency. Such a trap based mechanism exists on x86 as well and we use it for BUG_ON(). We intentionally dont use it to generate warnings and dont override __WARN(), because it would blow up way too often when a warning triggers in some sensitive codepath that cannot take a trap. Anyway, the warning obviously has to be fixed - but the boot crash itself is PowerPC's own doing. Ingo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
* Stephen Rothwell s...@canb.auug.org.au wrote:
Hi all,
Yesterday's (and today's) linux-next boot (PowerPC) failed like this:
[ cut here ]
Badness at kernel/lockdep.c:2301
NIP: c00a35c8 LR: c00084c4 CTR:
REGS: c0bf77e0 TRAP: 0700 Not tainted (2.6.34-rc4-autokern1)
MSR: 80021032 ME,CE,IR,DR CR: 2444 XER: 0004
TASK = c0aa3d30[0] 'swapper' THREAD: c0bf4000 CPU: 0
GPR00: 0001 c0bf7a60 c0bf32f0 c00084c4
GPR04: 0a00 0068
GPR08: 0008 c0c4fabe 7265677368657265
GPR12: 80009032 c7691000 01c0 c0770bf8
GPR16: c076f390 0043 024876f0
GPR20: c0887480 02487480 c08876f0 01b5f8d0
GPR24: c0770478 0330 c0c1f1c8 c0884610
GPR28: c0c1b290 c00084c4 c0b45068 c0aa3d30
NIP [c00a35c8] .trace_hardirqs_on_caller+0xb0/0x224
LR [c00084c4] system_call_common+0xc4/0x114
Call Trace:
[c0bf7a60] [c0bf7ba0] init_thread_union+0x3ba0/0x4000
(unreliable)
[c0bf7af0] [c00084c4] system_call_common+0xc4/0x114
--- Exception: c01 at .kernel_thread+0x28/0x70
LR = .rest_init+0x34/0xf8
[c0bf7de0] [c086916c] .proc_sys_init+0x20/0x64 (unreliable)
[c0bf7e50] [c00099c0] .rest_init+0x20/0xf8
[c0bf7ee0] [c0848af0] .start_kernel+0x484/0x4a8
[c0bf7f90] [c00083c0] .start_here_common+0x1c/0x5c
Instruction dump:
409e0188 0fe0 48000180 801f08d8 2f80 41be0050 880d01da 2fa0
41be0028 e93e8538 8809 6801 0b00 2fa0 41be0010 e93e8538
[ cut here ]
Caused by commit bd6d29c25bb1a24a4c160ec5de43e0004e01f72b (lockstat:
Make lockstat counting per cpu). This added a WARN_ON_ONCE to
debug_atomic_inc() which is called from trace_hardirqs_on_caller() with
irqs enabled.
Line 2301 is:
if (unlikely(curr-hardirqs_enabled)) {
debug_atomic_inc(redundant_hardirqs_on); --- 2301
return;
}
This is especially bad since on PowerPC, WARN_ON is a TRAP and the return
path from the TRAP also calls trace_hardirqs_on_caller(), so the TRAP
recurses ...
Ok, we'll fix the warning.
Btw., WARN_ON trapping on PowerPC is clearly a PowerPC bug - there's a good
reason we have WARN_ON versus BUG_ON - it should be fixed.
Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
Hi Ingo, On Thu, 15 Apr 2010 08:49:40 +0200 Ingo Molnar mi...@elte.hu wrote: Ok, we'll fix the warning. Thanks. -- Cheers, Stephen Rothwells...@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ pgpGux6Rr8FG8.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
On Thu, Apr 15, 2010 at 08:49:40AM +0200, Ingo Molnar wrote:
* Stephen Rothwell s...@canb.auug.org.au wrote:
Hi all,
Yesterday's (and today's) linux-next boot (PowerPC) failed like this:
[ cut here ]
Badness at kernel/lockdep.c:2301
NIP: c00a35c8 LR: c00084c4 CTR:
REGS: c0bf77e0 TRAP: 0700 Not tainted (2.6.34-rc4-autokern1)
MSR: 80021032 ME,CE,IR,DR CR: 2444 XER: 0004
TASK = c0aa3d30[0] 'swapper' THREAD: c0bf4000 CPU: 0
GPR00: 0001 c0bf7a60 c0bf32f0 c00084c4
GPR04: 0a00 0068
GPR08: 0008 c0c4fabe 7265677368657265
GPR12: 80009032 c7691000 01c0 c0770bf8
GPR16: c076f390 0043 024876f0
GPR20: c0887480 02487480 c08876f0 01b5f8d0
GPR24: c0770478 0330 c0c1f1c8 c0884610
GPR28: c0c1b290 c00084c4 c0b45068 c0aa3d30
NIP [c00a35c8] .trace_hardirqs_on_caller+0xb0/0x224
LR [c00084c4] system_call_common+0xc4/0x114
Call Trace:
[c0bf7a60] [c0bf7ba0] init_thread_union+0x3ba0/0x4000
(unreliable)
[c0bf7af0] [c00084c4] system_call_common+0xc4/0x114
--- Exception: c01 at .kernel_thread+0x28/0x70
LR = .rest_init+0x34/0xf8
[c0bf7de0] [c086916c] .proc_sys_init+0x20/0x64 (unreliable)
[c0bf7e50] [c00099c0] .rest_init+0x20/0xf8
[c0bf7ee0] [c0848af0] .start_kernel+0x484/0x4a8
[c0bf7f90] [c00083c0] .start_here_common+0x1c/0x5c
Instruction dump:
409e0188 0fe0 48000180 801f08d8 2f80 41be0050 880d01da 2fa0
41be0028 e93e8538 8809 6801 0b00 2fa0 41be0010 e93e8538
[ cut here ]
Caused by commit bd6d29c25bb1a24a4c160ec5de43e0004e01f72b (lockstat:
Make lockstat counting per cpu). This added a WARN_ON_ONCE to
debug_atomic_inc() which is called from trace_hardirqs_on_caller() with
irqs enabled.
Line 2301 is:
if (unlikely(curr-hardirqs_enabled)) {
debug_atomic_inc(redundant_hardirqs_on); --- 2301
return;
}
This is especially bad since on PowerPC, WARN_ON is a TRAP and the return
path from the TRAP also calls trace_hardirqs_on_caller(), so the TRAP
recurses ...
Ok, we'll fix the warning.
Btw., WARN_ON trapping on PowerPC is clearly a PowerPC bug - there's a good
reason we have WARN_ON versus BUG_ON - it should be fixed.
In this case, I guess the following fix should be sufficient?
I'm going to test it and provide a sane changelog.
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 78325f8..65d4336 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2298,7 +2298,11 @@ void trace_hardirqs_on_caller(unsigned long ip)
return;
if (unlikely(curr-hardirqs_enabled)) {
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
debug_atomic_inc(redundant_hardirqs_on);
+ raw_local_irq_restore(flags);
return;
}
/* we'll do an OFF - ON transition: */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
* Frederic Weisbecker fweis...@gmail.com wrote:
On Thu, Apr 15, 2010 at 08:49:40AM +0200, Ingo Molnar wrote:
* Stephen Rothwell s...@canb.auug.org.au wrote:
Hi all,
Yesterday's (and today's) linux-next boot (PowerPC) failed like this:
[ cut here ]
Badness at kernel/lockdep.c:2301
NIP: c00a35c8 LR: c00084c4 CTR:
REGS: c0bf77e0 TRAP: 0700 Not tainted (2.6.34-rc4-autokern1)
MSR: 80021032 ME,CE,IR,DR CR: 2444 XER: 0004
TASK = c0aa3d30[0] 'swapper' THREAD: c0bf4000 CPU: 0
GPR00: 0001 c0bf7a60 c0bf32f0
c00084c4
GPR04: 0a00
0068
GPR08: 0008 c0c4fabe
7265677368657265
GPR12: 80009032 c7691000 01c0
c0770bf8
GPR16: c076f390 0043
024876f0
GPR20: c0887480 02487480 c08876f0
01b5f8d0
GPR24: c0770478 0330 c0c1f1c8
c0884610
GPR28: c0c1b290 c00084c4 c0b45068
c0aa3d30
NIP [c00a35c8] .trace_hardirqs_on_caller+0xb0/0x224
LR [c00084c4] system_call_common+0xc4/0x114
Call Trace:
[c0bf7a60] [c0bf7ba0] init_thread_union+0x3ba0/0x4000
(unreliable)
[c0bf7af0] [c00084c4] system_call_common+0xc4/0x114
--- Exception: c01 at .kernel_thread+0x28/0x70
LR = .rest_init+0x34/0xf8
[c0bf7de0] [c086916c] .proc_sys_init+0x20/0x64
(unreliable)
[c0bf7e50] [c00099c0] .rest_init+0x20/0xf8
[c0bf7ee0] [c0848af0] .start_kernel+0x484/0x4a8
[c0bf7f90] [c00083c0] .start_here_common+0x1c/0x5c
Instruction dump:
409e0188 0fe0 48000180 801f08d8 2f80 41be0050 880d01da 2fa0
41be0028 e93e8538 8809 6801 0b00 2fa0 41be0010 e93e8538
[ cut here ]
Caused by commit bd6d29c25bb1a24a4c160ec5de43e0004e01f72b (lockstat:
Make lockstat counting per cpu). This added a WARN_ON_ONCE to
debug_atomic_inc() which is called from trace_hardirqs_on_caller() with
irqs enabled.
Line 2301 is:
if (unlikely(curr-hardirqs_enabled)) {
debug_atomic_inc(redundant_hardirqs_on); --- 2301
return;
}
This is especially bad since on PowerPC, WARN_ON is a TRAP and the return
path from the TRAP also calls trace_hardirqs_on_caller(), so the TRAP
recurses ...
Ok, we'll fix the warning.
Btw., WARN_ON trapping on PowerPC is clearly a PowerPC bug - there's a good
reason we have WARN_ON versus BUG_ON - it should be fixed.
In this case, I guess the following fix should be sufficient?
I'm going to test it and provide a sane changelog.
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 78325f8..65d4336 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2298,7 +2298,11 @@ void trace_hardirqs_on_caller(unsigned long ip)
return;
if (unlikely(curr-hardirqs_enabled)) {
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
debug_atomic_inc(redundant_hardirqs_on);
+ raw_local_irq_restore(flags);
return;
}
/* we'll do an OFF - ON transition: */
that looks rather ugly. Why not do a raw:
this_cpu_inc(lockdep_stats.redundant_hardirqs_on);
which basically open-codes debug_atomic_inc(), but without the warning?
Btw., using the this_cpu() methods might result in faster code for all the
debug_atomic_inc() macros as well?
Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
On Thu, Apr 15, 2010 at 04:03:58PM +0200, Ingo Molnar wrote:
In this case, I guess the following fix should be sufficient?
I'm going to test it and provide a sane changelog.
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 78325f8..65d4336 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2298,7 +2298,11 @@ void trace_hardirqs_on_caller(unsigned long ip)
return;
if (unlikely(curr-hardirqs_enabled)) {
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
debug_atomic_inc(redundant_hardirqs_on);
+ raw_local_irq_restore(flags);
return;
}
/* we'll do an OFF - ON transition: */
that looks rather ugly. Why not do a raw:
this_cpu_inc(lockdep_stats.redundant_hardirqs_on);
which basically open-codes debug_atomic_inc(), but without the warning?
Because that would open a race against interrupts that might
touch lockdep_stats.redundant_hardirqs_on too.
If you think it's not very important (this race must be pretty rare I guess),
I can use your solution.
Btw., using the this_cpu() methods might result in faster code for all the
debug_atomic_inc() macros as well?
Indeed, will change that too.
Thanks.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
On Thu, Apr 15, 2010 at 04:03:58PM +0200, Ingo Molnar wrote:
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 78325f8..65d4336 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2298,7 +2298,11 @@ void trace_hardirqs_on_caller(unsigned long ip)
return;
if (unlikely(curr-hardirqs_enabled)) {
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
debug_atomic_inc(redundant_hardirqs_on);
+ raw_local_irq_restore(flags);
return;
}
/* we'll do an OFF - ON transition: */
that looks rather ugly. Why not do a raw:
this_cpu_inc(lockdep_stats.redundant_hardirqs_on);
which basically open-codes debug_atomic_inc(), but without the warning?
There is also no guarantee we are in a non-preemptable section. We can then
also race against another cpu.
I'm not sure what to do.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
* Frederic Weisbecker fweis...@gmail.com wrote:
On Thu, Apr 15, 2010 at 04:03:58PM +0200, Ingo Molnar wrote:
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 78325f8..65d4336 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2298,7 +2298,11 @@ void trace_hardirqs_on_caller(unsigned long ip)
return;
if (unlikely(curr-hardirqs_enabled)) {
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
debug_atomic_inc(redundant_hardirqs_on);
+ raw_local_irq_restore(flags);
return;
}
/* we'll do an OFF - ON transition: */
that looks rather ugly. Why not do a raw:
this_cpu_inc(lockdep_stats.redundant_hardirqs_on);
which basically open-codes debug_atomic_inc(), but without the warning?
There is also no guarantee we are in a non-preemptable section. We can then
also race against another cpu.
I'm not sure what to do.
it's a statistics counter so worst-case we lose a count. It's not a real issue
- but might be worth adding a comment.
Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
On Thu, 2010-04-15 at 09:32 +0200, Ingo Molnar wrote: It trades robustness for slightly better space/code efficiency. Such a trap based mechanism exists on x86 as well and we use it for BUG_ON(). We intentionally dont use it to generate warnings and dont override __WARN(), because it would blow up way too often when a warning triggers in some sensitive codepath that cannot take a trap. Anyway, the warning obviously has to be fixed - but the boot crash itself is PowerPC's own doing. Well, yes and no, as I explained in a separate branch of that thread. We indeed can't cope with a WARN in that spot because it goes recursive. Now the reason we have this double-enable is due afaik to the way I implemented IRQ trace, because things like syscalls basically force-enable IRQs on powerpc and I don't necessarily have tracking informations in the exception return path of what the old value was. I need to double check what the exact scenario here is and whether I can fix it but it's one of those cases where what lockdep is warning about isn't actually an error I believe. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: PowerPC WARN_ON_ONCE() after merge of the final tree (tip related)
On Wed, 2010-04-14 at 23:55 -0700, David Miller wrote: From: Ingo Molnar mi...@elte.hu Date: Thu, 15 Apr 2010 08:49:40 +0200 Btw., WARN_ON trapping on PowerPC is clearly a PowerPC bug - there's a good reason we have WARN_ON versus BUG_ON - it should be fixed. I disagree, an implementation should be allowed to use the most efficient implementation possible for both interfaces. Right, and I don't think the reason why we have WARN_ON vs. BUG_ON ever had anything to do with whether it's implemented with a trap or not :-) It's purely related to whether it's supposed to be fatal or not. Now, there is indeed the potential problem you mention of WARN_ON being called in places where such a trap is unsafe, but so far, this is the first time I can remember we hit that problem so we've been getting away with it for quite a while :-) Now, whether the trap is or is not more efficient than an explicit test is something that is still being debated on powerpc. It has the advantage of not un-leafing functions (and thus not creating stack frames, adding register reloads, etc... when not needed), basically putting the burden of saving/restoring registers to the (hopefully) rare path of actually taking the WARN/BUG. We could probably manufacture something similar with careful use of inline asm and an out of line asm trampoline. The benefit of the trap instruction vs. conditional branches per-se is probably nil. It's really more about the codegen impact, register clobber due to the added function call, etc.. at least for us. Cheers, Ben. I would be using traps for both on sparc64 if that were really feasible on sparc64 (and actually with gcc-4.5's asm goto it might actually be now) The WARN and BUG macros, when implemented without traps, have serious implications for overall code size and register pressure. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
