| Message ID | 20220310045454.672097-1-changbin.du@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: ftrace: no need to acquire text_mutex when executed in stop_machine | expand |
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
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
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.
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);
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(-)