From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Leo Yan <leo.yan@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
Ian Rogers <irogers@google.com>,
linux-perf-users@vger.kernel.org, bpf@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 5/5] perf trace: Remove unused bpf map 'syscalls'
Date: Mon, 21 Nov 2022 15:58:13 -0300 [thread overview]
Message-ID: <Y3vKRZtaW5Mnxm11@kernel.org> (raw)
In-Reply-To: <20221121075237.127706-6-leo.yan@linaro.org>
Em Mon, Nov 21, 2022 at 07:52:37AM +0000, Leo Yan escreveu:
> augmented_raw_syscalls.c defines the bpf map 'syscalls' which is
> initialized by perf tool in user space to indicate which system calls
> are enabled for tracing, on the other flip eBPF program relies on the
> map to filter out the trace events which are not enabled.
>
> The map also includes a field 'string_args_len[6]' which presents the
> string length if the corresponding argument is a string type.
This one was for implementing something like 'strace -s', but yeah, that
didn't get implemented, better ditch it now. At some point we can find
another way to implement that, per syscall even :)
Thanks for working on this, I'm applying the series.
- Arnaldo
> Now the map 'syscalls' is not used, bpf program doesn't use it as filter
> anymore, this is replaced by using the function bpf_tail_call() and
> PROG_ARRAY syscalls map. And we don't need to explicitly set the string
> length anymore, bpf_probe_read_str() is smart to copy the string and
> return string length.
>
> Therefore, it's safe to remove the bpf map 'syscalls'.
>
> To consolidate the code, this patch removes the definition of map
> 'syscalls' from augmented_raw_syscalls.c and drops code for using
> the map in the perf trace.
>
> Note, since function trace__set_ev_qualifier_bpf_filter() is removed,
> calling trace__init_syscall_bpf_progs() from it is also removed. We
> don't need to worry it because trace__init_syscall_bpf_progs() is
> still invoked from trace__init_syscalls_bpf_prog_array_maps() for
> initialization the system call's bpf program callback.
>
> After:
>
> # perf trace -e examples/bpf/augmented_raw_syscalls.c,open* --max-events 10 perf stat --quiet sleep 0.001
> openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libm.so.6", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libelf.so.1", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libdw.so.1", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libunwind.so.8", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libunwind-aarch64.so.8", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libcrypto.so.3", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libslang.so.2", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libperl.so.5.34", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>
> # perf trace -e examples/bpf/augmented_raw_syscalls.c --max-events 10 perf stat --quiet sleep 0.001
> ... [continued]: execve()) = 0
> brk(NULL) = 0xaaaab1d28000
> faccessat(-100, "/etc/ld.so.preload", 4) = -1 ENOENT (No such file or directory)
> openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
> close(3</usr/lib/aarch64-linux-gnu/libcrypto.so.3>) = 0
> openat(AT_FDCWD, "/lib/aarch64-linux-gnu/libm.so.6", O_RDONLY|O_CLOEXEC) = 3
> read(3</usr/lib/aarch64-linux-gnu/libcrypto.so.3>, 0xfffff33f70d0, 832) = 832
> munmap(0xffffb5519000, 28672) = 0
> munmap(0xffffb55b7000, 32880) = 0
> mprotect(0xffffb55a6000, 61440, PROT_NONE) = 0
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/builtin-trace.c | 101 ------------------
> .../examples/bpf/augmented_raw_syscalls.c | 17 ---
> 2 files changed, 118 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 071e7598391f..543c379d2a57 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -122,7 +122,6 @@ struct trace {
> struct syscalltbl *sctbl;
> struct {
> struct syscall *table;
> - struct bpf_map *map;
> struct { // per syscall BPF_MAP_TYPE_PROG_ARRAY
> struct bpf_map *sys_enter,
> *sys_exit;
> @@ -1224,16 +1223,6 @@ struct syscall {
> struct syscall_arg_fmt *arg_fmt;
> };
>
> -/*
> - * Must match what is in the BPF program:
> - *
> - * tools/perf/examples/bpf/augmented_raw_syscalls.c
> - */
> -struct bpf_map_syscall_entry {
> - bool enabled;
> - u16 string_args_len[RAW_SYSCALL_ARGS_NUM];
> -};
> -
> /*
> * We need to have this 'calculated' boolean because in some cases we really
> * don't know what is the duration of a syscall, for instance, when we start
> @@ -3259,7 +3248,6 @@ static void trace__set_bpf_map_filtered_pids(struct trace *trace)
>
> static void trace__set_bpf_map_syscalls(struct trace *trace)
> {
> - trace->syscalls.map = trace__find_bpf_map_by_name(trace, "syscalls");
> trace->syscalls.prog_array.sys_enter = trace__find_bpf_map_by_name(trace, "syscalls_sys_enter");
> trace->syscalls.prog_array.sys_exit = trace__find_bpf_map_by_name(trace, "syscalls_sys_exit");
> }
> @@ -3339,80 +3327,6 @@ static int trace__bpf_prog_sys_exit_fd(struct trace *trace, int id)
> return sc ? bpf_program__fd(sc->bpf_prog.sys_exit) : bpf_program__fd(trace->syscalls.unaugmented_prog);
> }
>
> -static void trace__init_bpf_map_syscall_args(struct trace *trace, int id, struct bpf_map_syscall_entry *entry)
> -{
> - struct syscall *sc = trace__syscall_info(trace, NULL, id);
> - int arg = 0;
> -
> - if (sc == NULL)
> - goto out;
> -
> - for (; arg < sc->nr_args; ++arg) {
> - entry->string_args_len[arg] = 0;
> - if (sc->arg_fmt[arg].scnprintf == SCA_FILENAME) {
> - /* Should be set like strace -s strsize */
> - entry->string_args_len[arg] = PATH_MAX;
> - }
> - }
> -out:
> - for (; arg < 6; ++arg)
> - entry->string_args_len[arg] = 0;
> -}
> -static int trace__set_ev_qualifier_bpf_filter(struct trace *trace)
> -{
> - int fd = bpf_map__fd(trace->syscalls.map);
> - struct bpf_map_syscall_entry value = {
> - .enabled = !trace->not_ev_qualifier,
> - };
> - int err = 0;
> - size_t i;
> -
> - for (i = 0; i < trace->ev_qualifier_ids.nr; ++i) {
> - int key = trace->ev_qualifier_ids.entries[i];
> -
> - if (value.enabled) {
> - trace__init_bpf_map_syscall_args(trace, key, &value);
> - trace__init_syscall_bpf_progs(trace, key);
> - }
> -
> - err = bpf_map_update_elem(fd, &key, &value, BPF_EXIST);
> - if (err)
> - break;
> - }
> -
> - return err;
> -}
> -
> -static int __trace__init_syscalls_bpf_map(struct trace *trace, bool enabled)
> -{
> - int fd = bpf_map__fd(trace->syscalls.map);
> - struct bpf_map_syscall_entry value = {
> - .enabled = enabled,
> - };
> - int err = 0, key;
> -
> - for (key = 0; key < trace->sctbl->syscalls.nr_entries; ++key) {
> - if (enabled)
> - trace__init_bpf_map_syscall_args(trace, key, &value);
> -
> - err = bpf_map_update_elem(fd, &key, &value, BPF_ANY);
> - if (err)
> - break;
> - }
> -
> - return err;
> -}
> -
> -static int trace__init_syscalls_bpf_map(struct trace *trace)
> -{
> - bool enabled = true;
> -
> - if (trace->ev_qualifier_ids.nr)
> - enabled = trace->not_ev_qualifier;
> -
> - return __trace__init_syscalls_bpf_map(trace, enabled);
> -}
> -
> static struct bpf_program *trace__find_usable_bpf_prog_entry(struct trace *trace, struct syscall *sc)
> {
> struct tep_format_field *field, *candidate_field;
> @@ -3627,16 +3541,6 @@ static void trace__set_bpf_map_syscalls(struct trace *trace __maybe_unused)
> {
> }
>
> -static int trace__set_ev_qualifier_bpf_filter(struct trace *trace __maybe_unused)
> -{
> - return 0;
> -}
> -
> -static int trace__init_syscalls_bpf_map(struct trace *trace __maybe_unused)
> -{
> - return 0;
> -}
> -
> static struct bpf_program *trace__find_bpf_program_by_title(struct trace *trace __maybe_unused,
> const char *name __maybe_unused)
> {
> @@ -3670,8 +3574,6 @@ static bool trace__only_augmented_syscalls_evsels(struct trace *trace)
>
> static int trace__set_ev_qualifier_filter(struct trace *trace)
> {
> - if (trace->syscalls.map)
> - return trace__set_ev_qualifier_bpf_filter(trace);
> if (trace->syscalls.events.sys_enter)
> return trace__set_ev_qualifier_tp_filter(trace);
> return 0;
> @@ -4045,9 +3947,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
> if (err < 0)
> goto out_error_mem;
>
> - if (trace->syscalls.map)
> - trace__init_syscalls_bpf_map(trace);
> -
> if (trace->syscalls.prog_array.sys_enter)
> trace__init_syscalls_bpf_prog_array_maps(trace);
>
> diff --git a/tools/perf/examples/bpf/augmented_raw_syscalls.c b/tools/perf/examples/bpf/augmented_raw_syscalls.c
> index 4203f92c063b..9a03189d33d3 100644
> --- a/tools/perf/examples/bpf/augmented_raw_syscalls.c
> +++ b/tools/perf/examples/bpf/augmented_raw_syscalls.c
> @@ -37,23 +37,6 @@ struct __augmented_syscalls__ {
> __uint(max_entries, __NR_CPUS__);
> } __augmented_syscalls__ SEC(".maps");
>
> -/*
> - * string_args_len: one per syscall arg, 0 means not a string or don't copy it,
> - * PATH_MAX for copying everything, any other value to limit
> - * it a la 'strace -s strsize'.
> - */
> -struct syscall {
> - bool enabled;
> - __u16 string_args_len[6];
> -};
> -
> -struct syscalls {
> - __uint(type, BPF_MAP_TYPE_ARRAY);
> - __type(key, int);
> - __type(value, struct syscall);
> - __uint(max_entries, 512);
> -} syscalls SEC(".maps");
> -
> /*
> * What to augment at entry?
> *
> --
> 2.34.1
--
- Arnaldo
next prev parent reply other threads:[~2022-11-21 18:58 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-21 7:52 [PATCH v1 0/5] perf trace: Cleanup and remove unused bpf map Leo Yan
2022-11-21 7:52 ` [PATCH v1 1/5] perf trace: Use macro RAW_SYSCALL_ARGS_NUM to replace number Leo Yan
2022-11-21 7:52 ` [PATCH v1 2/5] perf trace: Return error if a system call doesn't exist Leo Yan
2022-11-21 7:52 ` [PATCH v1 3/5] perf trace: Handle failure when trace point folder is missed Leo Yan
2022-11-21 7:52 ` [PATCH v1 4/5] perf augmented_raw_syscalls: Remove unused variable 'syscall' Leo Yan
2022-11-21 7:52 ` [PATCH v1 5/5] perf trace: Remove unused bpf map 'syscalls' Leo Yan
2022-11-21 18:58 ` Arnaldo Carvalho de Melo [this message]
2022-11-21 17:05 ` [PATCH v1 0/5] perf trace: Cleanup and remove unused bpf map Ian Rogers
2022-11-21 18:58 ` Arnaldo Carvalho de Melo
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=Y3vKRZtaW5Mnxm11@kernel.org \
--to=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=bpf@vger.kernel.org \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=leo.yan@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.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.