RE: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()
> -Original Message- > From: Michael Ellerman > Sent: Tuesday, 31 July 2018 7:32 PM > To: Murilo Opsfelder Araujo ; LEROY Christophe > > Cc: linux-ker...@vger.kernel.org; Alastair D'Silva ; > Andrew Donnellan ; Balbir Singh > ; Benjamin Herrenschmidt > ; Cyril Bur ; Eric W . > Biederman ; Joe Perches ; > Michael Neuling ; Nicholas Piggin > ; Paul Mackerras ; Simon Guo > ; Sukadev Bhattiprolu > ; Tobin C . Harding ; linuxppc- > d...@lists.ozlabs.org > Subject: Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in > show_signal_msg() > > Murilo Opsfelder Araujo writes: > > On Mon, Jul 30, 2018 at 06:30:47PM +0200, LEROY Christophe wrote: > >> Murilo Opsfelder Araujo a écrit : > >> > On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote: > >> > > Murilo Opsfelder Araujo a écrit : > >> > > > >> > > > Simplify the message format by using REG_FMT as the register > >> > > > format. This avoids having two different formats and avoids > checking for MSR_64BIT. > >> > > > >> > > Are you sure it is what we want ? > >> > > >> > Yes. > >> > > >> > > Won't it change the behaviour for a 32 bits app running on a 64bits > kernel ? > >> > > >> > In fact, this changes how many zeroes are prefixed when displaying > >> > the registers (%016lx vs. %08lx format). For example, 32-bits > >> > userspace, 64-bits kernel: > >> > >> Indeed that's what I suspected. What is the real benefit of this change ? > >> Why not keep the current format for 32bits userspace ? All those > >> leading zeroes are pointless to me. > > > > One of the benefits is simplifying the code by removing some checks. > > Another is deduplicating almost identical format strings in favor of a > > unified > one. > > > > After reading Joe's comment [1], %px seems to be the format we're > looking for. > > An extract from Documentation/core-api/printk-formats.rst: > > > > "%px is functionally equivalent to %lx (or %lu). %px is preferred because > > it > > is more uniquely grep'able." > > > > So I guess we don't need to worry about the format (%016lx vs. %08lx), > > let's just use %px, as per the guideline. > > I don't think I like %px. Me neither, semantically, it's for pointers, and the data being displayed is not a pointer. > It makes the format string cleaner, but it means we have to cast everything > to void * which is ugly as heck. > > I actually don't think the leading zeroes are helpful at all in the signal > message, ie. we should just use %lx there. > > They are useful in show_regs() because we want everything to line up. > > So I think I'll drop patch 3 and use 0x%lx in show_signal_msg(), meaning we > end up with, eg: > > [ 73.414535] segv[3759]: segfault (11) at 0x0 nip 0x1420 lr 0xfe61854 > code 0x1 in segv[1000+1] > [ 73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6 > 4b30 9421ffd0 93e1002c 7c3f0b78 > [ 73.414665] segv[3759]: code: 3920 913f001c 813f001c 3941 > <9149> 3920 7d234b78 397f0030 Or better yet, "%#lx" - the hash adds the appropriate prefix in the right case for the format. -- Alastair D'Silva mob: 0423 762 819 skype: alastair_dsilva msn: alast...@d-silva.org blog: http://alastair.d-silva.orgTwitter: @EvilDeece
Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()
Murilo Opsfelder Araujo writes: > On Mon, Jul 30, 2018 at 06:30:47PM +0200, LEROY Christophe wrote: >> Murilo Opsfelder Araujo a écrit : >> > On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote: >> > > Murilo Opsfelder Araujo a écrit : >> > > >> > > > Simplify the message format by using REG_FMT as the register format. >> > > > This >> > > > avoids having two different formats and avoids checking for MSR_64BIT. >> > > >> > > Are you sure it is what we want ? >> > >> > Yes. >> > >> > > Won't it change the behaviour for a 32 bits app running on a 64bits >> > > kernel ? >> > >> > In fact, this changes how many zeroes are prefixed when displaying the >> > registers >> > (%016lx vs. %08lx format). For example, 32-bits userspace, 64-bits kernel: >> >> Indeed that's what I suspected. What is the real benefit of this change ? >> Why not keep the current format for 32bits userspace ? All those leading >> zeroes are pointless to me. > > One of the benefits is simplifying the code by removing some checks. Another > is > deduplicating almost identical format strings in favor of a unified one. > > After reading Joe's comment [1], %px seems to be the format we're looking for. > An extract from Documentation/core-api/printk-formats.rst: > > "%px is functionally equivalent to %lx (or %lu). %px is preferred because it > is more uniquely grep'able." > > So I guess we don't need to worry about the format (%016lx vs. %08lx), let's > just use %px, as per the guideline. I don't think I like %px. It makes the format string cleaner, but it means we have to cast everything to void * which is ugly as heck. I actually don't think the leading zeroes are helpful at all in the signal message, ie. we should just use %lx there. They are useful in show_regs() because we want everything to line up. So I think I'll drop patch 3 and use 0x%lx in show_signal_msg(), meaning we end up with, eg: [ 73.414535] segv[3759]: segfault (11) at 0x0 nip 0x1420 lr 0xfe61854 code 0x1 in segv[1000+1] [ 73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6 4b30 9421ffd0 93e1002c 7c3f0b78 [ 73.414665] segv[3759]: code: 3920 913f001c 813f001c 3941 <9149> 3920 7d234b78 397f0030 I'll do that unless anyone screams loudly, because it would be nice to get this into 4.19. cheers
Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()
Hi, Christophe. On Mon, Jul 30, 2018 at 06:30:47PM +0200, LEROY Christophe wrote: > Murilo Opsfelder Araujo a écrit : > > > Hi, Christophe. > > > > On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote: > > > Murilo Opsfelder Araujo a écrit : > > > > > > > Simplify the message format by using REG_FMT as the register format. > > > > This > > > > avoids having two different formats and avoids checking for MSR_64BIT. > > > > > > Are you sure it is what we want ? > > > > Yes. > > > > > Won't it change the behaviour for a 32 bits app running on a 64bits > > > kernel ? > > > > In fact, this changes how many zeroes are prefixed when displaying the > > registers > > (%016lx vs. %08lx format). For example, 32-bits userspace, 64-bits kernel: > > Indeed that's what I suspected. What is the real benefit of this change ? > Why not keep the current format for 32bits userspace ? All those leading > zeroes are pointless to me. One of the benefits is simplifying the code by removing some checks. Another is deduplicating almost identical format strings in favor of a unified one. After reading Joe's comment [1], %px seems to be the format we're looking for. An extract from Documentation/core-api/printk-formats.rst: "%px is functionally equivalent to %lx (or %lu). %px is preferred because it is more uniquely grep'able." So I guess we don't need to worry about the format (%016lx vs. %08lx), let's just use %px, as per the guideline. [1] https://lore.kernel.org/lkml/26f07092cdde378ebb42c1034badde1b56521c36.ca...@perches.com/ Cheers Murilo
Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()
Murilo Opsfelder Araujo a écrit : Hi, Christophe. On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote: Murilo Opsfelder Araujo a écrit : > Simplify the message format by using REG_FMT as the register format. This > avoids having two different formats and avoids checking for MSR_64BIT. Are you sure it is what we want ? Yes. Won't it change the behaviour for a 32 bits app running on a 64bits kernel ? In fact, this changes how many zeroes are prefixed when displaying the registers (%016lx vs. %08lx format). For example, 32-bits userspace, 64-bits kernel: Indeed that's what I suspected. What is the real benefit of this change ? Why not keep the current format for 32bits userspace ? All those leading zeroes are pointless to me. before this series: [66475.002900] segv[4599]: unhandled signal 11 at nip 1420 lr 0fe61854 code 1 after this series: [ 73.414535] segv[3759]: segfault (11) at nip 1420 lr 0fe61854 code 1 in segv[1000+1] [ 73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6 4b30 9421ffd0 93e1002c 7c3f0b78 [ 73.414665] segv[3759]: code: 3920 913f001c 813f001c 3941 <9149> 3920 7d234b78 397f0030 Have you spotted any other behaviour change? Not as of today Christophe Cheers Murilo
Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()
Hi, Christophe. On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote: > Murilo Opsfelder Araujo a écrit : > > > Simplify the message format by using REG_FMT as the register format. This > > avoids having two different formats and avoids checking for MSR_64BIT. > > Are you sure it is what we want ? Yes. > Won't it change the behaviour for a 32 bits app running on a 64bits kernel ? In fact, this changes how many zeroes are prefixed when displaying the registers (%016lx vs. %08lx format). For example, 32-bits userspace, 64-bits kernel: before this series: [66475.002900] segv[4599]: unhandled signal 11 at nip 1420 lr 0fe61854 code 1 after this series: [ 73.414535] segv[3759]: segfault (11) at nip 1420 lr 0fe61854 code 1 in segv[1000+1] [ 73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6 4b30 9421ffd0 93e1002c 7c3f0b78 [ 73.414665] segv[3759]: code: 3920 913f001c 813f001c 3941 <9149> 3920 7d234b78 397f0030 Have you spotted any other behaviour change? Cheers Murilo
Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()
On Fri, 2018-07-27 at 18:40 +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo a écrit :
>
> > Simplify the message format by using REG_FMT as the register format. This
> > avoids having two different formats and avoids checking for MSR_64BIT.
>
> Are you sure it is what we want ?
>
> Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?
[]
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
[]
> > @@ -311,17 +311,13 @@ static bool show_unhandled_signals_ratelimited(void)
> > static void show_signal_msg(int signr, struct pt_regs *regs, int code,
> > unsigned long addr)
> > {
> > - const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
> > - "at %08lx nip %08lx lr %08lx code %x\n";
> > - const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
> > - "at %016lx nip %016lx lr %016lx code %x\n";
> > -
> > if (!unhandled_signal(current, signr))
> > return;
> >
> > - printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
> > - current->comm, current->pid, signr,
> > - addr, regs->nip, regs->link, code);
> > + pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
I think it better to use a space after the close "
and also the line continuation is unnecessary.
> > + " nip "REG_FMT" lr "REG_FMT" code %x\n",
And spaces before the open quotes too.
I'd also prefer the format on a single line:
pr_info("%s[%d]: unhandled signal %d at " REG_FMT " nip " REG_FMT " lr
" REG_FMT " code %x\n",
> > + current->comm, current->pid, signr, addr,
> > + regs->nip, regs->link, code);
Seeing as these are all unsigned long, a better way to do
this is to use %p and cast to pointer.
This might be better anyway as this output exposes pointer
addresses and instead would now use pointer hashed output.
pr_info("%s[%d]: unhandled signal %d at %p nip %p lr %p code %x\n",
current->comm, current->pid, signr,
(void *)addr, (void *)regs->nip, (void *)regs->link, code);
Use %px if you _really_ need to emit unhashed addresses.
see: Documentation/core-api/printk-formats.rst
Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()
Murilo Opsfelder Araujo a écrit :
Simplify the message format by using REG_FMT as the register format. This
avoids having two different formats and avoids checking for MSR_64BIT.
Are you sure it is what we want ?
Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?
Christophe
Signed-off-by: Murilo Opsfelder Araujo
---
arch/powerpc/kernel/traps.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4faab4705774..047d980ac776 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,17 +311,13 @@ static bool show_unhandled_signals_ratelimited(void)
static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
{
- const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
- "at %08lx nip %08lx lr %08lx code %x\n";
- const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
- "at %016lx nip %016lx lr %016lx code %x\n";
-
if (!unhandled_signal(current, signr))
return;
- printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
- current->comm, current->pid, signr,
- addr, regs->nip, regs->link, code);
+ pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
+ " nip "REG_FMT" lr "REG_FMT" code %x\n",
+ current->comm, current->pid, signr, addr,
+ regs->nip, regs->link, code);
}
void _exception_pkey(int signr, struct pt_regs *regs, int code,
--
2.17.1
