2023-03-31 12:54:00

by pengdonglin

[permalink] [raw]
Subject: [PATCH v10 0/8] function_graph: Support recording and printing the return value of function

When using the function_graph tracer to analyze system call failures,
it can be time-consuming to analyze the trace logs and locate the kernel
function that first returns an error. This change aims to simplify the
process by recording the function return value to the 'retval' member of
'ftrace_graph_ent' and printing it when outputing the trace log.

Note that even if a function's return type is void, a return value will
still be printed, so it should be ignored. If you care about this, the
BTF file can be used to obtain the details of function return type. We
can implement a tool to process the trace log and display the return
value based on its actual type.

Here is an example:

...

1) | cgroup_attach_task() {
1) | cgroup_migrate_add_src() {
1) 1.403 us | cset_cgroup_from_root(); /* = 0xffff93fc86f58010 */
1) 2.154 us | } /* cgroup_migrate_add_src = 0xffffb286c1297d00 */
1) ! 386.538 us | cgroup_migrate_prepare_dst(); /* = 0x0 */
1) | cgroup_migrate() {
1) 0.651 us | cgroup_migrate_add_task(); /* = 0xffff93fcfd346c00 */
1) | cgroup_migrate_execute() {
1) | cpu_cgroup_can_attach() {
1) | cgroup_taskset_first() {
1) 0.732 us | cgroup_taskset_next(); /* = 0xffff93fc8fb20000 */
1) 1.232 us | } /* cgroup_taskset_first = 0xffff93fc8fb20000 */
1) 0.380 us | sched_rt_can_attach(); /* = 0x0 */
1) 2.335 us | } /* cpu_cgroup_can_attach = -22 */
1) 4.369 us | } /* cgroup_migrate_execute = -22 */
1) 7.143 us | } /* cgroup_migrate = -22 */
1) | cgroup_migrate_finish() {
1) 0.411 us | put_css_set_locked(); /* = 0x8 */
1) + 62.397 us | put_css_set_locked(); /* = 0x80000001 */
1) + 64.742 us | } /* cgroup_migrate_finish = 0x80000000 */
1) ! 465.605 us | } /* cgroup_attach_task = -22 */

...

After processing the above trace logs using BTF information:

...

1) | cgroup_attach_task() {
1) | cgroup_migrate_add_src() {
1) 1.403 us | cset_cgroup_from_root(); /* = 0xffff93fc86f58010 */
1) 2.154 us | } /* cgroup_migrate_add_src */
1) ! 386.538 us | cgroup_migrate_prepare_dst(); /* = 0 */
1) | cgroup_migrate() {
1) 0.651 us | cgroup_migrate_add_task();
1) | cgroup_migrate_execute() {
1) | cpu_cgroup_can_attach() {
1) | cgroup_taskset_first() {
1) 0.732 us | cgroup_taskset_next(); /* = 0xffff93fc8fb20000 */
1) 1.232 us | } /* cgroup_taskset_first = 0xffff93fc8fb20000 */
1) 0.380 us | sched_rt_can_attach(); /* = 0 */
1) 2.335 us | } /* cpu_cgroup_can_attach = -22 */
1) 4.369 us | } /* cgroup_migrate_execute = -22 */
1) 7.143 us | } /* cgroup_migrate = -22 */
1) | cgroup_migrate_finish() {
1) 0.411 us | put_css_set_locked();
1) + 62.397 us | put_css_set_locked();
1) + 64.742 us | } /* cgroup_migrate_finish */
1) ! 465.605 us | } /* cgroup_attach_task = -22 */

...

---
v10:
- Fix code style issues for LoongArch
- Fix selftest issues
- Use CONFIG_FUNCTION_GRAPH_TRACER to control fgraph_ret_regs definition

v9:
- Fix align issues in ARM asm code
- Fix align issues in LoongArch asm code
- Update commit messages
- Update comments for ftrace_return_to_handler

v8:
- Fix issues in ARM64 asm code
- Fix issues in selftest
- Add some comments on CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
- Make CONFIG_FUNCTION_GRAPH_RETVAL switable
- Modify the control range of CONFIG_HAVE_FUNCTION_GRAPH_RETVAL

v7:
- Rename trace option 'graph_retval_hex' to 'funcgraph-retval-hex'
- Introduce a new structure fgraph_ret_regs for each architecture to
hold return registers
- Separate each architecture modification info individual patches
- Add a test case for funcgraph-retval
- Update documentation description
- Support LoongArch

v6:
- Remove the conversion code for short and char types, because these
two types are rarely used to store an error code.
- Modify the limitations for funcgraph-retval
- Optimize the English expression

v5:
- Pass both the return values to ftrace_return_to_handler
- Modify the parameter sequence of ftrace_return_to_handler to
decrease the modification of assembly code, thanks to Russell King
- Wrap __ftrace_return_to_handler with ftrace_return_to_handler
for compatible
- Describe the limitations of funcgraph-retval

v4:
- Modify commit message
- Introduce new option graph_retval_hex to control display format
- Introduce macro CONFIG_FUNCTION_GRAPH_RETVAL and
CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
- Add related arch maintainers to review

v3:
- Modify the commit message: add trace logs processed with the btf tool

v2:
- Modify the commit message: use BTF to get the return type of function

Donglin Peng (8):
function_graph: Support recording and printing the return value of
function
tracing: Add documentation for funcgraph-retval and
funcgraph-retval-hex
ARM: ftrace: Enable HAVE_FUNCTION_GRAPH_RETVAL
arm64: ftrace: Enable HAVE_FUNCTION_GRAPH_RETVAL
riscv: ftrace: Enable HAVE_FUNCTION_GRAPH_RETVAL
x86/ftrace: Enable HAVE_FUNCTION_GRAPH_RETVAL
LoongArch: ftrace: Enable HAVE_FUNCTION_GRAPH_RETVAL
selftests/ftrace: Add funcgraph-retval test case

Documentation/trace/ftrace.rst | 74 +++++++++++++++
arch/arm/Kconfig | 1 +
arch/arm/include/asm/ftrace.h | 22 +++++
arch/arm/kernel/asm-offsets.c | 8 +-
arch/arm/kernel/entry-ftrace.S | 10 +-
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/ftrace.h | 22 +++++
arch/arm64/kernel/asm-offsets.c | 13 +++
arch/arm64/kernel/entry-ftrace.S | 27 +++---
arch/loongarch/Kconfig | 1 +
arch/loongarch/include/asm/ftrace.h | 22 +++++
arch/loongarch/kernel/asm-offsets.c | 15 ++-
arch/loongarch/kernel/mcount.S | 14 +--
arch/loongarch/kernel/mcount_dyn.S | 15 +--
arch/riscv/Kconfig | 1 +
arch/riscv/include/asm/ftrace.h | 21 +++++
arch/riscv/kernel/mcount.S | 7 +-
arch/x86/Kconfig | 1 +
arch/x86/include/asm/ftrace.h | 20 ++++
arch/x86/kernel/ftrace_32.S | 8 +-
arch/x86/kernel/ftrace_64.S | 7 +-
include/linux/ftrace.h | 3 +
kernel/trace/Kconfig | 15 +++
kernel/trace/fgraph.c | 23 ++++-
kernel/trace/trace.h | 2 +
kernel/trace/trace_entries.h | 26 ++++++
kernel/trace/trace_functions_graph.c | 93 +++++++++++++++++--
.../ftrace/test.d/ftrace/fgraph-retval.tc | 43 +++++++++
28 files changed, 460 insertions(+), 55 deletions(-)
create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc

--
2.25.1


2023-03-31 12:54:01

by pengdonglin

[permalink] [raw]
Subject: [PATCH v10 8/8] selftests/ftrace: Add funcgraph-retval test case

Add a test case for the funcgraph-retval and funcgraph-retval-hex
trace options.

Signed-off-by: Donglin Peng <[email protected]>
---
v10:
- Fix issues in selftest

v8:
- Fix issues in selftest
---
.../ftrace/test.d/ftrace/fgraph-retval.tc | 43 +++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
new file mode 100644
index 000000000000..5819aa2dd6ad
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
@@ -0,0 +1,43 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: ftrace - function graph print function return value
+# requires: options/funcgraph-retval options/funcgraph-retval-hex function_graph:tracer
+
+# Make sure that funcgraph-retval works
+
+fail() { # msg
+ echo $1
+ exit_fail
+}
+
+disable_tracing
+clear_trace
+
+read PID _ < /proc/self/stat
+[ -f set_ftrace_pid ] && echo ${PID} > set_ftrace_pid
+[ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
+[ -f set_graph_function ] && echo proc_reg_write > set_graph_function
+echo function_graph > current_tracer
+echo funcgraph-retval > trace_options
+
+set +e
+enable_tracing
+echo > /proc/interrupts
+disable_tracing
+set -e
+
+: "Test printing the error code in signed decimal format"
+echo nofuncgraph-retval-hex > trace_options
+count=`cat trace | grep 'proc_reg_write' | grep '= -5' | wc -l`
+if [ $count -eq 0 ]; then
+ fail "Return value can not be printed in signed decimal format"
+fi
+
+: "Test printing the error code in hexadecimal format"
+echo funcgraph-retval-hex > trace_options
+count=`cat trace | grep 'proc_reg_write' | grep 'fffffffb' | wc -l`
+if [ $count -eq 0 ]; then
+ fail "Return value can not be printed in hexadecimal format"
+fi
+
+exit 0
--
2.25.1

2023-03-31 12:54:44

by pengdonglin

[permalink] [raw]
Subject: [PATCH v10 7/8] LoongArch: ftrace: Enable HAVE_FUNCTION_GRAPH_RETVAL

The previous patch ("function_graph: Support recording and printing
the return value of function") has laid the groundwork for the for
the funcgraph-retval, and this modification makes it available on
the LoongArch platform.

We introduce a new structure called fgraph_ret_regs for the LoongArch
platform to hold return registers and the frame pointer. We then fill
its content in the return_to_handler and pass its address to the
function ftrace_return_to_handler to record the return value.

Signed-off-by: Donglin Peng <[email protected]>
---
v10:
- Fix code style issues for LoongArch
- Use CONFIG_FUNCTION_GRAPH_TRACER to control fgraph_ret_regs definition

v9:
- Fix stack pointer align issues
- Update the commit message

v8:
- Modify the control range of CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
---
arch/loongarch/Kconfig | 1 +
arch/loongarch/include/asm/ftrace.h | 22 ++++++++++++++++++++++
arch/loongarch/kernel/asm-offsets.c | 15 ++++++++++++++-
arch/loongarch/kernel/mcount.S | 14 ++++++++------
arch/loongarch/kernel/mcount_dyn.S | 15 ++++++++-------
5 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index 7fd51257e0ed..4bf60132869b 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -99,6 +99,7 @@ config LOONGARCH
select HAVE_FAST_GUP
select HAVE_FTRACE_MCOUNT_RECORD
select HAVE_FUNCTION_ARG_ACCESS_API
+ select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
select HAVE_FUNCTION_GRAPH_TRACER
select HAVE_FUNCTION_TRACER
select HAVE_GENERIC_VDSO
diff --git a/arch/loongarch/include/asm/ftrace.h b/arch/loongarch/include/asm/ftrace.h
index 3418d32d4fc7..22797b7504b5 100644
--- a/arch/loongarch/include/asm/ftrace.h
+++ b/arch/loongarch/include/asm/ftrace.h
@@ -63,4 +63,26 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,

#endif /* CONFIG_FUNCTION_TRACER */

+#ifndef __ASSEMBLY__
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+struct fgraph_ret_regs {
+ /* a0 - a1 */
+ unsigned long regs[2];
+
+ unsigned long fp;
+ unsigned long __unused;
+};
+
+static inline unsigned long fgraph_ret_regs_return_value(struct fgraph_ret_regs *ret_regs)
+{
+ return ret_regs->regs[0];
+}
+
+static inline unsigned long fgraph_ret_regs_frame_pointer(struct fgraph_ret_regs *ret_regs)
+{
+ return ret_regs->fp;
+}
+#endif /* ifdef CONFIG_FUNCTION_GRAPH_TRACER */
+#endif
+
#endif /* _ASM_LOONGARCH_FTRACE_H */
diff --git a/arch/loongarch/kernel/asm-offsets.c b/arch/loongarch/kernel/asm-offsets.c
index 4bdb203fc66e..505e4bf59603 100644
--- a/arch/loongarch/kernel/asm-offsets.c
+++ b/arch/loongarch/kernel/asm-offsets.c
@@ -12,6 +12,7 @@
#include <asm/cpu-info.h>
#include <asm/ptrace.h>
#include <asm/processor.h>
+#include <asm/ftrace.h>

void output_ptreg_defines(void)
{
@@ -264,7 +265,7 @@ void output_smpboot_defines(void)
#ifdef CONFIG_HIBERNATION
void output_pbe_defines(void)
{
- COMMENT(" Linux struct pbe offsets. ");
+ COMMENT("Linux struct pbe offsets.");
OFFSET(PBE_ADDRESS, pbe, address);
OFFSET(PBE_ORIG_ADDRESS, pbe, orig_address);
OFFSET(PBE_NEXT, pbe, next);
@@ -272,3 +273,15 @@ void output_pbe_defines(void)
BLANK();
}
#endif
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+void output_fgraph_ret_regs_defines(void)
+{
+ COMMENT("LoongArch fgraph_ret_regs offsets.");
+ OFFSET(FGRET_REGS_A0, fgraph_ret_regs, regs[0]);
+ OFFSET(FGRET_REGS_A1, fgraph_ret_regs, regs[1]);
+ OFFSET(FGRET_REGS_FP, fgraph_ret_regs, fp);
+ DEFINE(FGRET_REGS_SIZE, sizeof(struct fgraph_ret_regs));
+ BLANK();
+}
+#endif
diff --git a/arch/loongarch/kernel/mcount.S b/arch/loongarch/kernel/mcount.S
index 8cdc1563cd33..cb8e5803de4b 100644
--- a/arch/loongarch/kernel/mcount.S
+++ b/arch/loongarch/kernel/mcount.S
@@ -79,18 +79,20 @@ SYM_FUNC_START(ftrace_graph_caller)
SYM_FUNC_END(ftrace_graph_caller)

SYM_FUNC_START(return_to_handler)
- PTR_ADDI sp, sp, -2 * SZREG
- PTR_S a0, sp, 0
- PTR_S a1, sp, SZREG
+ PTR_ADDI sp, sp, -FGRET_REGS_SIZE
+ PTR_S a0, sp, FGRET_REGS_A0
+ PTR_S a1, sp, FGRET_REGS_A1
+ PTR_S zero, sp, FGRET_REGS_FP

+ move a0, sp
bl ftrace_return_to_handler

/* Restore the real parent address: a0 -> ra */
move ra, a0

- PTR_L a0, sp, 0
- PTR_L a1, sp, SZREG
- PTR_ADDI sp, sp, 2 * SZREG
+ PTR_L a0, sp, FGRET_REGS_A0
+ PTR_L a1, sp, FGRET_REGS_A1
+ PTR_ADDI sp, sp, FGRET_REGS_SIZE
jr ra
SYM_FUNC_END(return_to_handler)
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/loongarch/kernel/mcount_dyn.S b/arch/loongarch/kernel/mcount_dyn.S
index bbabf06244c2..ec24ae1de741 100644
--- a/arch/loongarch/kernel/mcount_dyn.S
+++ b/arch/loongarch/kernel/mcount_dyn.S
@@ -131,18 +131,19 @@ SYM_CODE_END(ftrace_graph_caller)

SYM_CODE_START(return_to_handler)
/* Save return value regs */
- PTR_ADDI sp, sp, -2 * SZREG
- PTR_S a0, sp, 0
- PTR_S a1, sp, SZREG
+ PTR_ADDI sp, sp, -FGRET_REGS_SIZE
+ PTR_S a0, sp, FGRET_REGS_A0
+ PTR_S a1, sp, FGRET_REGS_A1
+ PTR_S zero, sp, FGRET_REGS_FP

- move a0, zero
+ move a0, sp
bl ftrace_return_to_handler
move ra, a0

/* Restore return value regs */
- PTR_L a0, sp, 0
- PTR_L a1, sp, SZREG
- PTR_ADDI sp, sp, 2 * SZREG
+ PTR_L a0, sp, FGRET_REGS_A0
+ PTR_L a1, sp, FGRET_REGS_A1
+ PTR_ADDI sp, sp, FGRET_REGS_SIZE

jr ra
SYM_CODE_END(return_to_handler)
--
2.25.1

2023-03-31 12:56:17

by pengdonglin

[permalink] [raw]
Subject: [PATCH v10 1/8] function_graph: Support recording and printing the return value of function

Analyzing system call failures with the function_graph tracer can be a
time-consuming process, particularly when locating the kernel function
that first returns an error in the trace logs. This change aims to
simplify the process by recording the function return value to the
'retval' member of 'ftrace_graph_ent' and printing it when outputting
the trace log.

We have introduced new trace options: funcgraph-retval and
funcgraph-retval-hex. The former controls whether to display the return
value, while the latter controls the display format.

Please note that even if a function's return type is void, a return
value will still be printed. You can simply ignore it.

This patch only establishes the fundamental infrastructure. Subsequent
patches will make this feature available on some commonly used processor
architectures.

Here is an example:

I attempted to attach the demo process to a cpu cgroup, but it failed:

echo `pidof demo` > /sys/fs/cgroup/cpu/test/tasks
-bash: echo: write error: Invalid argument

The strace logs indicate that the write system call returned -EINVAL(-22):
...
write(1, "273\n", 4) = -1 EINVAL (Invalid argument)
...

To capture trace logs during a write system call, use the following
commands:

cd /sys/kernel/debug/tracing/
echo 0 > tracing_on
echo > trace
echo *sys_write > set_graph_function
echo *spin* > set_graph_notrace
echo *rcu* >> set_graph_notrace
echo *alloc* >> set_graph_notrace
echo preempt* >> set_graph_notrace
echo kfree* >> set_graph_notrace
echo $$ > set_ftrace_pid
echo function_graph > current_tracer
echo 1 > options/funcgraph-retval
echo 0 > options/funcgraph-retval-hex
echo 1 > tracing_on
echo `pidof demo` > /sys/fs/cgroup/cpu/test/tasks
echo 0 > tracing_on
cat trace > ~/trace.log

To locate the root cause, search for error code -22 directly in the file
trace.log and identify the first function that returned -22. Once you
have identified this function, examine its code to determine the root
cause.

For example, in the trace log below, cpu_cgroup_can_attach
returned -22 first, so we can focus our analysis on this function to
identify the root cause.

...

1) | cgroup_migrate() {
1) 0.651 us | cgroup_migrate_add_task(); /* = 0xffff93fcfd346c00 */
1) | cgroup_migrate_execute() {
1) | cpu_cgroup_can_attach() {
1) | cgroup_taskset_first() {
1) 0.732 us | cgroup_taskset_next(); /* = 0xffff93fc8fb20000 */
1) 1.232 us | } /* cgroup_taskset_first = 0xffff93fc8fb20000 */
1) 0.380 us | sched_rt_can_attach(); /* = 0x0 */
1) 2.335 us | } /* cpu_cgroup_can_attach = -22 */
1) 4.369 us | } /* cgroup_migrate_execute = -22 */
1) 7.143 us | } /* cgroup_migrate = -22 */

...

Tested-by: Florian Kauer <[email protected]>
Acked-by: Masami Hiramatsu (Google) <[email protected]>
Signed-off-by: Donglin Peng <[email protected]>
---
v9:
- Update comments for ftrace_return_to_handler

v8:
- Add some comments on CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
- Make CONFIG_FUNCTION_GRAPH_RETVAL switable

v7:
- Rename trace option 'graph_retval_hex' to 'funcgraph-retval-hex'
- Separate each architecture modification info individual patches
- Introduce a new structure fgraph_ret_regs for each architecture to
hold return registers

v6:
- Remove the conversion code for short and char types, because these
two types are rarely used to store an error code.

v5:
- Pass both the return values to ftrace_return_to_handler
- Modify the parameter sequence of ftrace_return_to_handler to
decrease the modification of assembly code, thanks to Russell King
- Wrap __ftrace_return_to_handler with ftrace_return_to_handler
for compatible

v4:
- Modify commit message
- Introduce new option graph_retval_hex to control display format
- Introduce macro CONFIG_FUNCTION_GRAPH_RETVAL and
CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
- Add related arch maintainers to review

v3:
- Modify the commit message: add trace logs processed with the btf tool

v2:
- Modify the commit message: use BTF to get the return type of function
---
include/linux/ftrace.h | 3 +
kernel/trace/Kconfig | 15 +++++
kernel/trace/fgraph.c | 23 ++++++-
kernel/trace/trace.h | 2 +
kernel/trace/trace_entries.h | 26 ++++++++
kernel/trace/trace_functions_graph.c | 93 +++++++++++++++++++++++++---
6 files changed, 151 insertions(+), 11 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 366c730beaa3..be662a11cd13 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1032,6 +1032,9 @@ struct ftrace_graph_ent {
*/
struct ftrace_graph_ret {
unsigned long func; /* Current function */
+#ifdef CONFIG_FUNCTION_GRAPH_RETVAL
+ unsigned long retval;
+#endif
int depth;
/* Number of functions that overran the depth limit for current task */
unsigned int overrun;
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index a856d4a34c67..87de5964c81d 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -31,6 +31,9 @@ config HAVE_FUNCTION_GRAPH_TRACER
help
See Documentation/trace/ftrace-design.rst

+config HAVE_FUNCTION_GRAPH_RETVAL
+ bool
+
config HAVE_DYNAMIC_FTRACE
bool
help
@@ -227,6 +230,18 @@ config FUNCTION_GRAPH_TRACER
the return value. This is done by setting the current return
address on the current task structure into a stack of calls.

+config FUNCTION_GRAPH_RETVAL
+ bool "Function Graph Return Value"
+ depends on HAVE_FUNCTION_GRAPH_RETVAL
+ depends on FUNCTION_GRAPH_TRACER
+ default n
+ help
+ Support recording and printing the function return value when
+ using function graph tracer. It can be helpful to locate functions
+ that return errors. This feature is off by default, and you can
+ enable it via the trace option funcgraph-retval.
+ See Documentation/trace/ftrace.rst
+
config DYNAMIC_FTRACE
bool "enable/disable function tracing dynamically"
depends on FUNCTION_TRACER
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 218cd95bf8e4..170ec9429efb 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -240,12 +240,16 @@ static struct notifier_block ftrace_suspend_notifier = {
* Send the trace to the ring-buffer.
* @return the original return address.
*/
-unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
+static unsigned long __ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs,
+ unsigned long frame_pointer)
{
struct ftrace_graph_ret trace;
unsigned long ret;

ftrace_pop_return_trace(&trace, &ret, frame_pointer);
+#ifdef CONFIG_FUNCTION_GRAPH_RETVAL
+ trace.retval = fgraph_ret_regs_return_value(ret_regs);
+#endif
trace.rettime = trace_clock_local();
ftrace_graph_return(&trace);
/*
@@ -266,6 +270,23 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
return ret;
}

+/*
+ * After all architecures have selected HAVE_FUNCTION_GRAPH_RETVAL, we can
+ * leave only ftrace_return_to_handler(ret_regs).
+ */
+#ifdef CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
+unsigned long ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs)
+{
+ return __ftrace_return_to_handler(ret_regs,
+ fgraph_ret_regs_frame_pointer(ret_regs));
+}
+#else
+unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
+{
+ return __ftrace_return_to_handler(NULL, frame_pointer);
+}
+#endif
+
/**
* ftrace_graph_get_ret_stack - return the entry of the shadow stack
* @task: The task to read the shadow stack from
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 616e1aa1c4da..0a3c4582df10 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -831,6 +831,8 @@ static __always_inline bool ftrace_hash_empty(struct ftrace_hash *hash)
#define TRACE_GRAPH_PRINT_TAIL 0x100
#define TRACE_GRAPH_SLEEP_TIME 0x200
#define TRACE_GRAPH_GRAPH_TIME 0x400
+#define TRACE_GRAPH_PRINT_RETVAL 0x800
+#define TRACE_GRAPH_PRINT_RETVAL_HEX 0x1000
#define TRACE_GRAPH_PRINT_FILL_SHIFT 28
#define TRACE_GRAPH_PRINT_FILL_MASK (0x3 << TRACE_GRAPH_PRINT_FILL_SHIFT)

diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index cd41e863b51c..340b2fa98218 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -86,6 +86,30 @@ FTRACE_ENTRY_PACKED(funcgraph_entry, ftrace_graph_ent_entry,
);

/* Function return entry */
+#ifdef CONFIG_FUNCTION_GRAPH_RETVAL
+
+FTRACE_ENTRY_PACKED(funcgraph_exit, ftrace_graph_ret_entry,
+
+ TRACE_GRAPH_RET,
+
+ F_STRUCT(
+ __field_struct( struct ftrace_graph_ret, ret )
+ __field_packed( unsigned long, ret, func )
+ __field_packed( unsigned long, ret, retval )
+ __field_packed( int, ret, depth )
+ __field_packed( unsigned int, ret, overrun )
+ __field_packed( unsigned long long, ret, calltime)
+ __field_packed( unsigned long long, ret, rettime )
+ ),
+
+ F_printk("<-- %ps (%d) (start: %llx end: %llx) over: %d retval: %lx",
+ (void *)__entry->func, __entry->depth,
+ __entry->calltime, __entry->rettime,
+ __entry->depth, __entry->retval)
+);
+
+#else
+
FTRACE_ENTRY_PACKED(funcgraph_exit, ftrace_graph_ret_entry,

TRACE_GRAPH_RET,
@@ -105,6 +129,8 @@ FTRACE_ENTRY_PACKED(funcgraph_exit, ftrace_graph_ret_entry,
__entry->depth)
);

+#endif
+
/*
* Context switch trace entry - which task (and prio) we switched from/to:
*
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 203204cadf92..c35fbaab2a47 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -58,6 +58,12 @@ static struct tracer_opt trace_opts[] = {
{ TRACER_OPT(funcgraph-irqs, TRACE_GRAPH_PRINT_IRQS) },
/* Display function name after trailing } */
{ TRACER_OPT(funcgraph-tail, TRACE_GRAPH_PRINT_TAIL) },
+#ifdef CONFIG_FUNCTION_GRAPH_RETVAL
+ /* Display function return value ? */
+ { TRACER_OPT(funcgraph-retval, TRACE_GRAPH_PRINT_RETVAL) },
+ /* Display function return value in hexadecimal format ? */
+ { TRACER_OPT(funcgraph-retval-hex, TRACE_GRAPH_PRINT_RETVAL_HEX) },
+#endif
/* Include sleep time (scheduled out) between entry and return */
{ TRACER_OPT(sleep-time, TRACE_GRAPH_SLEEP_TIME) },

@@ -619,6 +625,56 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
trace_seq_puts(s, "| ");
}

+#ifdef CONFIG_FUNCTION_GRAPH_RETVAL
+
+#define __TRACE_GRAPH_PRINT_RETVAL TRACE_GRAPH_PRINT_RETVAL
+
+static void print_graph_retval(struct trace_seq *s, unsigned long retval,
+ bool leaf, void *func, bool hex_format)
+{
+ unsigned long err_code = 0;
+
+ if (retval == 0 || hex_format)
+ goto done;
+
+ /* Check if the return value matches the negative format */
+ if (IS_ENABLED(CONFIG_64BIT) && (retval & BIT(31)) &&
+ (((u64)retval) >> 32) == 0) {
+ /* sign extension */
+ err_code = (unsigned long)(s32)retval;
+ } else {
+ err_code = retval;
+ }
+
+ if (!IS_ERR_VALUE(err_code))
+ err_code = 0;
+
+done:
+ if (leaf) {
+ if (hex_format || (err_code == 0))
+ trace_seq_printf(s, "%ps(); /* = 0x%lx */\n",
+ func, retval);
+ else
+ trace_seq_printf(s, "%ps(); /* = %ld */\n",
+ func, err_code);
+ } else {
+ if (hex_format || (err_code == 0))
+ trace_seq_printf(s, "} /* %ps = 0x%lx */\n",
+ func, retval);
+ else
+ trace_seq_printf(s, "} /* %ps = %ld */\n",
+ func, err_code);
+ }
+}
+
+#else
+
+#define __TRACE_GRAPH_PRINT_RETVAL 0
+
+#define print_graph_retval(_seq, _retval, _leaf, _func, _format) do {} while (0)
+
+#endif
+
/* Case of a leaf function on its call entry */
static enum print_line_t
print_graph_entry_leaf(struct trace_iterator *iter,
@@ -663,7 +719,15 @@ print_graph_entry_leaf(struct trace_iterator *iter,
for (i = 0; i < call->depth * TRACE_GRAPH_INDENT; i++)
trace_seq_putc(s, ' ');

- trace_seq_printf(s, "%ps();\n", (void *)call->func);
+ /*
+ * Write out the function return value if the option function-retval is
+ * enabled.
+ */
+ if (flags & __TRACE_GRAPH_PRINT_RETVAL)
+ print_graph_retval(s, graph_ret->retval, true, (void *)call->func,
+ !!(flags & TRACE_GRAPH_PRINT_RETVAL_HEX));
+ else
+ trace_seq_printf(s, "%ps();\n", (void *)call->func);

print_graph_irq(iter, graph_ret->func, TRACE_GRAPH_RET,
cpu, iter->ent->pid, flags);
@@ -942,16 +1006,25 @@ print_graph_return(struct ftrace_graph_ret *trace, struct trace_seq *s,
trace_seq_putc(s, ' ');

/*
- * If the return function does not have a matching entry,
- * then the entry was lost. Instead of just printing
- * the '}' and letting the user guess what function this
- * belongs to, write out the function name. Always do
- * that if the funcgraph-tail option is enabled.
+ * Always write out the function name and its return value if the
+ * function-retval option is enabled.
*/
- if (func_match && !(flags & TRACE_GRAPH_PRINT_TAIL))
- trace_seq_puts(s, "}\n");
- else
- trace_seq_printf(s, "} /* %ps */\n", (void *)trace->func);
+ if (flags & __TRACE_GRAPH_PRINT_RETVAL) {
+ print_graph_retval(s, trace->retval, false, (void *)trace->func,
+ !!(flags & TRACE_GRAPH_PRINT_RETVAL_HEX));
+ } else {
+ /*
+ * If the return function does not have a matching entry,
+ * then the entry was lost. Instead of just printing
+ * the '}' and letting the user guess what function this
+ * belongs to, write out the function name. Always do
+ * that if the funcgraph-tail option is enabled.
+ */
+ if (func_match && !(flags & TRACE_GRAPH_PRINT_TAIL))
+ trace_seq_puts(s, "}\n");
+ else
+ trace_seq_printf(s, "} /* %ps */\n", (void *)trace->func);
+ }

/* Overrun */
if (flags & TRACE_GRAPH_PRINT_OVERRUN)
--
2.25.1

2023-04-01 22:08:41

by Masami Hiramatsu

[permalink] [raw]
Subject: Re: [PATCH v10 8/8] selftests/ftrace: Add funcgraph-retval test case

On Fri, 31 Mar 2023 05:47:44 -0700
Donglin Peng <[email protected]> wrote:

> Add a test case for the funcgraph-retval and funcgraph-retval-hex
> trace options.
>
> Signed-off-by: Donglin Peng <[email protected]>
> ---
> v10:
> - Fix issues in selftest
>
> v8:
> - Fix issues in selftest
> ---
> .../ftrace/test.d/ftrace/fgraph-retval.tc | 43 +++++++++++++++++++
> 1 file changed, 43 insertions(+)
> create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>
> diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
> new file mode 100644
> index 000000000000..5819aa2dd6ad
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: ftrace - function graph print function return value
> +# requires: options/funcgraph-retval options/funcgraph-retval-hex function_graph:tracer
> +
> +# Make sure that funcgraph-retval works
> +
> +fail() { # msg
> + echo $1
> + exit_fail
> +}
> +
> +disable_tracing
> +clear_trace
> +
> +read PID _ < /proc/self/stat

You can use "$$" for self pid.

> +[ -f set_ftrace_pid ] && echo ${PID} > set_ftrace_pid
> +[ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
> +[ -f set_graph_function ] && echo proc_reg_write > set_graph_function

You should set the required files for this test, so that the test result
is always same. BTW, you need to set either 'set_ftrace_filter' or
'set_graph_function'.

> +echo function_graph > current_tracer
> +echo funcgraph-retval > trace_options
> +
> +set +e
> +enable_tracing
> +echo > /proc/interrupts
> +disable_tracing
> +set -e
> +
> +: "Test printing the error code in signed decimal format"
> +echo nofuncgraph-retval-hex > trace_options

echo 0 > options/funcgraph-retval-hex

If you require 'options/funcgraph-retval-hex' file, you can use the
file to set it or clear it.

> +count=`cat trace | grep 'proc_reg_write' | grep '= -5' | wc -l`
> +if [ $count -eq 0 ]; then
> + fail "Return value can not be printed in signed decimal format"
> +fi
> +
> +: "Test printing the error code in hexadecimal format"
> +echo funcgraph-retval-hex > trace_options

Ditto.

Thanks,

> +count=`cat trace | grep 'proc_reg_write' | grep 'fffffffb' | wc -l`
> +if [ $count -eq 0 ]; then
> + fail "Return value can not be printed in hexadecimal format"
> +fi
> +
> +exit 0
> --
> 2.25.1
>


--
Masami Hiramatsu (Google) <[email protected]>

2023-04-03 00:55:22

by Huacai Chen

[permalink] [raw]
Subject: Re: [PATCH v10 7/8] LoongArch: ftrace: Enable HAVE_FUNCTION_GRAPH_RETVAL

Reviewed-by: Huacai Chen <[email protected]>

On Fri, Mar 31, 2023 at 8:48 PM Donglin Peng <[email protected]> wrote:
>
> The previous patch ("function_graph: Support recording and printing
> the return value of function") has laid the groundwork for the for
> the funcgraph-retval, and this modification makes it available on
> the LoongArch platform.
>
> We introduce a new structure called fgraph_ret_regs for the LoongArch
> platform to hold return registers and the frame pointer. We then fill
> its content in the return_to_handler and pass its address to the
> function ftrace_return_to_handler to record the return value.
>
> Signed-off-by: Donglin Peng <[email protected]>
> ---
> v10:
> - Fix code style issues for LoongArch
> - Use CONFIG_FUNCTION_GRAPH_TRACER to control fgraph_ret_regs definition
>
> v9:
> - Fix stack pointer align issues
> - Update the commit message
>
> v8:
> - Modify the control range of CONFIG_HAVE_FUNCTION_GRAPH_RETVAL
> ---
> arch/loongarch/Kconfig | 1 +
> arch/loongarch/include/asm/ftrace.h | 22 ++++++++++++++++++++++
> arch/loongarch/kernel/asm-offsets.c | 15 ++++++++++++++-
> arch/loongarch/kernel/mcount.S | 14 ++++++++------
> arch/loongarch/kernel/mcount_dyn.S | 15 ++++++++-------
> 5 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index 7fd51257e0ed..4bf60132869b 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -99,6 +99,7 @@ config LOONGARCH
> select HAVE_FAST_GUP
> select HAVE_FTRACE_MCOUNT_RECORD
> select HAVE_FUNCTION_ARG_ACCESS_API
> + select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
> select HAVE_FUNCTION_GRAPH_TRACER
> select HAVE_FUNCTION_TRACER
> select HAVE_GENERIC_VDSO
> diff --git a/arch/loongarch/include/asm/ftrace.h b/arch/loongarch/include/asm/ftrace.h
> index 3418d32d4fc7..22797b7504b5 100644
> --- a/arch/loongarch/include/asm/ftrace.h
> +++ b/arch/loongarch/include/asm/ftrace.h
> @@ -63,4 +63,26 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>
> #endif /* CONFIG_FUNCTION_TRACER */
>
> +#ifndef __ASSEMBLY__
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +struct fgraph_ret_regs {
> + /* a0 - a1 */
> + unsigned long regs[2];
> +
> + unsigned long fp;
> + unsigned long __unused;
> +};
> +
> +static inline unsigned long fgraph_ret_regs_return_value(struct fgraph_ret_regs *ret_regs)
> +{
> + return ret_regs->regs[0];
> +}
> +
> +static inline unsigned long fgraph_ret_regs_frame_pointer(struct fgraph_ret_regs *ret_regs)
> +{
> + return ret_regs->fp;
> +}
> +#endif /* ifdef CONFIG_FUNCTION_GRAPH_TRACER */
> +#endif
> +
> #endif /* _ASM_LOONGARCH_FTRACE_H */
> diff --git a/arch/loongarch/kernel/asm-offsets.c b/arch/loongarch/kernel/asm-offsets.c
> index 4bdb203fc66e..505e4bf59603 100644
> --- a/arch/loongarch/kernel/asm-offsets.c
> +++ b/arch/loongarch/kernel/asm-offsets.c
> @@ -12,6 +12,7 @@
> #include <asm/cpu-info.h>
> #include <asm/ptrace.h>
> #include <asm/processor.h>
> +#include <asm/ftrace.h>
>
> void output_ptreg_defines(void)
> {
> @@ -264,7 +265,7 @@ void output_smpboot_defines(void)
> #ifdef CONFIG_HIBERNATION
> void output_pbe_defines(void)
> {
> - COMMENT(" Linux struct pbe offsets. ");
> + COMMENT("Linux struct pbe offsets.");
> OFFSET(PBE_ADDRESS, pbe, address);
> OFFSET(PBE_ORIG_ADDRESS, pbe, orig_address);
> OFFSET(PBE_NEXT, pbe, next);
> @@ -272,3 +273,15 @@ void output_pbe_defines(void)
> BLANK();
> }
> #endif
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +void output_fgraph_ret_regs_defines(void)
> +{
> + COMMENT("LoongArch fgraph_ret_regs offsets.");
> + OFFSET(FGRET_REGS_A0, fgraph_ret_regs, regs[0]);
> + OFFSET(FGRET_REGS_A1, fgraph_ret_regs, regs[1]);
> + OFFSET(FGRET_REGS_FP, fgraph_ret_regs, fp);
> + DEFINE(FGRET_REGS_SIZE, sizeof(struct fgraph_ret_regs));
> + BLANK();
> +}
> +#endif
> diff --git a/arch/loongarch/kernel/mcount.S b/arch/loongarch/kernel/mcount.S
> index 8cdc1563cd33..cb8e5803de4b 100644
> --- a/arch/loongarch/kernel/mcount.S
> +++ b/arch/loongarch/kernel/mcount.S
> @@ -79,18 +79,20 @@ SYM_FUNC_START(ftrace_graph_caller)
> SYM_FUNC_END(ftrace_graph_caller)
>
> SYM_FUNC_START(return_to_handler)
> - PTR_ADDI sp, sp, -2 * SZREG
> - PTR_S a0, sp, 0
> - PTR_S a1, sp, SZREG
> + PTR_ADDI sp, sp, -FGRET_REGS_SIZE
> + PTR_S a0, sp, FGRET_REGS_A0
> + PTR_S a1, sp, FGRET_REGS_A1
> + PTR_S zero, sp, FGRET_REGS_FP
>
> + move a0, sp
> bl ftrace_return_to_handler
>
> /* Restore the real parent address: a0 -> ra */
> move ra, a0
>
> - PTR_L a0, sp, 0
> - PTR_L a1, sp, SZREG
> - PTR_ADDI sp, sp, 2 * SZREG
> + PTR_L a0, sp, FGRET_REGS_A0
> + PTR_L a1, sp, FGRET_REGS_A1
> + PTR_ADDI sp, sp, FGRET_REGS_SIZE
> jr ra
> SYM_FUNC_END(return_to_handler)
> #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> diff --git a/arch/loongarch/kernel/mcount_dyn.S b/arch/loongarch/kernel/mcount_dyn.S
> index bbabf06244c2..ec24ae1de741 100644
> --- a/arch/loongarch/kernel/mcount_dyn.S
> +++ b/arch/loongarch/kernel/mcount_dyn.S
> @@ -131,18 +131,19 @@ SYM_CODE_END(ftrace_graph_caller)
>
> SYM_CODE_START(return_to_handler)
> /* Save return value regs */
> - PTR_ADDI sp, sp, -2 * SZREG
> - PTR_S a0, sp, 0
> - PTR_S a1, sp, SZREG
> + PTR_ADDI sp, sp, -FGRET_REGS_SIZE
> + PTR_S a0, sp, FGRET_REGS_A0
> + PTR_S a1, sp, FGRET_REGS_A1
> + PTR_S zero, sp, FGRET_REGS_FP
>
> - move a0, zero
> + move a0, sp
> bl ftrace_return_to_handler
> move ra, a0
>
> /* Restore return value regs */
> - PTR_L a0, sp, 0
> - PTR_L a1, sp, SZREG
> - PTR_ADDI sp, sp, 2 * SZREG
> + PTR_L a0, sp, FGRET_REGS_A0
> + PTR_L a1, sp, FGRET_REGS_A1
> + PTR_ADDI sp, sp, FGRET_REGS_SIZE
>
> jr ra
> SYM_CODE_END(return_to_handler)
> --
> 2.25.1
>

2023-04-03 02:37:18

by pengdonglin

[permalink] [raw]
Subject: Re: [PATCH v10 8/8] selftests/ftrace: Add funcgraph-retval test case

On 2023/4/2 6:04, Masami Hiramatsu (Google) wrote:
> On Fri, 31 Mar 2023 05:47:44 -0700
> Donglin Peng <[email protected]> wrote:
>
>> Add a test case for the funcgraph-retval and funcgraph-retval-hex
>> trace options.
>>
>> Signed-off-by: Donglin Peng <[email protected]>
>> ---
>> v10:
>> - Fix issues in selftest
>>
>> v8:
>> - Fix issues in selftest
>> ---
>> .../ftrace/test.d/ftrace/fgraph-retval.tc | 43 +++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>
>> diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>> new file mode 100644
>> index 000000000000..5819aa2dd6ad
>> --- /dev/null
>> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>> @@ -0,0 +1,43 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0
>> +# description: ftrace - function graph print function return value
>> +# requires: options/funcgraph-retval options/funcgraph-retval-hex function_graph:tracer
>> +
>> +# Make sure that funcgraph-retval works
>> +
>> +fail() { # msg
>> + echo $1
>> + exit_fail
>> +}
>> +
>> +disable_tracing
>> +clear_trace
>> +
>> +read PID _ < /proc/self/stat
>
> You can use "$$" for self pid.

Yeah, I will fix it.

>
>> +[ -f set_ftrace_pid ] && echo ${PID} > set_ftrace_pid
>> +[ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
>> +[ -f set_graph_function ] && echo proc_reg_write > set_graph_function
>
> You should set the required files for this test, so that the test result
> is always same. BTW, you need to set either 'set_ftrace_filter' or
> 'set_graph_function'.

Yes, but I discovered that set_ftrace_filter and set_graph_function rely
on the CONFIG_DYNAMIC_FTRACE configuration, which means that these two
files are not present when CONFIG_DYNAMIC_FTRACE is disabled, even if
CONFIG_FUNCTION_GRAPH_RETVAL is enabled. Therefore, I think that these
two trace files are not necessary for this test.

I will modify the above like this:

[ -f set_ftrace_pid ] && echo $$ > set_ftrace_pid
[ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter

>
>> +echo function_graph > current_tracer
>> +echo funcgraph-retval > trace_options
>> +
>> +set +e
>> +enable_tracing
>> +echo > /proc/interrupts
>> +disable_tracing
>> +set -e
>> +
>> +: "Test printing the error code in signed decimal format"
>> +echo nofuncgraph-retval-hex > trace_options
>
> echo 0 > options/funcgraph-retval-hex
>
> If you require 'options/funcgraph-retval-hex' file, you can use the
> file to set it or clear it.

Yeah.

>
>> +count=`cat trace | grep 'proc_reg_write' | grep '= -5' | wc -l`
>> +if [ $count -eq 0 ]; then
>> + fail "Return value can not be printed in signed decimal format"
>> +fi
>> +
>> +: "Test printing the error code in hexadecimal format"
>> +echo funcgraph-retval-hex > trace_options
>
> Ditto.

Thanks.

>
> Thanks,
>
>> +count=`cat trace | grep 'proc_reg_write' | grep 'fffffffb' | wc -l`
>> +if [ $count -eq 0 ]; then
>> + fail "Return value can not be printed in hexadecimal format"
>> +fi
>> +
>> +exit 0
>> --
>> 2.25.1
>>
>
>

2023-04-03 05:12:41

by pengdonglin

[permalink] [raw]
Subject: Re: [PATCH v10 8/8] selftests/ftrace: Add funcgraph-retval test case

On 2023/4/3 10:33, Donglin Peng wrote:
> On 2023/4/2 6:04, Masami Hiramatsu (Google) wrote:
>> On Fri, 31 Mar 2023 05:47:44 -0700
>> Donglin Peng <[email protected]> wrote:
>>
>>> Add a test case for the funcgraph-retval and funcgraph-retval-hex
>>> trace options.
>>>
>>> Signed-off-by: Donglin Peng <[email protected]>
>>> ---
>>> v10:
>>>   - Fix issues in selftest
>>>
>>> v8:
>>>   - Fix issues in selftest
>>> ---
>>>   .../ftrace/test.d/ftrace/fgraph-retval.tc     | 43 +++++++++++++++++++
>>>   1 file changed, 43 insertions(+)
>>>   create mode 100644
>>> tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>>
>>> diff --git
>>> a/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>> b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>> new file mode 100644
>>> index 000000000000..5819aa2dd6ad
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>> @@ -0,0 +1,43 @@
>>> +#!/bin/sh
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +# description: ftrace - function graph print function return value
>>> +# requires: options/funcgraph-retval options/funcgraph-retval-hex
>>> function_graph:tracer
>>> +
>>> +# Make sure that funcgraph-retval works
>>> +
>>> +fail() { # msg
>>> +    echo $1
>>> +    exit_fail
>>> +}
>>> +
>>> +disable_tracing
>>> +clear_trace
>>> +
>>> +read PID _ < /proc/self/stat
>>
>> You can use "$$" for self pid.
>
> Yeah, I will fix it.

I found that ftracetest used () to launch a new child process for
executing a .tc script file, however the $$ value remains unchanged,
so we can not use $$ here, because it is PPID. Therefore I think we
have to get PID from /proc/self/stat.

Here is the code from ftracetest that launches a child shell for
executing the .tc file.

__run_test() { # testfile
# setup PID and PPID, $$ is *not* updated.
(cd $TRACING_DIR; read PID _ < /proc/self/stat; set -e; set -x;
checkreq $1; initialize_ftrace; . $1)
[ $? -ne 0 ] && kill -s $SIG_FAIL $SIG_PID
}

>
>>
>>> +[ -f set_ftrace_pid ] && echo ${PID} > set_ftrace_pid
>>> +[ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
>>> +[ -f set_graph_function ] && echo proc_reg_write > set_graph_function
>>
>> You should set the required files for this test, so that the test result
>> is always same. BTW, you need to set either 'set_ftrace_filter' or
>> 'set_graph_function'.
>
> Yes, but I discovered that set_ftrace_filter and set_graph_function rely
> on the CONFIG_DYNAMIC_FTRACE configuration, which means that these two
> files are not present when CONFIG_DYNAMIC_FTRACE is disabled, even if
> CONFIG_FUNCTION_GRAPH_RETVAL is enabled. Therefore, I think that these
> two trace files are not necessary for this test.
>
> I will modify the above like this:
>
> [ -f set_ftrace_pid ] && echo $$ > set_ftrace_pid
> [ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
>
>>
>>> +echo function_graph > current_tracer
>>> +echo funcgraph-retval > trace_options
>>> +
>>> +set +e
>>> +enable_tracing
>>> +echo > /proc/interrupts
>>> +disable_tracing
>>> +set -e
>>> +
>>> +: "Test printing the error code in signed decimal format"
>>> +echo nofuncgraph-retval-hex > trace_options
>>
>> echo 0 > options/funcgraph-retval-hex
>>
>> If you require 'options/funcgraph-retval-hex' file, you can use the
>> file to set it or clear it.
>
> Yeah.
>
>>
>>> +count=`cat trace | grep 'proc_reg_write' | grep '= -5' | wc -l`
>>> +if [ $count -eq 0 ]; then
>>> +    fail "Return value can not be printed in signed decimal format"
>>> +fi
>>> +
>>> +: "Test printing the error code in hexadecimal format"
>>> +echo funcgraph-retval-hex > trace_options
>>
>> Ditto.
>
> Thanks.
>
>>
>> Thanks,
>>
>>> +count=`cat trace | grep 'proc_reg_write' | grep 'fffffffb' | wc -l`
>>> +if [ $count -eq 0 ]; then
>>> +    fail "Return value can not be printed in hexadecimal format"
>>> +fi
>>> +
>>> +exit 0
>>> --
>>> 2.25.1
>>>
>>
>>
>

2023-04-06 10:51:49

by pengdonglin

[permalink] [raw]
Subject: Re: [PATCH v10 8/8] selftests/ftrace: Add funcgraph-retval test case

On 2023/4/3 13:01, Donglin Peng wrote:
> On 2023/4/3 10:33, Donglin Peng wrote:
>> On 2023/4/2 6:04, Masami Hiramatsu (Google) wrote:
>>> On Fri, 31 Mar 2023 05:47:44 -0700
>>> Donglin Peng <[email protected]> wrote:
>>>
>>>> Add a test case for the funcgraph-retval and funcgraph-retval-hex
>>>> trace options.
>>>>
>>>> Signed-off-by: Donglin Peng <[email protected]>
>>>> ---
>>>> v10:
>>>>   - Fix issues in selftest
>>>>
>>>> v8:
>>>>   - Fix issues in selftest
>>>> ---
>>>>   .../ftrace/test.d/ftrace/fgraph-retval.tc     | 43
>>>> +++++++++++++++++++
>>>>   1 file changed, 43 insertions(+)
>>>>   create mode 100644
>>>> tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>>>
>>>> diff --git
>>>> a/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>>> b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>>> new file mode 100644
>>>> index 000000000000..5819aa2dd6ad
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/fgraph-retval.tc
>>>> @@ -0,0 +1,43 @@
>>>> +#!/bin/sh
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# description: ftrace - function graph print function return value
>>>> +# requires: options/funcgraph-retval options/funcgraph-retval-hex
>>>> function_graph:tracer
>>>> +
>>>> +# Make sure that funcgraph-retval works
>>>> +
>>>> +fail() { # msg
>>>> +    echo $1
>>>> +    exit_fail
>>>> +}
>>>> +
>>>> +disable_tracing
>>>> +clear_trace
>>>> +
>>>> +read PID _ < /proc/self/stat
>>>
>>> You can use "$$" for self pid.
>>
>> Yeah, I will fix it.
>
> I found that ftracetest used () to launch a new child process for
> executing a .tc script file, however the $$ value remains unchanged,
> so we can not use $$ here, because it is PPID. Therefore I think we
> have to get PID from /proc/self/stat.
>
> Here is the code from ftracetest that launches a child shell for
> executing the .tc file.
>
> __run_test() { # testfile
>   # setup PID and PPID, $$ is *not* updated.
>   (cd $TRACING_DIR; read PID _ < /proc/self/stat; set -e; set -x;
>    checkreq $1; initialize_ftrace; . $1)
>   [ $? -ne 0 ] && kill -s $SIG_FAIL $SIG_PID
> }
>
>>
>>>
>>>> +[ -f set_ftrace_pid ] && echo ${PID} > set_ftrace_pid
>>>> +[ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
>>>> +[ -f set_graph_function ] && echo proc_reg_write > set_graph_function
>>>
>>> You should set the required files for this test, so that the test result
>>> is always same. BTW, you need to set either 'set_ftrace_filter' or
>>> 'set_graph_function'.
>>
>> Yes, but I discovered that set_ftrace_filter and set_graph_function
>> rely on the CONFIG_DYNAMIC_FTRACE configuration, which means that
>> these two files are not present when CONFIG_DYNAMIC_FTRACE is
>> disabled, even if CONFIG_FUNCTION_GRAPH_RETVAL is enabled. Therefore,
>> I think that these two trace files are not necessary for this test.
>>
>> I will modify the above like this:
>>
>> [ -f set_ftrace_pid ] && echo $$ > set_ftrace_pid
>> [ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
>>
>>>
>>>> +echo function_graph > current_tracer
>>>> +echo funcgraph-retval > trace_options
>>>> +
>>>> +set +e
>>>> +enable_tracing
>>>> +echo > /proc/interrupts
>>>> +disable_tracing
>>>> +set -e
>>>> +
>>>> +: "Test printing the error code in signed decimal format"
>>>> +echo nofuncgraph-retval-hex > trace_options
>>>
>>> echo 0 > options/funcgraph-retval-hex
>>>
>>> If you require 'options/funcgraph-retval-hex' file, you can use the
>>> file to set it or clear it.
>>
>> Yeah.
>>
>>>
>>>> +count=`cat trace | grep 'proc_reg_write' | grep '= -5' | wc -l`
>>>> +if [ $count -eq 0 ]; then
>>>> +    fail "Return value can not be printed in signed decimal format"
>>>> +fi
>>>> +
>>>> +: "Test printing the error code in hexadecimal format"
>>>> +echo funcgraph-retval-hex > trace_options
>>>
>>> Ditto.
>>
>> Thanks.
>>
>>>
>>> Thanks,
>>>
>>>> +count=`cat trace | grep 'proc_reg_write' | grep 'fffffffb' | wc -l`
>>>> +if [ $count -eq 0 ]; then
>>>> +    fail "Return value can not be printed in hexadecimal format"
>>>> +fi
>>>> +
>>>> +exit 0
>>>> --
>>>> 2.25.1
>>>>
>>>
>>>
>>
>

Hi Masami,

I will update the selftest as follows, according to the comments:

#!/bin/sh
# SPDX-License-Identifier: GPL-2.0
# description: ftrace - function graph print function return value
# requires: options/funcgraph-retval options/funcgraph-retval-hex
function_graph:tracer

# Make sure that funcgraph-retval works

fail() { # msg
echo $1
exit_fail
}

disable_tracing
clear_trace

# get self PID, can not use $$, because it is PPID
read PID _ < /proc/self/stat

[ -f set_ftrace_filter ] && echo proc_reg_write > set_ftrace_filter
[ -f set_ftrace_pid ] && echo ${PID} > set_ftrace_pid
echo function_graph > current_tracer
echo 1 > options/funcgraph-retval

set +e
enable_tracing
echo > /proc/interrupts
disable_tracing
set -e

: "Test printing the error code in signed decimal format"
echo 0 > options/funcgraph-retval-hex
count=`cat trace | grep 'proc_reg_write' | grep '= -5' | wc -l`
if [ $count -eq 0 ]; then
fail "Return value can not be printed in signed decimal format"
fi

: "Test printing the error code in hexadecimal format"
echo 1 > options/funcgraph-retval-hex
count=`cat trace | grep 'proc_reg_write' | grep 'fffffffb' | wc -l`
if [ $count -eq 0 ]; then
fail "Return value can not be printed in hexadecimal format"
fi

exit 0