[PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Changbin Du]

It's safe to patch text segment in stop_machine. No race is possible here.
Besides, there is a false positive for the lock assertion in
patch_insn_write() since the lock is not held by cpu migration thread.

So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
the lock assertion in patch_insn_write() should be removed to avoid
producing lots of false positive warnings.

Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
 arch/riscv/kernel/ftrace.c | 16 ++--------------
 arch/riscv/kernel/patch.c  | 12 +++++-------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 4716f4cdc038..381ecdae4ea5 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -12,18 +12,6 @@
 #include <asm/patch.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)
-{
-	mutex_lock(&text_mutex);
-	return 0;
-}
-
-int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
-{
-	mutex_unlock(&text_mutex);
-	return 0;
-}
-
 static int ftrace_check_current_call(unsigned long hook_pos,
 				     unsigned int *expected)
 {
@@ -136,9 +124,9 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
 {
 	int out;
 
-	ftrace_arch_code_modify_prepare();
+	mutex_lock(&text_mutex);
 	out = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
-	ftrace_arch_code_modify_post_process();
+	mutex_unlock(&text_mutex);
 
 	return out;
 }
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 0b552873a577..b7c5dea70924 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -49,19 +49,17 @@ static void patch_unmap(int fixmap)
 }
 NOKPROBE_SYMBOL(patch_unmap);
 
+/*
+ * Before reaching here, unless executed by stop_machine, it was
+ * expected to lock the text_mutex already which could ensure that
+ * it was safe between each cores.
+ */
 static int patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
 	int ret;
 
-	/*
-	 * Before reaching here, it was expected to lock the text_mutex
-	 * already, so we don't need to give another lock here and could
-	 * ensure that it was safe between each cores.
-	 */
-	lockdep_assert_held(&text_mutex);
-
 	if (across_pages)
 		patch_map(addr + len, FIX_TEXT_POKE1);
 
-- 
2.25.1

[PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Changbin Du]

It's safe to patch text segment in stop_machine. No race is possible here.
Besides, there is a false positive for the lock assertion in
patch_insn_write() since the lock is not held by cpu migration thread.

So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
the lock assertion in patch_insn_write() should be removed to avoid
producing lots of false positive warnings.

Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
 arch/riscv/kernel/ftrace.c | 16 ++--------------
 arch/riscv/kernel/patch.c  | 12 +++++-------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 4716f4cdc038..381ecdae4ea5 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -12,18 +12,6 @@
 #include <asm/patch.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)
-{
-	mutex_lock(&text_mutex);
-	return 0;
-}
-
-int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
-{
-	mutex_unlock(&text_mutex);
-	return 0;
-}
-
 static int ftrace_check_current_call(unsigned long hook_pos,
 				     unsigned int *expected)
 {
@@ -136,9 +124,9 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
 {
 	int out;
 
-	ftrace_arch_code_modify_prepare();
+	mutex_lock(&text_mutex);
 	out = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
-	ftrace_arch_code_modify_post_process();
+	mutex_unlock(&text_mutex);
 
 	return out;
 }
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 0b552873a577..b7c5dea70924 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -49,19 +49,17 @@ static void patch_unmap(int fixmap)
 }
 NOKPROBE_SYMBOL(patch_unmap);
 
+/*
+ * Before reaching here, unless executed by stop_machine, it was
+ * expected to lock the text_mutex already which could ensure that
+ * it was safe between each cores.
+ */
 static int patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
 	int ret;
 
-	/*
-	 * Before reaching here, it was expected to lock the text_mutex
-	 * already, so we don't need to give another lock here and could
-	 * ensure that it was safe between each cores.
-	 */
-	lockdep_assert_held(&text_mutex);
-
 	if (across_pages)
 		patch_map(addr + len, FIX_TEXT_POKE1);
 
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Steven Rostedt]

On Thu, 10 Mar 2022 12:54:54 +0800
Changbin Du <changbin.du@gmail.com> wrote:

> It's safe to patch text segment in stop_machine. No race is possible here.
> Besides, there is a false positive for the lock assertion in
> patch_insn_write() since the lock is not held by cpu migration thread.
> 
> So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
> the lock assertion in patch_insn_write() should be removed to avoid
> producing lots of false positive warnings.
> 
> Signed-off-by: Changbin Du <changbin.du@gmail.com>

Ideally, RISC-V should try to get off of the stop_machine approach, and
move to the breakpoint modification.

-- Steve

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Steven Rostedt]

On Thu, 10 Mar 2022 12:54:54 +0800
Changbin Du <changbin.du@gmail.com> wrote:

> It's safe to patch text segment in stop_machine. No race is possible here.
> Besides, there is a false positive for the lock assertion in
> patch_insn_write() since the lock is not held by cpu migration thread.
> 
> So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
> the lock assertion in patch_insn_write() should be removed to avoid
> producing lots of false positive warnings.
> 
> Signed-off-by: Changbin Du <changbin.du@gmail.com>

Ideally, RISC-V should try to get off of the stop_machine approach, and
move to the breakpoint modification.

-- Steve

Re: [PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Changbin Du]

On Thu, Mar 10, 2022 at 09:27:42AM -0500, Steven Rostedt wrote:
> On Thu, 10 Mar 2022 12:54:54 +0800
> Changbin Du <changbin.du@gmail.com> wrote:
> 
> > It's safe to patch text segment in stop_machine. No race is possible here.
> > Besides, there is a false positive for the lock assertion in
> > patch_insn_write() since the lock is not held by cpu migration thread.
> > 
> > So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
> > the lock assertion in patch_insn_write() should be removed to avoid
> > producing lots of false positive warnings.
> > 
> > Signed-off-by: Changbin Du <changbin.du@gmail.com>
> 
> Ideally, RISC-V should try to get off of the stop_machine approach, and
> move to the breakpoint modification.
>
yes, that's a further step. I can feel a obvious stall to enable ftrace running
in QEMU. (maybe qemu-riscv tcg is too slow...)

> -- Steve

-- 
Cheers,
Changbin Du

Re: [PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Changbin Du]

On Thu, Mar 10, 2022 at 09:27:42AM -0500, Steven Rostedt wrote:
> On Thu, 10 Mar 2022 12:54:54 +0800
> Changbin Du <changbin.du@gmail.com> wrote:
> 
> > It's safe to patch text segment in stop_machine. No race is possible here.
> > Besides, there is a false positive for the lock assertion in
> > patch_insn_write() since the lock is not held by cpu migration thread.
> > 
> > So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
> > the lock assertion in patch_insn_write() should be removed to avoid
> > producing lots of false positive warnings.
> > 
> > Signed-off-by: Changbin Du <changbin.du@gmail.com>
> 
> Ideally, RISC-V should try to get off of the stop_machine approach, and
> move to the breakpoint modification.
>
yes, that's a further step. I can feel a obvious stall to enable ftrace running
in QEMU. (maybe qemu-riscv tcg is too slow...)

> -- Steve

-- 
Cheers,
Changbin Du

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Palmer Dabbelt]

On Sun, 13 Mar 2022 00:07:11 PST (-0800), changbin.du@gmail.com wrote:
> On Thu, Mar 10, 2022 at 09:27:42AM -0500, Steven Rostedt wrote:
>> On Thu, 10 Mar 2022 12:54:54 +0800
>> Changbin Du <changbin.du@gmail.com> wrote:
>>
>> > It's safe to patch text segment in stop_machine. No race is possible here.
>> > Besides, there is a false positive for the lock assertion in
>> > patch_insn_write() since the lock is not held by cpu migration thread.
>> >
>> > So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
>> > the lock assertion in patch_insn_write() should be removed to avoid
>> > producing lots of false positive warnings.
>> >
>> > Signed-off-by: Changbin Du <changbin.du@gmail.com>
>>
>> Ideally, RISC-V should try to get off of the stop_machine approach, and
>> move to the breakpoint modification.
>>
> yes, that's a further step. I can feel a obvious stall to enable ftrace running
> in QEMU. (maybe qemu-riscv tcg is too slow...)

Looks like we've had this exact discussion before, even with exactly the 
same patch and people and everything.  I guess I dropped the ball here, 
I got so distracted trying to figure out those sequences to avoid 
stop_machine() that I forgot to clean up the patch to fix stop machine.  
I just sent out a v2 <20220322022331.32136-1-palmer@rivosinc.com>.  That 
should at least fix the crash, we can deal with getting rid of 
stop_machine() later.

Re: [PATCH] riscv: ftrace: no need to acquire text_mutex when executed in stop_machine

[Palmer Dabbelt]

On Sun, 13 Mar 2022 00:07:11 PST (-0800), changbin.du@gmail.com wrote:
> On Thu, Mar 10, 2022 at 09:27:42AM -0500, Steven Rostedt wrote:
>> On Thu, 10 Mar 2022 12:54:54 +0800
>> Changbin Du <changbin.du@gmail.com> wrote:
>>
>> > It's safe to patch text segment in stop_machine. No race is possible here.
>> > Besides, there is a false positive for the lock assertion in
>> > patch_insn_write() since the lock is not held by cpu migration thread.
>> >
>> > So we actually don't need our ftrace_arch_code_modify_prepare/post(). And
>> > the lock assertion in patch_insn_write() should be removed to avoid
>> > producing lots of false positive warnings.
>> >
>> > Signed-off-by: Changbin Du <changbin.du@gmail.com>
>>
>> Ideally, RISC-V should try to get off of the stop_machine approach, and
>> move to the breakpoint modification.
>>
> yes, that's a further step. I can feel a obvious stall to enable ftrace running
> in QEMU. (maybe qemu-riscv tcg is too slow...)

Looks like we've had this exact discussion before, even with exactly the 
same patch and people and everything.  I guess I dropped the ball here, 
I got so distracted trying to figure out those sequences to avoid 
stop_machine() that I forgot to clean up the patch to fix stop machine.  
I just sent out a v2 <20220322022331.32136-1-palmer@rivosinc.com>.  That 
should at least fix the crash, we can deal with getting rid of 
stop_machine() later.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv