arm64: ftrace: add cond_resched() to func ftrace_make_(call|nop)

Both of those functions end up calling ftrace_modify_code(), which is expensive because it changes the page tables and flush caches. Microseconds add up because this is called in a loop for each dyn_ftrace record, and this triggers the softlockup watchdog unless we let it sleep occasionally.
Rework so that we call cond_resched() before going into the ftrace_modify_code() function.

Co-developed-by Arnd Bergmann <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

Signed-off-by Anders Roxell <[email protected]>

---
 arch/arm64/kernel/ftrace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm64/kernel/ftrace.c +10 -0

index de1a397d2d3f..9da38da58df7 100644
@@ -130,6 +130,11 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	old = aarch64_insn_gen_nop();
 	new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK);
 
+	/* This function can take a long time when sanitizers are enabled, so
+	 * lets make sure we allow RCU processing.
+	 */
+	cond_resched();
+
 	return ftrace_modify_code(pc, old, new, true);
 }
 

@@ -188,6 +193,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 
 	new = aarch64_insn_gen_nop();
 
+	/* This function can take a long time when sanitizers are enabled, so
+	 * lets make sure we allow RCU processing.
+	 */
+	cond_resched();
+
 	return ftrace_modify_code(pc, old, new, validate);
 }
 

Both of those functions end up calling ftrace_modify_code(), which is expensive because it changes the page tables and flush caches. Microseconds add up because this is called in a loop for each dyn_ftrace record, and this triggers the softlockup watchdog unless we let it sleep occasionally.
Rework so that we call cond_resched() before going into the ftrace_modify_code() function.

Co-developed-by Arnd Bergmann <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

Signed-off-by Anders Roxell <[email protected]>

---
 arch/arm64/kernel/ftrace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Hi Anders,

Quoted 8 lines · Anders Roxell

Both of those functions end up calling ftrace_modify_code(), which is expensive because it changes the page tables and flush caches. Microseconds add up because this is called in a loop for each dyn_ftrace record, and this triggers the softlockup watchdog unless we let it sleep occasionally.
Rework so that we call cond_resched() before going into the ftrace_modify_code() function.

Quoted 6 lines

Co-developed-by Arnd Bergmann <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

Signed-off-by Anders Roxell <[email protected]>

---
 arch/arm64/kernel/ftrace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

It sounds like you're running into issues with the existing code, but I'd like to understand a bit more about exactly what you're seeing. Which part of the ftrace patching is proving to be expensive?

The page table manipulation only happens once per module when using PLTs, and the cache maintenance is just a single line per patch site without an IPI.

Is it the loop in ftrace_replace_code() that is causing the hassle?

Quoted 23 lines · Will Deacon

Hi Anders,

Quoted 8 lines · Anders Roxell

Both of those functions end up calling ftrace_modify_code(), which is expensive because it changes the page tables and flush caches. Microseconds add up because this is called in a loop for each dyn_ftrace record, and this triggers the softlockup watchdog unless we let it sleep occasionally.
Rework so that we call cond_resched() before going into the ftrace_modify_code() function.

Quoted 6 lines

Co-developed-by Arnd Bergmann <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

Signed-off-by Anders Roxell <[email protected]>

---
 arch/arm64/kernel/ftrace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)  

It sounds like you're running into issues with the existing code, but I'd like to understand a bit more about exactly what you're seeing. Which part of the ftrace patching is proving to be expensive?

Quoted 5 lines

The page table manipulation only happens once per module when using PLTs, and the cache maintenance is just a single line per patch site without an IPI.

Is it the loop in ftrace_replace_code() that is causing the hassle?

Yes: with an allmodconfig kernel, the ftrace selftest calls ftrace_replace_code to look >40000 through ftrace_make_call/ftrace_make_nop, and these end up calling

static int __kprobes __aarch64_insn_write(void *addr, __le32 insn)

{
        void *waddr = addr;
        unsigned long flags = 0;
        int ret;

        raw_spin_lock_irqsave(&patch_lock, flags);
        waddr = patch_map(addr, FIX_TEXT_POKE0);

        ret = probe_kernel_write(waddr, &insn, AARCH64_INSN_SIZE);

        patch_unmap(FIX_TEXT_POKE0);
        raw_spin_unlock_irqrestore(&patch_lock, flags);

        return ret;
}
int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
{
        u32 *tp = addr;
        int ret;

        /* A64 instructions must be word aligned */
        if ((uintptr_t)tp & 0x3)
                return -EINVAL;

        ret = aarch64_insn_write(tp, insn);
        if (ret == 0)
                __flush_icache_range((uintptr_t)tp,
                                     (uintptr_t)tp + AARCH64_INSN_SIZE);

        return ret;
}

which seems to be where the main cost is. This is with inside of qemu, and with lots of debugging options (in particular kcov and ubsan) enabled, that make each function call more expensive.

Quoted 25 lines · On Mon, 3 Dec 2018 22:51:52 +0100 Arnd Bergmann <[email protected]> wrote:

Quoted 24 lines · Will Deacon

Quoted 8 lines · Anders Roxell

Both of those functions end up calling ftrace_modify_code(), which is expensive because it changes the page tables and flush caches. Microseconds add up because this is called in a loop for each dyn_ftrace record, and this triggers the softlockup watchdog unless we let it sleep occasionally.
Rework so that we call cond_resched() before going into the ftrace_modify_code() function.

Quoted 6 lines

Co-developed-by Arnd Bergmann <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

Signed-off-by Anders Roxell <[email protected]>

---
 arch/arm64/kernel/ftrace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)  

It sounds like you're running into issues with the existing code, but I'd like to understand a bit more about exactly what you're seeing. Which part of the ftrace patching is proving to be expensive?

The page table manipulation only happens once per module when using PLTs, and the cache maintenance is just a single line per patch site without an IPI.

Quoted 5 lines

Quoted 1 line

Is it the loop in ftrace_replace_code() that is causing the hassle?

Yes: with an allmodconfig kernel, the ftrace selftest calls ftrace_replace_code to look >40000 through ftrace_make_call/ftrace_make_nop, and these end up calling

Quoted 33 lines

static int __kprobes __aarch64_insn_write(void *addr, __le32 insn)

{
        void *waddr = addr;
        unsigned long flags = 0;
        int ret;

        raw_spin_lock_irqsave(&patch_lock, flags);
        waddr = patch_map(addr, FIX_TEXT_POKE0);

        ret = probe_kernel_write(waddr, &insn, AARCH64_INSN_SIZE);

        patch_unmap(FIX_TEXT_POKE0);
        raw_spin_unlock_irqrestore(&patch_lock, flags);

        return ret;
}
int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
{
        u32 *tp = addr;
        int ret;

        /* A64 instructions must be word aligned */
        if ((uintptr_t)tp & 0x3)
                return -EINVAL;

        ret = aarch64_insn_write(tp, insn);
        if (ret == 0)
                __flush_icache_range((uintptr_t)tp,
                                     (uintptr_t)tp + AARCH64_INSN_SIZE);

        return ret;
}

Quoted 4 lines

which seems to be where the main cost is. This is with inside of qemu, and with lots of debugging options (in particular kcov and ubsan) enabled, that make each function call more expensive.

I was thinking more about this. Would something like this work?

-- Steve

kernel/trace/ftrace.c +0 -0

index 8ef9fc226037..42e89397778b 100644

Quoted 11 lines

kernel/trace/ftrace.c +1 -0

index 8ef9fc226037..42e89397778b 100644
@@ -2393,11 +2393,14 @@ void __weak ftrace_replace_code(int enable)
 {
 	struct dyn_ftrace *rec;
 	struct ftrace_page *pg;
+	bool schedulable;
 	int failed;
 

kernel/trace/ftrace.c void __weak ftrace_replace_code(int enable) · line 2402

Quoted 4 lines

kernel/trace/ftrace.c +1 -0

 	if (unlikely(ftrace_disabled))
 		return;
 
+	schedulable = !irqs_disabled() & !preempt_count();  

Looks suspiciously like a bitwise preemptible() to me!

Quoted 4 lines

kernel/trace/ftrace.c +1 -0

+
 	do_for_each_ftrace_rec(pg, rec) {
 

kernel/trace/ftrace.c void __weak ftrace_replace_code(int enable) · line 2418

Quoted 9 lines

kernel/trace/ftrace.c +2 -0

 		if (rec->flags & FTRACE_FL_DISABLED)

@@ -2409,6 +2412,8 @@ void __weak ftrace_replace_code(int enable)
 			/* Stop processing */
 			return;
 		}
+		if (schedulable)
+			cond_resched();
 	} while_for_each_ftrace_rec();
 }  

If this solves the problem in core code, them I'm all for it. Otherwise, I was thinking of rolling our own ftrace_replace_code() for arm64, but that's going to involve a fair amount of duplication.

Quoted 11 lines · On Tue, 4 Dec 2018 11:12:43 +0000 Will Deacon <[email protected]> wrote:

Quoted 11 lines

kernel/trace/ftrace.c +1 -0

index 8ef9fc226037..42e89397778b 100644
@@ -2393,11 +2393,14 @@ void __weak ftrace_replace_code(int enable)
 {
    struct dyn_ftrace *rec;
    struct ftrace_page *pg;
+   bool schedulable;
    int failed;
 

Quoted 6 lines

kernel/trace/ftrace.c void __weak ftrace_replace_code(int enable) · line 2402

Quoted 4 lines

kernel/trace/ftrace.c +1 -0

    if (unlikely(ftrace_disabled))
            return;
 
+   schedulable = !irqs_disabled() & !preempt_count();

Looks suspiciously like a bitwise preemptible() to me!

Ah, thanks. Yeah, that should have been &&. But what did you expect. I didn't even compile this ;-)

Quoted 4 lines

Quoted 4 lines

kernel/trace/ftrace.c +1 -0

+
    do_for_each_ftrace_rec(pg, rec) {
 

Quoted 14 lines

kernel/trace/ftrace.c void __weak ftrace_replace_code(int enable) · line 2418

Quoted 9 lines

kernel/trace/ftrace.c +2 -0

            if (rec->flags & FTRACE_FL_DISABLED)

@@ -2409,6 +2412,8 @@ void __weak ftrace_replace_code(int enable)
                    /* Stop processing */
                    return;
            }
+           if (schedulable)
+                   cond_resched();
    } while_for_each_ftrace_rec();
 }

If this solves the problem in core code, them I'm all for it. Otherwise, I was thinking of rolling our own ftrace_replace_code() for arm64, but that's going to involve a fair amount of duplication.

If it does, then I'll add it. Or take a patch for it ;-)

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

+ schedulable = !irqs_disabled() & !preempt_count();

Looks suspiciously like a bitwise preemptible() to me!

Ah, thanks. Yeah, that should have been &&. But what did you expect. I didn't even compile this ;-)

Quoted 1 line

If it does, then I'll add it. Or take a patch for it ;-)

I tested your patch. it worked.

I'll send a patch shortly.