Message-ID: <20230131005315.phdnhkeeconxxm3e@macbook-pro-6.dhcp.thefacebook.com>
Date:   Mon, 30 Jan 2023 16:53:15 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Ross Zwisler <zwisler@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ross Zwisler <zwisler@...gle.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        linux-trace-kernel@...r.kernel.org,
        Mykola Lysenko <mykolal@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Shuah Khan <shuah@...nel.org>, bpf <bpf@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH 3/9] selftests/bpf: use canonical ftrace path

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@...il.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.

Powered by blists - more mailing lists