| Message ID | 20180724192720.32417-7-muriloo@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Modernize unhandled signals message | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | Test checkpatch on branch next |
Hi Murilo, LGTM. Just a comment: On 07/24/2018 04:27 PM, Murilo Opsfelder Araujo wrote: > This adds a human-readable name in the unhandled signal message. > > Before this patch, a page fault looked like: > > Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled signal 11 at 00000000100007d0 nip 000000001000061c lr 00007fff93c55100 code 2 in pandafault[10000000+10000] > > After this patch, a page fault looks like: > > Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault (11) at 000000013a2a09f8 nip 000000013a2a086c lr 00007fffb63e5100 code 2 in pandafault[13a2a0000+10000] I _really_ don't want to bikeshed here, but I vouch for keeping the "unhandled" word before the signal name, like: [...] pandafault[6352]: unhandled segfault (11) at 000000013a2a09f8 nip [...] because the issue reported here is really that we got a segfault _and_ there was no handler to catch it. But feel free to wait for additional comments to decide it. Cheers, Gustavo
Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit : > This adds a human-readable name in the unhandled signal message. > > Before this patch, a page fault looked like: > > Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled > signal 11 at 00000000100007d0 nip 000000001000061c lr > 00007fff93c55100 code 2 in pandafault[10000000+10000] > > After this patch, a page fault looks like: > > Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault > (11) at 000000013a2a09f8 nip 000000013a2a086c lr 00007fffb63e5100 > code 2 in pandafault[13a2a0000+10000] > > Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > --- > arch/powerpc/kernel/traps.c | 43 +++++++++++++++++++++++++++++++++---- > 1 file changed, 39 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index e6c43ef9fb50..e55ee639d010 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler); > #define TM_DEBUG(x...) do { } while(0) > #endif > > +static const char *signames[SIGRTMIN + 1] = { > + "UNKNOWN", > + "SIGHUP", // 1 > + "SIGINT", // 2 > + "SIGQUIT", // 3 > + "SIGILL", // 4 > + "unhandled trap", // 5 = SIGTRAP > + "SIGABRT", // 6 = SIGIOT > + "bus error", // 7 = SIGBUS > + "floating point exception", // 8 = SIGFPE > + "illegal instruction", // 9 = SIGILL > + "SIGUSR1", // 10 > + "segfault", // 11 = SIGSEGV > + "SIGUSR2", // 12 > + "SIGPIPE", // 13 > + "SIGALRM", // 14 > + "SIGTERM", // 15 > + "SIGSTKFLT", // 16 > + "SIGCHLD", // 17 > + "SIGCONT", // 18 > + "SIGSTOP", // 19 > + "SIGTSTP", // 20 > + "SIGTTIN", // 21 > + "SIGTTOU", // 22 > + "SIGURG", // 23 > + "SIGXCPU", // 24 > + "SIGXFSZ", // 25 > + "SIGVTALRM", // 26 > + "SIGPROF", // 27 > + "SIGWINCH", // 28 > + "SIGIO", // 29 = SIGPOLL = SIGLOST > + "SIGPWR", // 30 > + "SIGSYS", // 31 = SIGUNUSED > +}; > + > /* > * Trap & Exception support > */ > @@ -314,10 +349,10 @@ static void show_signal_msg(int signr, struct > pt_regs *regs, int code, > if (!unhandled_signal(current, signr)) > return; > > - pr_info("%s[%d]: unhandled signal %d at "REG_FMT \ > - " nip "REG_FMT" lr "REG_FMT" code %x", > - current->comm, current->pid, signr, addr, > - regs->nip, regs->link, code); > + pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \ > + " lr "REG_FMT" code %x", > + current->comm, current->pid, signames[signr], > + signr, addr, regs->nip, regs->link, code); Are we sure that signr is always within the limits of the table ? Christophe > > print_vma_addr(KERN_CONT " in ", regs->nip); > > -- > 2.17.1
Hi, Gustavo. On Wed, Jul 25, 2018 at 12:19:00PM -0300, Gustavo Romero wrote: > Hi Murilo, > > LGTM. > > Just a comment: > > On 07/24/2018 04:27 PM, Murilo Opsfelder Araujo wrote: > > This adds a human-readable name in the unhandled signal message. > > > > Before this patch, a page fault looked like: > > > > Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled signal 11 at 00000000100007d0 nip 000000001000061c lr 00007fff93c55100 code 2 in pandafault[10000000+10000] > > > > After this patch, a page fault looks like: > > > > Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault (11) at 000000013a2a09f8 nip 000000013a2a086c lr 00007fffb63e5100 code 2 in pandafault[13a2a0000+10000] > > I _really_ don't want to bikeshed here, but I vouch for keeping the > "unhandled" word before the signal name, like: > > [...] pandafault[6352]: unhandled segfault (11) at 000000013a2a09f8 nip [...] > > because the issue reported here is really that we got a segfault _and_ > there was no handler to catch it. Either way works for me. > But feel free to wait for additional comments to decide it. Sure. I intend to wait a couple of weeks to respin the series based on community feedback. Cheers Murilo
Hi, Christophe. On Wed, Jul 25, 2018 at 05:49:27PM +0200, LEROY Christophe wrote: > Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit : > > > This adds a human-readable name in the unhandled signal message. > > > > Before this patch, a page fault looked like: > > > > Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled signal > > 11 at 00000000100007d0 nip 000000001000061c lr 00007fff93c55100 code 2 > > in pandafault[10000000+10000] > > > > After this patch, a page fault looks like: > > > > Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault (11) at > > 000000013a2a09f8 nip 000000013a2a086c lr 00007fffb63e5100 code 2 in > > pandafault[13a2a0000+10000] > > > > Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > > --- > > arch/powerpc/kernel/traps.c | 43 +++++++++++++++++++++++++++++++++---- > > 1 file changed, 39 insertions(+), 4 deletions(-) > > > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > > index e6c43ef9fb50..e55ee639d010 100644 > > --- a/arch/powerpc/kernel/traps.c > > +++ b/arch/powerpc/kernel/traps.c > > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler); > > #define TM_DEBUG(x...) do { } while(0) > > #endif > > > > +static const char *signames[SIGRTMIN + 1] = { > > + "UNKNOWN", > > + "SIGHUP", // 1 > > + "SIGINT", // 2 > > + "SIGQUIT", // 3 > > + "SIGILL", // 4 > > + "unhandled trap", // 5 = SIGTRAP > > + "SIGABRT", // 6 = SIGIOT > > + "bus error", // 7 = SIGBUS > > + "floating point exception", // 8 = SIGFPE > > + "illegal instruction", // 9 = SIGILL > > + "SIGUSR1", // 10 > > + "segfault", // 11 = SIGSEGV > > + "SIGUSR2", // 12 > > + "SIGPIPE", // 13 > > + "SIGALRM", // 14 > > + "SIGTERM", // 15 > > + "SIGSTKFLT", // 16 > > + "SIGCHLD", // 17 > > + "SIGCONT", // 18 > > + "SIGSTOP", // 19 > > + "SIGTSTP", // 20 > > + "SIGTTIN", // 21 > > + "SIGTTOU", // 22 > > + "SIGURG", // 23 > > + "SIGXCPU", // 24 > > + "SIGXFSZ", // 25 > > + "SIGVTALRM", // 26 > > + "SIGPROF", // 27 > > + "SIGWINCH", // 28 > > + "SIGIO", // 29 = SIGPOLL = SIGLOST > > + "SIGPWR", // 30 > > + "SIGSYS", // 31 = SIGUNUSED > > +}; > > + > > /* > > * Trap & Exception support > > */ > > @@ -314,10 +349,10 @@ static void show_signal_msg(int signr, struct > > pt_regs *regs, int code, > > if (!unhandled_signal(current, signr)) > > return; > > > > - pr_info("%s[%d]: unhandled signal %d at "REG_FMT \ > > - " nip "REG_FMT" lr "REG_FMT" code %x", > > - current->comm, current->pid, signr, addr, > > - regs->nip, regs->link, code); > > + pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \ > > + " lr "REG_FMT" code %x", > > + current->comm, current->pid, signames[signr], > > + signr, addr, regs->nip, regs->link, code); > > Are we sure that signr is always within the limits of the table ? Looking at the code, we only pass the following signals: SIGBUS SIGFPE SIGILL SIGSEGV SIGTRAP All of them are within the limits of the table. We've added other signals for completeness. Cheers Murilo
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index e6c43ef9fb50..e55ee639d010 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler); #define TM_DEBUG(x...) do { } while(0) #endif +static const char *signames[SIGRTMIN + 1] = { + "UNKNOWN", + "SIGHUP", // 1 + "SIGINT", // 2 + "SIGQUIT", // 3 + "SIGILL", // 4 + "unhandled trap", // 5 = SIGTRAP + "SIGABRT", // 6 = SIGIOT + "bus error", // 7 = SIGBUS + "floating point exception", // 8 = SIGFPE + "illegal instruction", // 9 = SIGILL + "SIGUSR1", // 10 + "segfault", // 11 = SIGSEGV + "SIGUSR2", // 12 + "SIGPIPE", // 13 + "SIGALRM", // 14 + "SIGTERM", // 15 + "SIGSTKFLT", // 16 + "SIGCHLD", // 17 + "SIGCONT", // 18 + "SIGSTOP", // 19 + "SIGTSTP", // 20 + "SIGTTIN", // 21 + "SIGTTOU", // 22 + "SIGURG", // 23 + "SIGXCPU", // 24 + "SIGXFSZ", // 25 + "SIGVTALRM", // 26 + "SIGPROF", // 27 + "SIGWINCH", // 28 + "SIGIO", // 29 = SIGPOLL = SIGLOST + "SIGPWR", // 30 + "SIGSYS", // 31 = SIGUNUSED +}; + /* * Trap & Exception support */ @@ -314,10 +349,10 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code, if (!unhandled_signal(current, signr)) return; - pr_info("%s[%d]: unhandled signal %d at "REG_FMT \ - " nip "REG_FMT" lr "REG_FMT" code %x", - current->comm, current->pid, signr, addr, - regs->nip, regs->link, code); + pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \ + " lr "REG_FMT" code %x", + current->comm, current->pid, signames[signr], + signr, addr, regs->nip, regs->link, code); print_vma_addr(KERN_CONT " in ", regs->nip);
This adds a human-readable name in the unhandled signal message. Before this patch, a page fault looked like: Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled signal 11 at 00000000100007d0 nip 000000001000061c lr 00007fff93c55100 code 2 in pandafault[10000000+10000] After this patch, a page fault looks like: Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault (11) at 000000013a2a09f8 nip 000000013a2a086c lr 00007fffb63e5100 code 2 in pandafault[13a2a0000+10000] Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> --- arch/powerpc/kernel/traps.c | 43 +++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-)