Message ID | 20220923154143.1115645-2-npiggin@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [RFC,1/2] powerpc: Don't use extable for WARN | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
Le 23/09/2022 à 17:41, Nicholas Piggin a écrit : > WARN_ONCE and similar are often used in frequently executed code, and > should not crash the system. The program check interrupt caused by > WARN_ON_ONCE can be a significant overhead even when nothing is being > printed. This can cause performance to become unacceptable, having the > same effective impact to the user as a BUG_ON(). > > Avoid this overhead by patching the trap with a nop instruction after a > "once" trap fires. Conditional warnings that return a result must have > equivalent compare and branch instructions after the trap, so when it is > nopped the statement will behave the same way. It's possible the asm > goto should be removed entirely and this comparison just done in C now. You mean, just like PPC32 ? (Since db87a7199229 ("powerpc/bug: Remove specific powerpc BUG_ON() and WARN_ON() on PPC32")) But I'm having hard time with your change. You change only WARN_ON() But WARN_ON_ONCE() calls __WARN_FLAGS() And WARN_ONCE() calls WARN() via DO_ONCE_LITE_IF() So I don't see any ..._ONCE something going with WARN_ON(). Am I missing something ? Otherwise, what you want to do is to patch the 'twi' in __WARN_FLAGS() by a non conditional branch to __label_warn_on . Christophe
From: Nicholas Piggin > Sent: 23 September 2022 16:42 > > WARN_ONCE and similar are often used in frequently executed code, and > should not crash the system. The program check interrupt caused by > WARN_ON_ONCE can be a significant overhead even when nothing is being > printed. This can cause performance to become unacceptable, having the > same effective impact to the user as a BUG_ON(). > > Avoid this overhead by patching the trap with a nop instruction after a > "once" trap fires. Conditional warnings that return a result must have > equivalent compare and branch instructions after the trap, so when it is > nopped the statement will behave the same way. It's possible the asm > goto should be removed entirely and this comparison just done in C now. > > XXX: possibly this should schedule the patching to run in a different > context than the program check. I'm pretty sure WARN_ON_ONCE() is valid everywhere printk() is allowed. In many cases this means you can't call mutex_enter(). So you need a different scheme. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat Sep 24, 2022 at 2:47 AM AEST, Christophe Leroy wrote: > > > Le 23/09/2022 à 17:41, Nicholas Piggin a écrit : > > WARN_ONCE and similar are often used in frequently executed code, and > > should not crash the system. The program check interrupt caused by > > WARN_ON_ONCE can be a significant overhead even when nothing is being > > printed. This can cause performance to become unacceptable, having the > > same effective impact to the user as a BUG_ON(). > > > > Avoid this overhead by patching the trap with a nop instruction after a > > "once" trap fires. Conditional warnings that return a result must have > > equivalent compare and branch instructions after the trap, so when it is > > nopped the statement will behave the same way. It's possible the asm > > goto should be removed entirely and this comparison just done in C now. > > You mean, just like PPC32 ? (Since db87a7199229 ("powerpc/bug: Remove > specific powerpc BUG_ON() and WARN_ON() on PPC32")) > > But I'm having hard time with your change. > > You change only WARN_ON() > But WARN_ON_ONCE() calls __WARN_FLAGS() > And WARN_ONCE() calls WARN() via DO_ONCE_LITE_IF() > > So I don't see any ..._ONCE something going with WARN_ON(). > > Am I missing something ? Hmm, no I must have missed something. I guess it is the EMIT_WARN_ENTRY in asm which is the main problem I've seen. Although we could remove the DO_ONCE_LITE_IF code generation from our WARN_ON_ONCE as well if we did this patching. Thanks, Nick
Le 04/10/2022 à 06:31, Nicholas Piggin a écrit : > On Sat Sep 24, 2022 at 2:47 AM AEST, Christophe Leroy wrote: >> >> >> Le 23/09/2022 à 17:41, Nicholas Piggin a écrit : >>> WARN_ONCE and similar are often used in frequently executed code, and >>> should not crash the system. The program check interrupt caused by >>> WARN_ON_ONCE can be a significant overhead even when nothing is being >>> printed. This can cause performance to become unacceptable, having the >>> same effective impact to the user as a BUG_ON(). >>> >>> Avoid this overhead by patching the trap with a nop instruction after a >>> "once" trap fires. Conditional warnings that return a result must have >>> equivalent compare and branch instructions after the trap, so when it is >>> nopped the statement will behave the same way. It's possible the asm >>> goto should be removed entirely and this comparison just done in C now. >> >> You mean, just like PPC32 ? (Since db87a7199229 ("powerpc/bug: Remove >> specific powerpc BUG_ON() and WARN_ON() on PPC32")) >> >> But I'm having hard time with your change. >> >> You change only WARN_ON() >> But WARN_ON_ONCE() calls __WARN_FLAGS() >> And WARN_ONCE() calls WARN() via DO_ONCE_LITE_IF() >> >> So I don't see any ..._ONCE something going with WARN_ON(). >> >> Am I missing something ? > > Hmm, no I must have missed something. I guess it is the EMIT_WARN_ENTRY > in asm which is the main problem I've seen. Although we could remove the > DO_ONCE_LITE_IF code generation from our WARN_ON_ONCE as well if we did > this patching. > Yes, I guess having now the recovery address in the bug table instead of the extable is rather more efficient. Maybe DO_ONCE_LITE could be replaced by DO_ONCE which uses jump_label ? Not sure it is worth a specific patching implementation, is it ? Christophe
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index dec73137fea0..0ea7b9fe0305 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -14,6 +14,7 @@ .macro __EMIT_BUG_ENTRY addr,file,line,flags .section __bug_table,"aw" 5001: .4byte \addr - . + .4byte 0 .4byte 0 .4byte 5002f - . .short \line, \flags @@ -27,6 +28,7 @@ .macro __EMIT_BUG_ENTRY addr,file,line,flags .section __bug_table,"aw" 5001: .4byte \addr - . + .4byte 0 .4byte 0 .short \flags .org 5001b+BUG_ENTRY_SIZE @@ -48,6 +50,7 @@ #else /* !__ASSEMBLY__ */ struct arch_bug_entry { signed int recovery_addr_disp; + unsigned int insn; }; /* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3 to be FILE, LINE, flags and @@ -57,6 +60,7 @@ struct arch_bug_entry { ".section __bug_table,\"aw\"\n" \ "2: .4byte 1b - .\n" \ " .4byte 0\n" \ + " .4byte 0\n" \ " .4byte %0 - .\n" \ " .short %1, %2\n" \ ".org 2b+%3\n" \ @@ -66,6 +70,7 @@ struct arch_bug_entry { ".section __bug_table,\"aw\"\n" \ "2: .4byte 1b - .\n" \ " .4byte 0\n" \ + " .4byte 0\n" \ " .short %2\n" \ ".org 2b+%3\n" \ ".previous\n" @@ -76,6 +81,7 @@ struct arch_bug_entry { ".section __bug_table,\"aw\"\n" \ "2: .4byte 1b - .\n" \ " .4byte 1b - %l[" #label "]\n" \ + " .4byte 0\n" \ " .4byte %0 - .\n" \ " .short %1, %2\n" \ ".org 2b+%3\n" \ @@ -85,6 +91,7 @@ struct arch_bug_entry { ".section __bug_table,\"aw\"\n" \ "2: .4byte 1b - .\n" \ " .4byte 1b - %l[" #label "]\n" \ + " .4byte 0\n" \ " .short %2\n" \ ".org 2b+%3\n" \ ".previous\n" @@ -106,7 +113,7 @@ struct arch_bug_entry { : : "i" (__FILE__), "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry)), \ - ##__VA_ARGS__ : : label) + ##__VA_ARGS__ : "cr0" : label) /* * BUG_ON() and WARN_ON() do their best to cooperate with compile-time @@ -151,7 +158,7 @@ __label_warn_on: \ } else { \ __label__ __label_warn_on; \ \ - WARN_ENTRY(PPC_TLNEI " %4, 0", \ + WARN_ENTRY(PPC_TLNEI " %4, 0 ; cmpdi %4, 0 ; bne %l[__label_warn_on]", \ BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ __label_warn_on, \ "r" ((__force long)(x))); \ @@ -178,6 +185,9 @@ __label_warn_on: \ #define _EMIT_BUG_ENTRY #define _EMIT_WARN_ENTRY #endif +#define arch_generic_bug_entry_clear_once +void arch_generic_bug_entry_clear_once(struct bug_entry *bug); + #endif /* CONFIG_BUG */ #include <asm-generic/bug.h> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index 1e521a432bd0..14137c17b9cd 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -1458,6 +1458,44 @@ static int emulate_math(struct pt_regs *regs) static inline int emulate_math(struct pt_regs *regs) { return -1; } #endif +static DEFINE_MUTEX(bug_lock); + +void arch_generic_bug_entry_clear_once(struct bug_entry *bug) +{ + unsigned long bugaddr = bug_addr(bug); + + BUG_ON(!bug->arch.insn); + + mutex_lock(&bug_lock); + if (bug->arch.insn == 0) { + mutex_unlock(&bug_lock); + return; + } + patch_instruction((u32 *)bugaddr, ppc_inst(bug->arch.insn)); + bug->arch.insn = 0; + mutex_unlock(&bug_lock); +} + +static void bug_entry_done_once(struct bug_entry *bug) +{ + unsigned long bugaddr = bug_addr(bug); + unsigned int insn; + + if (!mutex_trylock(&bug_lock)) + return; + + if (bug->arch.insn != 0) + goto out; + + if (__get_user(insn, (unsigned int __user *)bugaddr)) + goto out; + + patch_instruction((u32 *)bugaddr, ppc_inst(PPC_RAW_NOP())); + +out: + mutex_unlock(&bug_lock); +} + static void do_program_check(struct pt_regs *regs) { unsigned int reason = get_reason(regs); @@ -1494,7 +1532,7 @@ static void do_program_check(struct pt_regs *regs) if (!(regs->msr & MSR_PR) && /* not user-mode */ report_bug(bugaddr, regs) == BUG_TRAP_TYPE_WARN) { - const struct bug_entry *bug; + struct bug_entry *bug; unsigned long recov; bug = find_bug(bugaddr); @@ -1502,6 +1540,8 @@ static void do_program_check(struct pt_regs *regs) recov = regs->nip + 4; else recov = bugaddr - bug->arch.recovery_addr_disp; + if (bug && (bug->flags & BUGFLAG_ONCE)) + bug_entry_done_once(bug); regs_set_return_ip(regs, recov); return; diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 1ce8431bdf02..a0cb717998c1 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -49,6 +49,15 @@ struct bug_entry { #endif unsigned short flags; }; + +static inline unsigned long bug_addr(const struct bug_entry *bug) +{ +#ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS + return (unsigned long)&bug->bug_addr_disp + bug->bug_addr_disp; +#else + return bug->bug_addr; +#endif +} #endif /* CONFIG_GENERIC_BUG */ /* diff --git a/include/linux/bug.h b/include/linux/bug.h index 348acf2558f3..a207445e5b5f 100644 --- a/include/linux/bug.h +++ b/include/linux/bug.h @@ -46,6 +46,7 @@ enum bug_trap_type report_bug(unsigned long bug_addr, struct pt_regs *regs); /* These are defined by the architecture */ int is_valid_bugaddr(unsigned long addr); +void __generic_bug_clear_once(void); void generic_bug_clear_once(void); #else /* !CONFIG_GENERIC_BUG */ diff --git a/lib/bug.c b/lib/bug.c index c223a2575b72..d1cc4f763679 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -50,15 +50,6 @@ extern struct bug_entry __start___bug_table[], __stop___bug_table[]; -static inline unsigned long bug_addr(const struct bug_entry *bug) -{ -#ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS - return (unsigned long)&bug->bug_addr_disp + bug->bug_addr_disp; -#else - return bug->bug_addr; -#endif -} - #ifdef CONFIG_MODULES /* Updates are protected by module mutex */ static LIST_HEAD(module_bug_list); @@ -209,12 +200,21 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) return BUG_TRAP_TYPE_BUG; } +#ifndef arch_generic_bug_entry_clear_once +#define arch_generic_bug_entry_clear_once(bug) +#endif + static void clear_once_table(struct bug_entry *start, struct bug_entry *end) { struct bug_entry *bug; - for (bug = start; bug < end; bug++) - bug->flags &= ~BUGFLAG_DONE; + for (bug = start; bug < end; bug++) { + bool triggered = bug->flags & BUGFLAG_DONE; + if (triggered) { + bug->flags &= ~BUGFLAG_DONE; + arch_generic_bug_entry_clear_once(bug); + } + } } void generic_bug_clear_once(void)