| Message ID | 20230130181915.1113313-4-zwisler@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | use canonical ftrace path whenever possible | expand |
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.
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
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.
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
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.
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
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.
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 --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;
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(-)