diff mbox series

[3/9] selftests/bpf: use canonical ftrace path

Message ID 20230130181915.1113313-4-zwisler@google.com (mailing list archive)
State Superseded
Headers show
Series use canonical ftrace path whenever possible | expand

Commit Message

Ross Zwisler Jan. 30, 2023, 6:19 p.m. UTC
The canonical location for the tracefs filesystem is at /sys/kernel/tracing.

But, from Documentation/trace/ftrace.rst:

  Before 4.1, all ftrace tracing control files were within the debugfs
  file system, which is typically located at /sys/kernel/debug/tracing.
  For backward compatibility, when mounting the debugfs file system,
  the tracefs file system will be automatically mounted at:

  /sys/kernel/debug/tracing

Many tests in the bpf selftest code still refer to this older debugfs
path, so let's update them to avoid confusion.

Signed-off-by: Ross Zwisler <zwisler@google.com>
---
 tools/testing/selftests/bpf/get_cgroup_id_user.c          | 2 +-
 .../testing/selftests/bpf/prog_tests/kprobe_multi_test.c  | 2 +-
 tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c | 2 +-
 tools/testing/selftests/bpf/prog_tests/tp_attach_query.c  | 2 +-
 tools/testing/selftests/bpf/prog_tests/trace_printk.c     | 2 +-
 tools/testing/selftests/bpf/prog_tests/trace_vprintk.c    | 2 +-
 tools/testing/selftests/bpf/progs/test_stacktrace_map.c   | 2 +-
 tools/testing/selftests/bpf/progs/test_tracepoint.c       | 2 +-
 tools/testing/selftests/bpf/test_ftrace.sh                | 2 +-
 tools/testing/selftests/bpf/test_tunnel.sh                | 8 ++++----
 tools/testing/selftests/bpf/trace_helpers.c               | 4 ++--
 11 files changed, 15 insertions(+), 15 deletions(-)

Comments

Alexei Starovoitov Jan. 30, 2023, 7:52 p.m. UTC | #1
On Mon, Jan 30, 2023 at 10:19 AM Ross Zwisler <zwisler@chromium.org> wrote:
>
> The canonical location for the tracefs filesystem is at /sys/kernel/tracing.
>
> But, from Documentation/trace/ftrace.rst:
>
>   Before 4.1, all ftrace tracing control files were within the debugfs
>   file system, which is typically located at /sys/kernel/debug/tracing.
>   For backward compatibility, when mounting the debugfs file system,
>   the tracefs file system will be automatically mounted at:
>
>   /sys/kernel/debug/tracing
>
> Many tests in the bpf selftest code still refer to this older debugfs
> path, so let's update them to avoid confusion.

I wish that was the case, but in reality there are still systems
out there where tracefs is only mounted in that old location.
For example in one my VMs:

$ cat /proc/mounts |grep tracefs
tracefs /sys/kernel/debug/tracing tracefs rw,relatime 0 0
$ ls /sys/kernel/tracing/
$ uname -r
6.2.0-rc5-01030-gc1a3daf7363b

So this change will break the tests. We cannot do it.
Steven Rostedt Jan. 30, 2023, 7:59 p.m. UTC | #2
On Mon, 30 Jan 2023 11:52:03 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Mon, Jan 30, 2023 at 10:19 AM Ross Zwisler <zwisler@chromium.org> wrote:
> >
> > The canonical location for the tracefs filesystem is at /sys/kernel/tracing.
> >
> > But, from Documentation/trace/ftrace.rst:
> >
> >   Before 4.1, all ftrace tracing control files were within the debugfs
> >   file system, which is typically located at /sys/kernel/debug/tracing.
> >   For backward compatibility, when mounting the debugfs file system,
> >   the tracefs file system will be automatically mounted at:
> >
> >   /sys/kernel/debug/tracing
> >
> > Many tests in the bpf selftest code still refer to this older debugfs
> > path, so let's update them to avoid confusion.  
> 
> I wish that was the case, but in reality there are still systems
> out there where tracefs is only mounted in that old location.
> For example in one my VMs:
> 
> $ cat /proc/mounts |grep tracefs
> tracefs /sys/kernel/debug/tracing tracefs rw,relatime 0 0
> $ ls /sys/kernel/tracing/
> $ uname -r
> 6.2.0-rc5-01030-gc1a3daf7363b
> 
> So this change will break the tests. We cannot do it.

Could we add a way to try to mount it?

If anything, the tests should not have the path hard coded. It should then
look to see if it is mounted and use the path that is found. Otherwise it
should try mounting it at the correct location.

Feel free to take the code from libtracefs (and modify it):

https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-utils.c#n89

It will make the test code much more robust.

-- Steve
Alexei Starovoitov Jan. 30, 2023, 8:03 p.m. UTC | #3
On Mon, Jan 30, 2023 at 11:59 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 30 Jan 2023 11:52:03 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Mon, Jan 30, 2023 at 10:19 AM Ross Zwisler <zwisler@chromium.org> wrote:
> > >
> > > The canonical location for the tracefs filesystem is at /sys/kernel/tracing.
> > >
> > > But, from Documentation/trace/ftrace.rst:
> > >
> > >   Before 4.1, all ftrace tracing control files were within the debugfs
> > >   file system, which is typically located at /sys/kernel/debug/tracing.
> > >   For backward compatibility, when mounting the debugfs file system,
> > >   the tracefs file system will be automatically mounted at:
> > >
> > >   /sys/kernel/debug/tracing
> > >
> > > Many tests in the bpf selftest code still refer to this older debugfs
> > > path, so let's update them to avoid confusion.
> >
> > I wish that was the case, but in reality there are still systems
> > out there where tracefs is only mounted in that old location.
> > For example in one my VMs:
> >
> > $ cat /proc/mounts |grep tracefs
> > tracefs /sys/kernel/debug/tracing tracefs rw,relatime 0 0
> > $ ls /sys/kernel/tracing/
> > $ uname -r
> > 6.2.0-rc5-01030-gc1a3daf7363b
> >
> > So this change will break the tests. We cannot do it.
>
> Could we add a way to try to mount it?
>
> If anything, the tests should not have the path hard coded. It should then
> look to see if it is mounted and use the path that is found. Otherwise it
> should try mounting it at the correct location.
>
> Feel free to take the code from libtracefs (and modify it):
>
> https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-utils.c#n89
>
> It will make the test code much more robust.

The point is not about tests. The point is that this change might break
some users that are working today with /sys/kernel/debug/tracing.
It also might be mounted differently.
For example from another system:
cat /proc/mounts|grep trace
tracefs /sys/kernel/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0
tracefs /sys/kernel/debug/tracing tracefs rw,relatime 0 0

So I suggest leaving the code as-is.
Steven Rostedt Jan. 30, 2023, 11:34 p.m. UTC | #4
On Mon, 30 Jan 2023 12:03:52 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > >
> > > So this change will break the tests. We cannot do it.  
> >
> > Could we add a way to try to mount it?
> >
> > If anything, the tests should not have the path hard coded. It should then
> > look to see if it is mounted and use the path that is found. Otherwise it
> > should try mounting it at the correct location.
> >
> > Feel free to take the code from libtracefs (and modify it):
> >
> > https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-utils.c#n89
> >
> > It will make the test code much more robust.  
> 
> The point is not about tests. The point is that this change might break
> some users that are working today with /sys/kernel/debug/tracing.

> It also might be mounted differently.
> For example from another system:
> cat /proc/mounts|grep trace
> tracefs /sys/kernel/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0
> tracefs /sys/kernel/debug/tracing tracefs rw,relatime 0 0

Yes, and the code works when it's mounted multiple times.

> 
> So I suggest leaving the code as-is.

Why?  I want to make /sys/kernel/debug/tracing deprecated. It's a hack to
not break old code. I've had complaints about that hack, and there's even
systems that disable the auto mounting (that is, /sys/kernel/debug/tracing
would not exist in such configs) This was never expected to be a permanent
solution.

If anything, leaving hardcoded calls like that forces the user to mount
debugfs when they may not want to. The entire point of tracefs was to allow
users to have access to the trace events without having to expose debugfs
and all the crud it brings with it. This was requested several times before
it was added.

What is your technical reason for not modifying the code to look for
tracefs in /sys/kernel/tracing and if it's not there try
/sys/kernel/debug/tracing, and if both are not found, try mounting it.

That change is not hard and makes the code much more robust and does not
break anything.

-- Steve
Alexei Starovoitov Jan. 31, 2023, 12:53 a.m. UTC | #5
On Mon, Jan 30, 2023 at 06:34:19PM -0500, Steven Rostedt wrote:
> On Mon, 30 Jan 2023 12:03:52 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > So this change will break the tests. We cannot do it.  
> > >
> > > Could we add a way to try to mount it?
> > >
> > > If anything, the tests should not have the path hard coded. It should then
> > > look to see if it is mounted and use the path that is found. Otherwise it
> > > should try mounting it at the correct location.
> > >
> > > Feel free to take the code from libtracefs (and modify it):
> > >
> > > https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-utils.c#n89
> > >
> > > It will make the test code much more robust.  
> > 
> > The point is not about tests. The point is that this change might break
> > some users that are working today with /sys/kernel/debug/tracing.
> 
> > It also might be mounted differently.
> > For example from another system:
> > cat /proc/mounts|grep trace
> > tracefs /sys/kernel/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0
> > tracefs /sys/kernel/debug/tracing tracefs rw,relatime 0 0
> 
> Yes, and the code works when it's mounted multiple times.
> 
> > 
> > So I suggest leaving the code as-is.
> 
> Why?  I want to make /sys/kernel/debug/tracing deprecated. It's a hack to
> not break old code. I've had complaints about that hack, and there's even
> systems that disable the auto mounting (that is, /sys/kernel/debug/tracing
> would not exist in such configs) This was never expected to be a permanent
> solution.

I don't think /sys/kernel/debug/tracing can ever be deprecated.
There are plenty of user space applications (not bpf related at all) that
expect it to be in that location.

Quick search shows:

android profiler:
https://android.googlesource.com/platform/external/perfetto/+/refs/heads/master/src/tools/dump_ftrace_stats/main.cc#60

java profiler:
https://github.com/jvm-profiling-tools/async-profiler/blob/master/src/perfEvents_linux.cpp#L85

> If anything, leaving hardcoded calls like that forces the user to mount
> debugfs when they may not want to. The entire point of tracefs was to allow
> users to have access to the trace events without having to expose debugfs
> and all the crud it brings with it. This was requested several times before
> it was added.

All makes sense.

> What is your technical reason for not modifying the code to look for
> tracefs in /sys/kernel/tracing and if it's not there try
> /sys/kernel/debug/tracing, and if both are not found, try mounting it.

libbpf already has code to probe both locations.
The point that full deprecation of /sys/kernel/debug/tracing is not possible,
hence no point doing the diff:
48 files changed, 96 insertions(+), 95 deletions(-)
It doesn't move the needle. Just a code churn.
Steven Rostedt Jan. 31, 2023, 7:50 p.m. UTC | #6
On Mon, 30 Jan 2023 16:53:15 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> I don't think /sys/kernel/debug/tracing can ever be deprecated.
> There are plenty of user space applications (not bpf related at all) that
> expect it to be in that location.
> 
> Quick search shows:
> 
> android profiler:
> https://android.googlesource.com/platform/external/perfetto/+/refs/heads/master/src/tools/dump_ftrace_stats/main.cc#60
> 
> java profiler:
> https://github.com/jvm-profiling-tools/async-profiler/blob/master/src/perfEvents_linux.cpp#L85

These can easily be changed. We have deprecated stuff in the past, by
making sure all the affected code is updated properly.

One way is to start adding printks when used. Then update to WARN() to get
people to complain. Yes, the burden is on us (me and others) to go out and
fix the issues. But it is possible to do, as I've done it before.

> 
> > If anything, leaving hardcoded calls like that forces the user to mount
> > debugfs when they may not want to. The entire point of tracefs was to allow
> > users to have access to the trace events without having to expose debugfs
> > and all the crud it brings with it. This was requested several times before
> > it was added.  
> 
> All makes sense.
> 
> > What is your technical reason for not modifying the code to look for
> > tracefs in /sys/kernel/tracing and if it's not there try
> > /sys/kernel/debug/tracing, and if both are not found, try mounting it.  
> 
> libbpf already has code to probe both locations.
> The point that full deprecation of /sys/kernel/debug/tracing is not possible,
> hence no point doing the diff:
> 48 files changed, 96 insertions(+), 95 deletions(-)
> It doesn't move the needle. Just a code churn.

As code in the Linux kernel is used as examples for future work, it should
not be using an interface that is obsolete. That's enough rational for code
churn. This "we can never deprecated so we won't even try" BS is not an
answer.

-- Steve
Alexei Starovoitov Jan. 31, 2023, 11:25 p.m. UTC | #7
On Tue, Jan 31, 2023 at 11:50 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> As code in the Linux kernel is used as examples for future work, it should
> not be using an interface that is obsolete. That's enough rational for code
> churn.

Fair enough. Please resubmit these two patches towards bpf-next
with [PATCH bpf-next] subj, so that BPF CI can chew on it.
Ross Zwisler Feb. 1, 2023, 12:05 a.m. UTC | #8
On Tue, Jan 31, 2023 at 4:26 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> Fair enough. Please resubmit these two patches towards bpf-next
> with [PATCH bpf-next] subj, so that BPF CI can chew on it.

Will do, thanks.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/get_cgroup_id_user.c b/tools/testing/selftests/bpf/get_cgroup_id_user.c
index 156743cf5870..478e080128be 100644
--- a/tools/testing/selftests/bpf/get_cgroup_id_user.c
+++ b/tools/testing/selftests/bpf/get_cgroup_id_user.c
@@ -87,7 +87,7 @@  int main(int argc, char **argv)
 	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
 
 	snprintf(buf, sizeof(buf),
-		 "/sys/kernel/debug/tracing/events/%s/id", probe_name);
+		 "/sys/kernel/tracing/events/%s/id", probe_name);
 	efd = open(buf, O_RDONLY, 0);
 	if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
 		goto close_prog;
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index d457a55ff408..9eaf16e9ff6e 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -338,7 +338,7 @@  static int get_syms(char ***symsp, size_t *cntp)
 	 * Filtering out duplicates by using hashmap__add, which won't
 	 * add existing entry.
 	 */
-	f = fopen("/sys/kernel/debug/tracing/available_filter_functions", "r");
+	f = fopen("/sys/kernel/tracing/available_filter_functions", "r");
 	if (!f)
 		return -EINVAL;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
index c717741bf8b6..6d70559fc19b 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
@@ -18,7 +18,7 @@  static void test_task_fd_query_tp_core(const char *probe_name,
 		goto close_prog;
 
 	snprintf(buf, sizeof(buf),
-		 "/sys/kernel/debug/tracing/events/%s/id", probe_name);
+		 "/sys/kernel/tracing/events/%s/id", probe_name);
 	efd = open(buf, O_RDONLY, 0);
 	if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
 		goto close_prog;
diff --git a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
index a479080533db..4308e3a828d8 100644
--- a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
+++ b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
@@ -17,7 +17,7 @@  void serial_test_tp_attach_query(void)
 		obj[i] = NULL;
 
 	snprintf(buf, sizeof(buf),
-		 "/sys/kernel/debug/tracing/events/sched/sched_switch/id");
+		 "/sys/kernel/tracing/events/sched/sched_switch/id");
 	efd = open(buf, O_RDONLY, 0);
 	if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
 		return;
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_printk.c b/tools/testing/selftests/bpf/prog_tests/trace_printk.c
index cade7f12315f..ff50a928cb98 100644
--- a/tools/testing/selftests/bpf/prog_tests/trace_printk.c
+++ b/tools/testing/selftests/bpf/prog_tests/trace_printk.c
@@ -5,7 +5,7 @@ 
 
 #include "trace_printk.lskel.h"
 
-#define TRACEBUF	"/sys/kernel/debug/tracing/trace_pipe"
+#define TRACEBUF	"/sys/kernel/tracing/trace_pipe"
 #define SEARCHMSG	"testing,testing"
 
 void serial_test_trace_printk(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c b/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
index 7a4e313e8558..e568d7f247ec 100644
--- a/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
+++ b/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
@@ -5,7 +5,7 @@ 
 
 #include "trace_vprintk.lskel.h"
 
-#define TRACEBUF	"/sys/kernel/debug/tracing/trace_pipe"
+#define TRACEBUF	"/sys/kernel/tracing/trace_pipe"
 #define SEARCHMSG	"1,2,3,4,5,6,7,8,9,10"
 
 void serial_test_trace_vprintk(void)
diff --git a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
index 728dbd39eff0..47568007b668 100644
--- a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
+++ b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
@@ -38,7 +38,7 @@  struct {
 	__type(value, stack_trace_t);
 } stack_amap SEC(".maps");
 
-/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
+/* taken from /sys/kernel/tracing/events/sched/sched_switch/format */
 struct sched_switch_args {
 	unsigned long long pad;
 	char prev_comm[TASK_COMM_LEN];
diff --git a/tools/testing/selftests/bpf/progs/test_tracepoint.c b/tools/testing/selftests/bpf/progs/test_tracepoint.c
index 43bd7a20cc50..4cb8bbb6a320 100644
--- a/tools/testing/selftests/bpf/progs/test_tracepoint.c
+++ b/tools/testing/selftests/bpf/progs/test_tracepoint.c
@@ -4,7 +4,7 @@ 
 #include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
 
-/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
+/* taken from /sys/kernel/tracing/events/sched/sched_switch/format */
 struct sched_switch_args {
 	unsigned long long pad;
 	char prev_comm[TASK_COMM_LEN];
diff --git a/tools/testing/selftests/bpf/test_ftrace.sh b/tools/testing/selftests/bpf/test_ftrace.sh
index 20de7bb873bc..e3e2328a1b65 100755
--- a/tools/testing/selftests/bpf/test_ftrace.sh
+++ b/tools/testing/selftests/bpf/test_ftrace.sh
@@ -1,6 +1,6 @@ 
 #!/bin/bash
 
-TR=/sys/kernel/debug/tracing/
+TR=/sys/kernel/tracing/
 clear_trace() { # reset trace output
     echo > $TR/trace
 }
diff --git a/tools/testing/selftests/bpf/test_tunnel.sh b/tools/testing/selftests/bpf/test_tunnel.sh
index e9ebc67d73f7..6927e586a3a2 100755
--- a/tools/testing/selftests/bpf/test_tunnel.sh
+++ b/tools/testing/selftests/bpf/test_tunnel.sh
@@ -542,7 +542,7 @@  setup_xfrm_tunnel()
 test_xfrm_tunnel()
 {
 	config_device
-	> /sys/kernel/debug/tracing/trace
+	> /sys/kernel/tracing/trace
 	setup_xfrm_tunnel
 	mkdir -p ${BPF_PIN_TUNNEL_DIR}
 	bpftool prog loadall ./test_tunnel_kern.o ${BPF_PIN_TUNNEL_DIR}
@@ -551,11 +551,11 @@  test_xfrm_tunnel()
 		${BPF_PIN_TUNNEL_DIR}/xfrm_get_state
 	ip netns exec at_ns0 ping $PING_ARG 10.1.1.200
 	sleep 1
-	grep "reqid 1" /sys/kernel/debug/tracing/trace
+	grep "reqid 1" /sys/kernel/tracing/trace
 	check_err $?
-	grep "spi 0x1" /sys/kernel/debug/tracing/trace
+	grep "spi 0x1" /sys/kernel/tracing/trace
 	check_err $?
-	grep "remote ip 0xac100164" /sys/kernel/debug/tracing/trace
+	grep "remote ip 0xac100164" /sys/kernel/tracing/trace
 	check_err $?
 	cleanup
 
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 9c4be2cdb21a..65895f5fb562 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -12,7 +12,7 @@ 
 #include <sys/mman.h>
 #include "trace_helpers.h"
 
-#define DEBUGFS "/sys/kernel/debug/tracing/"
+#define TRACEFS "/sys/kernel/tracing/"
 
 #define MAX_SYMS 300000
 static struct ksym syms[MAX_SYMS];
@@ -130,7 +130,7 @@  void read_trace_pipe(void)
 {
 	int trace_fd;
 
-	trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
+	trace_fd = open(TRACEFS "trace_pipe", O_RDONLY, 0);
 	if (trace_fd < 0)
 		return;