Date:   Mon, 30 Jan 2023 16:53:15 -0800
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Ross Zwisler <zwisler@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Ross Zwisler <zwisler@google.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        linux-trace-kernel@vger.kernel.org,
        Mykola Lysenko <mykolal@fb.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Shuah Khan <shuah@kernel.org>, bpf <bpf@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH 3/9] selftests/bpf: use canonical ftrace path
Message-ID: <20230131005315.phdnhkeeconxxm3e@macbook-pro-6.dhcp.thefacebook.com>
References: <20230130181915.1113313-1-zwisler@google.com>
 <20230130181915.1113313-4-zwisler@google.com>
 <CAADnVQJ7KxEK92qOz0Ya4MrACHpxngSpG4W38xuGEgZmXEG-vQ@mail.gmail.com>
 <20230130145932.37cf6b73@gandalf.local.home>
 <CAADnVQ+F3Z70mu3-QyyNFyJ2qCkDXnMJCW-o+fcnZo=LWj5d9g@mail.gmail.com>
 <20230130183419.0626dc21@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230130183419.0626dc21@gandalf.local.home>
Precedence: bulk

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.