| Message ID | b4505e936e1aee411f7132a27791cf138102f35f.1648131740.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: ftrace optimisation and cleanup and more [v1] | expand |
Christophe Leroy wrote: > Instead of returning -EPERM when patch_instruction() fails, > just return what patch_instruction returns. > > That simplifies ftrace_modify_code(): > > 0: 94 21 ff c0 stwu r1,-64(r1) > 4: 93 e1 00 3c stw r31,60(r1) > 8: 7c 7f 1b 79 mr. r31,r3 > c: 40 80 00 30 bge 3c <ftrace_modify_code+0x3c> > 10: 93 c1 00 38 stw r30,56(r1) > 14: 7c 9e 23 78 mr r30,r4 > 18: 7c a4 2b 78 mr r4,r5 > 1c: 80 bf 00 00 lwz r5,0(r31) > 20: 7c 1e 28 40 cmplw r30,r5 > 24: 40 82 00 34 bne 58 <ftrace_modify_code+0x58> > 28: 83 c1 00 38 lwz r30,56(r1) > 2c: 7f e3 fb 78 mr r3,r31 > 30: 83 e1 00 3c lwz r31,60(r1) > 34: 38 21 00 40 addi r1,r1,64 > 38: 48 00 00 00 b 38 <ftrace_modify_code+0x38> > 38: R_PPC_REL24 patch_instruction > > Before: > > 0: 94 21 ff c0 stwu r1,-64(r1) > 4: 93 e1 00 3c stw r31,60(r1) > 8: 7c 7f 1b 79 mr. r31,r3 > c: 40 80 00 4c bge 58 <ftrace_modify_code+0x58> > 10: 93 c1 00 38 stw r30,56(r1) > 14: 7c 9e 23 78 mr r30,r4 > 18: 7c a4 2b 78 mr r4,r5 > 1c: 80 bf 00 00 lwz r5,0(r31) > 20: 7c 08 02 a6 mflr r0 > 24: 90 01 00 44 stw r0,68(r1) > 28: 7c 1e 28 40 cmplw r30,r5 > 2c: 40 82 00 48 bne 74 <ftrace_modify_code+0x74> > 30: 7f e3 fb 78 mr r3,r31 > 34: 48 00 00 01 bl 34 <ftrace_modify_code+0x34> > 34: R_PPC_REL24 patch_instruction > 38: 80 01 00 44 lwz r0,68(r1) > 3c: 20 63 00 00 subfic r3,r3,0 > 40: 83 c1 00 38 lwz r30,56(r1) > 44: 7c 63 19 10 subfe r3,r3,r3 > 48: 7c 08 03 a6 mtlr r0 > 4c: 83 e1 00 3c lwz r31,60(r1) > 50: 38 21 00 40 addi r1,r1,64 > 54: 4e 80 00 20 blr > > It improves ftrace activation/deactivation duration by about 3%. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/kernel/trace/ftrace.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c > index 98e82fa4980f..1b05d33f96c6 100644 > --- a/arch/powerpc/kernel/trace/ftrace.c > +++ b/arch/powerpc/kernel/trace/ftrace.c > @@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new) > } > > /* replace the text with the new text */ > - if (patch_instruction((u32 *)ip, new)) > - return -EPERM; > - > - return 0; > + return patch_instruction((u32 *)ip, new); I think the reason we were returning -EPERM is so that ftrace_bug() can throw the right error message. That will change due to this patch, though I'm not sure how much it matters. -EFAULT and -EPERM seem to print almost the same error message. - Naveen
On Mon, 18 Apr 2022 11:51:16 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > --- a/arch/powerpc/kernel/trace/ftrace.c > > +++ b/arch/powerpc/kernel/trace/ftrace.c > > @@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new) > > } > > > > /* replace the text with the new text */ > > - if (patch_instruction((u32 *)ip, new)) > > - return -EPERM; > > - > > - return 0; > > + return patch_instruction((u32 *)ip, new); > > I think the reason we were returning -EPERM is so that ftrace_bug() can That is correct. > throw the right error message. That will change due to this patch, > though I'm not sure how much it matters. -EFAULT and -EPERM seem to > print almost the same error message. In these cases it helps to know the type of failure, as the way to debug it is different. -EFAULT: It failed to read it the location. This means that the memory is likely not even mapped in, or the pointer is way off. -EINVAL: Means that what was read did not match what was expected (the code was already updated, pointing to the wrong location, or simply the calculation of what to expect is incorrect). -EPERM: Means the write failed. What was read was expected, but the permissions to write have not been updated properly. Differentiating the three is crucial to looking at where the issue lies when an ftrace_bug() triggers. -- Steve
Le 18/04/2022 à 21:44, Steven Rostedt a écrit : > On Mon, 18 Apr 2022 11:51:16 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >>> --- a/arch/powerpc/kernel/trace/ftrace.c >>> +++ b/arch/powerpc/kernel/trace/ftrace.c >>> @@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new) >>> } >>> >>> /* replace the text with the new text */ >>> - if (patch_instruction((u32 *)ip, new)) >>> - return -EPERM; >>> - >>> - return 0; >>> + return patch_instruction((u32 *)ip, new); >> >> I think the reason we were returning -EPERM is so that ftrace_bug() can > > That is correct. > >> throw the right error message. That will change due to this patch, >> though I'm not sure how much it matters. -EFAULT and -EPERM seem to >> print almost the same error message. > > In these cases it helps to know the type of failure, as the way to debug it > is different. > > -EFAULT: It failed to read it the location. This means that the memory is > likely not even mapped in, or the pointer is way off. > > -EINVAL: Means that what was read did not match what was expected (the code > was already updated, pointing to the wrong location, or simply the > calculation of what to expect is incorrect). > > -EPERM: Means the write failed. What was read was expected, but the > permissions to write have not been updated properly. > > Differentiating the three is crucial to looking at where the issue lies > when an ftrace_bug() triggers. > Apparently no caller really care about the value returned by patch_instruction(), the ones who check the return value just check that it's not 0. So the most performant would be to have patch_instruction() return -EPERM instead of -EFAULT in case of failure. Christophe
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c index 98e82fa4980f..1b05d33f96c6 100644 --- a/arch/powerpc/kernel/trace/ftrace.c +++ b/arch/powerpc/kernel/trace/ftrace.c @@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new) } /* replace the text with the new text */ - if (patch_instruction((u32 *)ip, new)) - return -EPERM; - - return 0; + return patch_instruction((u32 *)ip, new); } /*
Instead of returning -EPERM when patch_instruction() fails, just return what patch_instruction returns. That simplifies ftrace_modify_code(): 0: 94 21 ff c0 stwu r1,-64(r1) 4: 93 e1 00 3c stw r31,60(r1) 8: 7c 7f 1b 79 mr. r31,r3 c: 40 80 00 30 bge 3c <ftrace_modify_code+0x3c> 10: 93 c1 00 38 stw r30,56(r1) 14: 7c 9e 23 78 mr r30,r4 18: 7c a4 2b 78 mr r4,r5 1c: 80 bf 00 00 lwz r5,0(r31) 20: 7c 1e 28 40 cmplw r30,r5 24: 40 82 00 34 bne 58 <ftrace_modify_code+0x58> 28: 83 c1 00 38 lwz r30,56(r1) 2c: 7f e3 fb 78 mr r3,r31 30: 83 e1 00 3c lwz r31,60(r1) 34: 38 21 00 40 addi r1,r1,64 38: 48 00 00 00 b 38 <ftrace_modify_code+0x38> 38: R_PPC_REL24 patch_instruction Before: 0: 94 21 ff c0 stwu r1,-64(r1) 4: 93 e1 00 3c stw r31,60(r1) 8: 7c 7f 1b 79 mr. r31,r3 c: 40 80 00 4c bge 58 <ftrace_modify_code+0x58> 10: 93 c1 00 38 stw r30,56(r1) 14: 7c 9e 23 78 mr r30,r4 18: 7c a4 2b 78 mr r4,r5 1c: 80 bf 00 00 lwz r5,0(r31) 20: 7c 08 02 a6 mflr r0 24: 90 01 00 44 stw r0,68(r1) 28: 7c 1e 28 40 cmplw r30,r5 2c: 40 82 00 48 bne 74 <ftrace_modify_code+0x74> 30: 7f e3 fb 78 mr r3,r31 34: 48 00 00 01 bl 34 <ftrace_modify_code+0x34> 34: R_PPC_REL24 patch_instruction 38: 80 01 00 44 lwz r0,68(r1) 3c: 20 63 00 00 subfic r3,r3,0 40: 83 c1 00 38 lwz r30,56(r1) 44: 7c 63 19 10 subfe r3,r3,r3 48: 7c 08 03 a6 mtlr r0 4c: 83 e1 00 3c lwz r31,60(r1) 50: 38 21 00 40 addi r1,r1,64 54: 4e 80 00 20 blr It improves ftrace activation/deactivation duration by about 3%. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/kernel/trace/ftrace.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)