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 <[email protected]>
---
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
On Thu, 10 Mar 2022 12:54:54 +0800
Changbin Du <[email protected]> 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 <[email protected]>
Ideally, RISC-V should try to get off of the stop_machine approach, and
move to the breakpoint modification.
-- Steve
On Thu, Mar 10, 2022 at 09:27:42AM -0500, Steven Rostedt wrote:
> On Thu, 10 Mar 2022 12:54:54 +0800
> Changbin Du <[email protected]> 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 <[email protected]>
>
> 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
On Sun, 13 Mar 2022 00:07:11 PST (-0800), [email protected] 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 <[email protected]> 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 <[email protected]>
>>
>> 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 <[email protected]>. That
should at least fix the crash, we can deal with getting rid of
stop_machine() later.