tracing: arm64: Make ftrace_replace_code() schedulable for arm64

PATCH In review Steven Rostedt 3 participants 9 messages Dec 5, 2018 – Dec 6, 2018

Subsystems (3)

arm64 Tracing ftrace

lore

Share & tools

lore

Steven Rostedt

author maintainer

2018-12-05 18:33 UTC #0 lore raw

cover tracing: arm64: Make ftrace_replace_code() schedulable for arm64

This is a little more involved, and I would like to push this through my tree. Can I get a reviewed-by/ack for the second (arm64) patch?

Anders, can you also test this to make sure that it fixes the issue you see?

Thanks!

-- Steve

Steven Rostedt (VMware) (2):
ftrace: Allow ftrace_replace_code() to be schedulable
arm64: ftrace: Set FTRACE_SCHEDULABLE before ftrace_modify_all_code()

----

 arch/arm64/kernel/ftrace.c |  1 +
 include/linux/ftrace.h     |  1 +
 kernel/trace/ftrace.c      | 19 ++++++++++++++++---
 3 files changed, 18 insertions(+), 3 deletions(-)

Steven Rostedt

author maintainer

2018-12-05 18:33 UTC #1 lore raw

PATCH 1/2 ftrace: Allow ftrace_replace_code() to be schedulable

From: "Steven Rostedt (VMware)" <[email protected]>

The function ftrace_replace_code() is the ftrace engine that does the work to modify all the nops into the calls to the function callback in all the functions being traced.

The generic version which is normally called from stop machine, but an architecture can implement a non stop machine version and still use the generic ftrace_replace_code(). When an architecture does this, ftrace_replace_code() may be called from a schedulable context, where it can allow the code to be preemptible, and schedule out.

In order to allow an architecture to make ftrace_replace_code() schedulable, a new command flag is added called:

FTRACE_SCHEDULABLE

Which can be or'd to the command that is passed to ftrace_modify_all_code() that calls ftrace_replace_code() and will have it call cond_resched() in the loop that modifies the nops into the calls to the ftrace trampolines.

Link http://lkml.kernel.org/r/[email protected] open here

Reported-by Anders Roxell <[email protected]>

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 include/linux/ftrace.h |  1 +
 kernel/trace/ftrace.c  | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

include/linux/ftrace.h +1 -0

index dd16e8218db3..c281b16baef9 100644
@@ -389,6 +389,7 @@ enum {
 	FTRACE_UPDATE_TRACE_FUNC	= (1 << 2),
 	FTRACE_START_FUNC_RET		= (1 << 3),
 	FTRACE_STOP_FUNC_RET		= (1 << 4),
+	FTRACE_SCHEDULABLE		= (1 << 5),
 };
 
 /*

kernel/trace/ftrace.c +16 -3

index 77734451cb05..74fdcacba514 100644
@@ -77,6 +77,11 @@
 #define ASSIGN_OPS_HASH(opsname, val)
 #endif
 
+enum {
+	FTRACE_MODIFY_ENABLE_FL		= (1 << 0),
+	FTRACE_MODIFY_SCHEDULABLE_FL	= (1 << 1),
+};
+
 static struct ftrace_ops ftrace_list_end __read_mostly = {
 	.func		= ftrace_stub,
 	.flags		= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_STUB,

@@ -2415,10 +2420,12 @@ __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
 	return -1; /* unknow ftrace bug */
 }
 
-void __weak ftrace_replace_code(int enable)
+void __weak ftrace_replace_code(int mod_flags)
 {
 	struct dyn_ftrace *rec;
 	struct ftrace_page *pg;
+	int enable = mod_flags & FTRACE_MODIFY_ENABLE_FL;
+	int schedulable = mod_flags & FTRACE_MODIFY_SCHEDULABLE_FL;
 	int failed;
 
 	if (unlikely(ftrace_disabled))

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

@@ -2548,8 +2557,12 @@ int __weak ftrace_arch_code_modify_post_process(void)
 void ftrace_modify_all_code(int command)
 {
 	int update = command & FTRACE_UPDATE_TRACE_FUNC;
+	int mod_flags = 0;
 	int err = 0;
 
+	if (command & FTRACE_SCHEDULABLE)
+		mod_flags = FTRACE_MODIFY_SCHEDULABLE_FL;
+
 	/*
 	 * If the ftrace_caller calls a ftrace_ops func directly,
 	 * we need to make sure that it only traces functions it

@@ -2567,9 +2580,9 @@ void ftrace_modify_all_code(int command)
 	}
 
 	if (command & FTRACE_UPDATE_CALLS)
-		ftrace_replace_code(1);
+		ftrace_replace_code(mod_flags | FTRACE_MODIFY_ENABLE_FL);
 	else if (command & FTRACE_DISABLE_CALLS)
-		ftrace_replace_code(0);
+		ftrace_replace_code(mod_flags);
 
 	if (update && ftrace_trace_function != ftrace_ops_list_func) {
 		function_trace_op = set_function_trace_op;

Signature · 1 line

2.19.1

Anders Roxell

Tested-by on v1 1/2

2018-12-06 14:32 UTC #2 lore raw

Re: [PATCH 1/2] ftrace: Allow ftrace_replace_code() to be schedulable

Quoted 23 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>
The function ftrace_replace_code() is the ftrace engine that does the work to modify all the nops into the calls to the function callback in all the functions being traced.
The generic version which is normally called from stop machine, but an architecture can implement a non stop machine version and still use the generic ftrace_replace_code(). When an architecture does this, ftrace_replace_code() may be called from a schedulable context, where it can allow the code to be preemptible, and schedule out.
In order to allow an architecture to make ftrace_replace_code() schedulable, a new command flag is added called:
FTRACE_SCHEDULABLE
Which can be or'd to the command that is passed to ftrace_modify_all_code() that calls ftrace_replace_code() and will have it call cond_resched() in the loop that modifies the nops into the calls to the ftrace trampolines.

Quoted 4 lines

Link http://lkml.kernel.org/r/[email protected] open here
Reported-by Anders Roxell <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>

Tested-by Anders Roxell <[email protected]>

Cheers,
Anders

Quoted 85 lines

---
 include/linux/ftrace.h |  1 +
 kernel/trace/ftrace.c  | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

include/linux/ftrace.h +1 -0

index dd16e8218db3..c281b16baef9 100644
@@ -389,6 +389,7 @@ enum {
        FTRACE_UPDATE_TRACE_FUNC        = (1 << 2),
        FTRACE_START_FUNC_RET           = (1 << 3),
        FTRACE_STOP_FUNC_RET            = (1 << 4),
+       FTRACE_SCHEDULABLE              = (1 << 5),
 };
 
 /*

kernel/trace/ftrace.c +16 -4

index 77734451cb05..74fdcacba514 100644
@@ -77,6 +77,11 @@
 #define ASSIGN_OPS_HASH(opsname, val)
 #endif
 
+enum {
+       FTRACE_MODIFY_ENABLE_FL         = (1 << 0),
+       FTRACE_MODIFY_SCHEDULABLE_FL    = (1 << 1),
+};
+
 static struct ftrace_ops ftrace_list_end __read_mostly = {
        .func           = ftrace_stub,
        .flags          = FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_STUB,

@@ -2415,10 +2420,12 @@ __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
        return -1; /* unknow ftrace bug */
 }
 
-void __weak ftrace_replace_code(int enable)
+void __weak ftrace_replace_code(int mod_flags)
 {
        struct dyn_ftrace *rec;
        struct ftrace_page *pg;
+       int enable = mod_flags & FTRACE_MODIFY_ENABLE_FL;
+       int schedulable = mod_flags & FTRACE_MODIFY_SCHEDULABLE_FL;
        int failed;
 
        if (unlikely(ftrace_disabled))

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

@@ -2548,8 +2557,12 @@ int __weak ftrace_arch_code_modify_post_process(void)
 void ftrace_modify_all_code(int command)
 {
        int update = command & FTRACE_UPDATE_TRACE_FUNC;
+       int mod_flags = 0;
        int err = 0;
 
+       if (command & FTRACE_SCHEDULABLE)
+               mod_flags = FTRACE_MODIFY_SCHEDULABLE_FL;
+
        /*
         * If the ftrace_caller calls a ftrace_ops func directly,
         * we need to make sure that it only traces functions it

@@ -2567,9 +2580,9 @@ void ftrace_modify_all_code(int command)
        }
 
        if (command & FTRACE_UPDATE_CALLS)
-               ftrace_replace_code(1);
+               ftrace_replace_code(mod_flags | FTRACE_MODIFY_ENABLE_FL);
        else if (command & FTRACE_DISABLE_CALLS)
-               ftrace_replace_code(0);
+               ftrace_replace_code(mod_flags);
 
        if (update && ftrace_trace_function != ftrace_ops_list_func) {
                function_trace_op = set_function_trace_op;
--

2.19.1

Steven Rostedt

author maintainer

2018-12-05 18:33 UTC #3 lore raw

PATCH 2/2 arm64: ftrace: Set FTRACE_SCHEDULABLE before ftrace_modify_all_code()

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

It has been reported that ftrace_replace_code() which is called by ftrace_modify_all_code() can cause a soft lockup warning for an allmodconfig kernel. This is because all the debug options enabled causes the loop in ftrace_replace_code() (which loops over all the functions being enabled where there can be 10s of thousands), is too slow, and never schedules out.

To solve this, setting FTRACE_SCHEDULABLE to the command passed into ftrace_replace_code() will make it call cond_resched() in the loop, which prevents the soft lockup warning from triggering.

Link http://lkml.kernel.org/r/[email protected] open here

Reported-by Anders Roxell <[email protected]>

Signed-off-by Steven Rostedt (VMware) <[email protected]>

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

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

index 57e962290df3..9a8de0a79f97 100644
@@ -193,6 +193,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 
 void arch_ftrace_update_code(int command)
 {
+	command |= FTRACE_SCHEDULABLE;
 	ftrace_modify_all_code(command);
 }

Signature · 1 line

2.19.1

Will Deacon

Acked-by on v1 2/2

2018-12-06 13:19 UTC #4 lore raw

Re: [PATCH 2/2] arm64: ftrace: Set FTRACE_SCHEDULABLE before ftrace_modify_all_code()

Quoted 26 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>
It has been reported that ftrace_replace_code() which is called by ftrace_modify_all_code() can cause a soft lockup warning for an allmodconfig kernel. This is because all the debug options enabled causes the loop in ftrace_replace_code() (which loops over all the functions being enabled where there can be 10s of thousands), is too slow, and never schedules out.
To solve this, setting FTRACE_SCHEDULABLE to the command passed into ftrace_replace_code() will make it call cond_resched() in the loop, which prevents the soft lockup warning from triggering.
Link http://lkml.kernel.org/r/[email protected] open here
Reported-by Anders Roxell <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>
---
 arch/arm64/kernel/ftrace.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/kernel/ftrace.c +0 -0
index 57e962290df3..9a8de0a79f97 100644

arch/arm64/kernel/ftrace.c int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, · line 198

Quoted 6 lines

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

@@ -193,6 +193,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 
 void arch_ftrace_update_code(int command)
 {
+	command |= FTRACE_SCHEDULABLE;
 	ftrace_modify_all_code(command);
 }

Bikeshed: I'd probably go for FTRACE_MAY_SLEEP, but I'm not going to die on that hill so...

Acked-by Will Deacon <[email protected]>

Thanks, Steve!

Will

Anders Roxell

Tested-by

2018-12-06 14:32 UTC #5 lore raw

Re: [PATCH 2/2] arm64: ftrace: Set FTRACE_SCHEDULABLE before ftrace_modify_all_code()

Quoted 38 lines · Will Deacon

Quoted 26 lines · Steven Rostedt
From: "Steven Rostedt (VMware)" <[email protected]>
It has been reported that ftrace_replace_code() which is called by ftrace_modify_all_code() can cause a soft lockup warning for an allmodconfig kernel. This is because all the debug options enabled causes the loop in ftrace_replace_code() (which loops over all the functions being enabled where there can be 10s of thousands), is too slow, and never schedules out.
To solve this, setting FTRACE_SCHEDULABLE to the command passed into ftrace_replace_code() will make it call cond_resched() in the loop, which prevents the soft lockup warning from triggering.
Link http://lkml.kernel.org/r/[email protected] open here
Reported-by Anders Roxell <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>
---
 arch/arm64/kernel/ftrace.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/kernel/ftrace.c +0 -0
index 57e962290df3..9a8de0a79f97 100644
arch/arm64/kernel/ftrace.c int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, · line 198
Quoted 6 lines
arch/arm64/kernel/ftrace.c +1 -0
@@ -193,6 +193,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 
 void arch_ftrace_update_code(int command)
 {
+     command |= FTRACE_SCHEDULABLE;
      ftrace_modify_all_code(command);
 }
Bikeshed: I'd probably go for FTRACE_MAY_SLEEP, but I'm not going to die on that hill so...

Quoted 1 line

Acked-by Will Deacon <[email protected]>

Tested-by Anders Roxell <[email protected]>

Cheers,
Anders

Quoted 4 lines

Thanks, Steve!
Will

Steven Rostedt

author maintainer

2018-12-06 15:59 UTC #6 lore raw

Re: [PATCH 2/2] arm64: ftrace: Set FTRACE_SCHEDULABLE before ftrace_modify_all_code()

In reply to Will Deacon

Quoted 28 lines · On Thu, 6 Dec 2018 13:20:07 +0000 Will Deacon <[email protected]> wrote:

Quoted 27 lines · Steven Rostedt
From: "Steven Rostedt (VMware)" <[email protected]>
It has been reported that ftrace_replace_code() which is called by ftrace_modify_all_code() can cause a soft lockup warning for an allmodconfig kernel. This is because all the debug options enabled causes the loop in ftrace_replace_code() (which loops over all the functions being enabled where there can be 10s of thousands), is too slow, and never schedules out.
To solve this, setting FTRACE_SCHEDULABLE to the command passed into ftrace_replace_code() will make it call cond_resched() in the loop, which prevents the soft lockup warning from triggering.
Link http://lkml.kernel.org/r/[email protected] open here
Reported-by Anders Roxell <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>
---
 arch/arm64/kernel/ftrace.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/kernel/ftrace.c +0 -0
index 57e962290df3..9a8de0a79f97 100644
@@ -193,6 +193,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 

Quoted 8 lines

arch/arm64/kernel/ftrace.c int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, · line 198
Quoted 5 lines
arch/arm64/kernel/ftrace.c +1 -0
 void arch_ftrace_update_code(int command)
 {
+	command |= FTRACE_SCHEDULABLE;
 	ftrace_modify_all_code(command);
 }  
Bikeshed: I'd probably go for FTRACE_MAY_SLEEP, but I'm not going to die on that hill so...

I like bike sheds. Hmm, it's not too late to change this. Perhaps I will.

Quoted 2 lines

Acked-by Will Deacon <[email protected]>

Thanks!

If I decide to change the name to MAY_SLEEP, I assume I can still keep your Acked-by.

-- Steve

Will Deacon

2018-12-06 16:06 UTC #7 lore raw

Re: [PATCH 2/2] arm64: ftrace: Set FTRACE_SCHEDULABLE before ftrace_modify_all_code()

Quoted 44 lines · Steven Rostedt

Quoted 28 lines · On Thu, 6 Dec 2018 13:20:07 +0000 Will Deacon <[email protected]> wrote:
Quoted 27 lines · Steven Rostedt
From: "Steven Rostedt (VMware)" <[email protected]>
It has been reported that ftrace_replace_code() which is called by ftrace_modify_all_code() can cause a soft lockup warning for an allmodconfig kernel. This is because all the debug options enabled causes the loop in ftrace_replace_code() (which loops over all the functions being enabled where there can be 10s of thousands), is too slow, and never schedules out.
To solve this, setting FTRACE_SCHEDULABLE to the command passed into ftrace_replace_code() will make it call cond_resched() in the loop, which prevents the soft lockup warning from triggering.
Link http://lkml.kernel.org/r/[email protected] open here
Reported-by Anders Roxell <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>
---
 arch/arm64/kernel/ftrace.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/kernel/ftrace.c +0 -0
index 57e962290df3..9a8de0a79f97 100644
@@ -193,6 +193,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 
Quoted 8 lines
arch/arm64/kernel/ftrace.c int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, · line 198
Quoted 5 lines
arch/arm64/kernel/ftrace.c +1 -0
 void arch_ftrace_update_code(int command)
 {
+	command |= FTRACE_SCHEDULABLE;
 	ftrace_modify_all_code(command);
 }  
Bikeshed: I'd probably go for FTRACE_MAY_SLEEP, but I'm not going to die on that hill so...
I like bike sheds. Hmm, it's not too late to change this. Perhaps I will.
Quoted 1 line

Quoted 6 lines

Quoted 1 line
Acked-by Will Deacon <[email protected]>
Thanks!
If I decide to change the name to MAY_SLEEP, I assume I can still keep your Acked-by.

Of course!

Will

Anders Roxell

2018-12-05 18:48 UTC #8 lore raw

In reply to Steven Rostedt

Quoted 7 lines · Steven Rostedt

This is a little more involved, and I would like to push this through my tree. Can I get a reviewed-by/ack for the second (arm64) patch?
Anders, can you also test this to make sure that it fixes the issue you see?

Yes of course, I'll try them.

Cheers,
Anders

Quoted 15 lines

Thanks!
-- Steve
Steven Rostedt (VMware) (2):
ftrace: Allow ftrace_replace_code() to be schedulable
arm64: ftrace: Set FTRACE_SCHEDULABLE before ftrace_modify_all_code()
----
 arch/arm64/kernel/ftrace.c |  1 +
 include/linux/ftrace.h     |  1 +
 kernel/trace/ftrace.c      | 19 ++++++++++++++++---
 3 files changed, 18 insertions(+), 3 deletions(-)