From: John Fastabend <john.fastabend@gmail.com>
To: Ross Zwisler <zwisler@google.com>, Steven Rostedt <rostedt@goodmis.org>
Cc: zwisler@kernel.org, bpf@vger.kernel.org,
linux-kernel@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Hao Luo <haoluo@google.com>, Jason Gunthorpe <jgg@ziepe.ca>,
Jiri Olsa <jolsa@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>, Leon Romanovsky <leon@kernel.org>,
Martin KaFai Lau <martin.lau@linux.dev>,
Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>,
Song Liu <song@kernel.org>, Stanislav Fomichev <sdf@google.com>,
Yonghong Song <yhs@fb.com>,
linux-kselftest@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-trace-kernel@vger.kernel.org,
"Michael S . Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH bpf-next v3 2/2] selftests/bpf: use canonical ftrace path
Date: Mon, 13 Mar 2023 22:27:32 -0700 [thread overview]
Message-ID: <641005c453661_4258120826@john.notmuch> (raw)
In-Reply-To: <20230313204050.GA592900@google.com>
Ross Zwisler wrote:
> On Fri, Mar 10, 2023 at 06:33:52PM -0500, Steven Rostedt wrote:
> > On Fri, 10 Mar 2023 10:52:09 -0700
> > zwisler@kernel.org wrote:
> >
> > > diff --git a/tools/testing/selftests/bpf/get_cgroup_id_user.c b/tools/testing/selftests/bpf/get_cgroup_id_user.c
> > > index 156743cf5870..4fa61ac8a0ee 100644
> > > --- a/tools/testing/selftests/bpf/get_cgroup_id_user.c
> > > +++ b/tools/testing/selftests/bpf/get_cgroup_id_user.c
> > > @@ -86,8 +86,12 @@ int main(int argc, char **argv)
> > > pid = getpid();
> > > bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
> > >
> > > - snprintf(buf, sizeof(buf),
> > > - "/sys/kernel/debug/tracing/events/%s/id", probe_name);
> > > + if (access("/sys/kernel/tracing/trace", F_OK) == 0)
> > > + snprintf(buf, sizeof(buf),
> > > + "/sys/kernel/tracing/events/%s/id", probe_name);
> > > + else
> > > + snprintf(buf, sizeof(buf),
> > > + "/sys/kernel/debug/tracing/events/%s/id", probe_name);
> >
> > I don't know how the BPF folks feel, but I do know some kernel developers
> > prefer that if you need to break a single command into multiple lines that
> > you then need to add brackets around it. As it makes it easier to read.
> >
> > if (access("/sys/kernel/tracing/trace", F_OK) == 0) {
> > snprintf(buf, sizeof(buf),
> > "/sys/kernel/tracing/events/%s/id", probe_name);
> > } else {
> > snprintf(buf, sizeof(buf),
> > "/sys/kernel/debug/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 113dba349a57..22be0a9a5a0a 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,12 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
> > > * Filtering out duplicates by using hashmap__add, which won't
> > > * add existing entry.
> > > */
> > > - f = fopen("/sys/kernel/debug/tracing/available_filter_functions", "r");
> > > +
> > > + if (access("/sys/kernel/tracing/trace", F_OK) == 0)
> > > + f = fopen("/sys/kernel/tracing/available_filter_functions", "r");
> > > + else
> > > + f = fopen("/sys/kernel/debug/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..60f92fd3c37a 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
> > > @@ -17,8 +17,12 @@ static void test_task_fd_query_tp_core(const char *probe_name,
> > > if (CHECK(err, "bpf_prog_test_load", "err %d errno %d\n", err, errno))
> > > goto close_prog;
> > >
> > > - snprintf(buf, sizeof(buf),
> > > - "/sys/kernel/debug/tracing/events/%s/id", probe_name);
> > > + if (access("/sys/kernel/tracing/trace", F_OK) == 0)
> > > + snprintf(buf, sizeof(buf),
> > > + "/sys/kernel/tracing/events/%s/id", probe_name);
> > > + else
> > > + snprintf(buf, sizeof(buf),
> > > + "/sys/kernel/debug/tracing/events/%s/id", probe_name);
> >
> > Same here.
> >
> > > 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 770fcc3bb1ba..d3e377fa8e9b 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
> > > @@ -16,8 +16,12 @@ void serial_test_tp_attach_query(void)
> > > for (i = 0; i < num_progs; i++)
> > > obj[i] = NULL;
> > >
> > > - snprintf(buf, sizeof(buf),
> > > - "/sys/kernel/debug/tracing/events/sched/sched_switch/id");
> > > + if (access("/sys/kernel/tracing/trace", F_OK) == 0)
> > > + snprintf(buf, sizeof(buf),
> > > + "/sys/kernel/tracing/events/sched/sched_switch/id");
> > > + else
> > > + snprintf(buf, sizeof(buf),
> > > + "/sys/kernel/debug/tracing/events/sched/sched_switch/id");
> >
> > and here.
> >
> > But perhaps the BPF folks don't care?
>
> Sure, I agree that this is more readable. I'll gather your Reviewed-by for
> patch #1, make this change, rebase to the current bpf/bpf-next and send out
> v4.
Also for the patch. LGTM
Acked-by: John Fastabend <john.fastabend@gmail.com>
next prev parent reply other threads:[~2023-03-14 5:27 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-10 17:52 [PATCH bpf-next v3 1/2] bpf: use canonical ftrace path zwisler
2023-03-10 17:52 ` [PATCH bpf-next v3 2/2] selftests/bpf: " zwisler
2023-03-10 23:33 ` Steven Rostedt
2023-03-13 20:40 ` Ross Zwisler
2023-03-14 5:27 ` John Fastabend [this message]
2023-03-10 23:34 ` [PATCH bpf-next v3 1/2] bpf: " Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=641005c453661_4258120826@john.notmuch \
--to=john.fastabend@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=jgg@ziepe.ca \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=leon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=mst@redhat.com \
--cc=mykolal@fb.com \
--cc=rostedt@goodmis.org \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yhs@fb.com \
--cc=zwisler@google.com \
--cc=zwisler@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.