Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 18 Apr 2022 11:51:16 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH v1 07/22] powerpc/ftrace: Use patch_instruction() return
 directly
To:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Ingo Molnar <mingo@redhat.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Paul Mackerras <paulus@samba.org>,
        Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
References: <cover.1648131740.git.christophe.leroy@csgroup.eu>
        <b4505e936e1aee411f7132a27791cf138102f35f.1648131740.git.christophe.leroy@csgroup.eu>
In-Reply-To: <b4505e936e1aee411f7132a27791cf138102f35f.1648131740.git.christophe.leroy@csgroup.eu>
MIME-Version: 1.0
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1650262058.altfknhzto.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Christophe Leroy wrote:
> Instead of returning -EPERM when patch_instruction() fails,
> just return what patch_instruction returns.
>=20
> That simplifies ftrace_modify_code():
>=20
> 	   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
>=20
> Before:
>=20
> 	   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
>=20
> It improves ftrace activation/deactivation duration by about 3%.
>=20
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/trace/ftrace.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/tra=
ce/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, p=
pc_inst_t new)
>  	}
>=20
>  	/* 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=20
throw the right error message. That will change due to this patch,=20
though I'm not sure how much it matters. -EFAULT and -EPERM seem to=20
print almost the same error message.

- Naveen