| Message ID | 20240926072755.2007-1-eric.yan@oppo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] Add BPF Kernel Function bpf_ptrace_vprintk | expand |
On Thu, Sep 26, 2024 at 12:28 AM Eric Yan <eric.yan@oppo.com> wrote: > > add a kfunc 'bpf_ptrace_vprintk' printing bpf msg with trace_marker > format requirement so that these msgs can be retrieved by android > perfetto by default and well represented in perfetto UI. > > [testing prog] > const volatile bool ptrace_enabled = true; > extern int bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const void *args, u32 args__sz) __ksym; > > ({ \ > if (!ptrace_enabled) { \ > bpf_printk(fmt, __VA_ARGS__); \ > } else { \ > char __fmt[] = fmt; \ > _Pragma("GCC diagnostic push") \ > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > u64 __params[] = { __VA_ARGS__ }; \ > _Pragma("GCC diagnostic pop") \ > bpf_ptrace_vprintk(__fmt, sizeof(__fmt), __params, sizeof(__params)); \ > } \ > }) > > SEC("perf_event") > int do_sample(struct bpf_perf_event_data *ctx) > { > u64 ip = PT_REGS_IP(&ctx->regs); > u64 id = bpf_get_current_pid_tgid(); > s32 pid = id >> 32; > s32 tid = id; > debug_printk("N|%d|BPRF-%d|BPRF:%llx", pid, tid, ip); > return 0; > } > > [output]: > app-3151 [000] d.h1. 6059.904239: tracing_mark_write: N|2491|BPRF-3151|BPRF:58750d0eec > > Signed-off-by: Eric Yan <eric.yan@oppo.com> > --- > kernel/bpf/helpers.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 1a43d06eab28..1e37dae74ca6 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -2521,6 +2521,39 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid) > return p; > } > > +static noinline void tracing_mark_write(char *buf) > +{ > + trace_printk(buf); > +} > + > +/* same as bpf_trace_vprintk, only with a trace_marker format requirement > + * @fmt: Format string, e.g. <B|E|C|N>|<%d:pid>|<%s:TAG>... > + */ > +__bpf_kfunc int bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const void *args, u32 args__sz) > +{ > + struct bpf_bprintf_data data = { > + .get_bin_args = true, > + .get_buf = true, > + }; > + int ret, num_args; > + > + if (args__sz & 7 || args__sz > MAX_BPRINTF_VARARGS * 8 || (args__sz && !args)) > + return -EINVAL; > + num_args = args__sz / 8; > + > + ret = bpf_bprintf_prepare(fmt, fmt_size, args, num_args, &data); > + if (ret < 0) > + return ret; > + > + ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt, data.bin_args); > + > + tracing_mark_write(data.buf); > + > + bpf_bprintf_cleanup(&data); > + > + return ret; > +} > + > /** > * bpf_dynptr_slice() - Obtain a read-only pointer to the dynptr data. > * @p: The dynptr whose data slice to retrieve > @@ -3090,6 +3123,7 @@ BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW) > BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL) > BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY) > BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE) > +BTF_ID_FLAGS(func, bpf_ptrace_vprintk) > BTF_KFUNCS_END(common_btf_ids) Why new kfunc? Use bpf_snprintf() and follow with bpf_trace_printk() ?
This patch is mainly considered based on the Android Perfetto (A powerful trace collection and analysis tool, support ftrace data source). The output of bpf_trace_printk and bpf_vtrace_printk in ftrace is like: app-12345 [001] d... 654321.1970001: bpf_trace_printk: blabla.. FUNCTION field of this kind of message is 'bpf_trace_printk', and there's no standard syntax format for it. Currently, Perfetto doesn't collect 'bpf_trace/bpf_trace_printk' trace event by default, but does support 'tracing_mark_write' function style by default, such as: app-3151 [000] d.h1. 6059.904239: tracing_mark_write: B|2491|BPRF-3151|TracingFunc app-3151 [000] d.h1. 6059.904239: tracing_mark_write: E|2491 Therefore, it's considered to add this kfunc to output formatted BPF messages to ftrace like trace_marker, allowing perfetto to collect and parse 'tracing_mark_write' events by default and eventually visualize them in the perfetto UI. -----邮件原件----- 发件人: Alexei Starovoitov <alexei.starovoitov@gmail.com> 发送时间: 2024年9月30日 1:10 收件人: 燕青洲(Eric Yan) <eric.yan@oppo.com> 抄送: kbuild test robot <lkp@intel.com>; Andrii Nakryiko <andrii@kernel.org>; Alexei Starovoitov <ast@kernel.org>; bpf <bpf@vger.kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Hao Luo <haoluo@google.com>; John Fastabend <john.fastabend@gmail.com>; Jiri Olsa <jolsa@kernel.org>; KP Singh <kpsingh@kernel.org>; LKML <linux-kernel@vger.kernel.org>; Martin KaFai Lau <martin.lau@linux.dev>; oe-kbuild-all@lists.linux.dev; Stanislav Fomichev <sdf@fomichev.me>; Song Liu <song@kernel.org>; Yonghong Song <yonghong.song@linux.dev> 主题: Re: [PATCH v2] Add BPF Kernel Function bpf_ptrace_vprintk On Thu, Sep 26, 2024 at 12:28 AM Eric Yan <eric.yan@oppo.com> wrote: > > add a kfunc 'bpf_ptrace_vprintk' printing bpf msg with trace_marker > format requirement so that these msgs can be retrieved by android > perfetto by default and well represented in perfetto UI. > > [testing prog] > const volatile bool ptrace_enabled = true; extern int > bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const void *args, u32 > args__sz) __ksym; > > ({ \ > if (!ptrace_enabled) { \ > bpf_printk(fmt, __VA_ARGS__); \ > } else { \ > char __fmt[] = fmt; \ > _Pragma("GCC diagnostic push") \ > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > u64 __params[] = { __VA_ARGS__ }; \ > _Pragma("GCC diagnostic pop") \ > bpf_ptrace_vprintk(__fmt, sizeof(__fmt), __params, sizeof(__params)); \ > } \ > }) > > SEC("perf_event") > int do_sample(struct bpf_perf_event_data *ctx) { > u64 ip = PT_REGS_IP(&ctx->regs); > u64 id = bpf_get_current_pid_tgid(); > s32 pid = id >> 32; > s32 tid = id; > debug_printk("N|%d|BPRF-%d|BPRF:%llx", pid, tid, ip); > return 0; > } > > [output]: > app-3151 [000] d.h1. 6059.904239: tracing_mark_write: N|2491|BPRF-3151|BPRF:58750d0eec > > Signed-off-by: Eric Yan <eric.yan@oppo.com> > --- > kernel/bpf/helpers.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index > 1a43d06eab28..1e37dae74ca6 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -2521,6 +2521,39 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid) > return p; > } > > +static noinline void tracing_mark_write(char *buf) { > + trace_printk(buf); > +} > + > +/* same as bpf_trace_vprintk, only with a trace_marker format > +requirement > + * @fmt: Format string, e.g. <B|E|C|N>|<%d:pid>|<%s:TAG>... > + */ > +__bpf_kfunc int bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const > +void *args, u32 args__sz) { > + struct bpf_bprintf_data data = { > + .get_bin_args = true, > + .get_buf = true, > + }; > + int ret, num_args; > + > + if (args__sz & 7 || args__sz > MAX_BPRINTF_VARARGS * 8 || (args__sz && !args)) > + return -EINVAL; > + num_args = args__sz / 8; > + > + ret = bpf_bprintf_prepare(fmt, fmt_size, args, num_args, &data); > + if (ret < 0) > + return ret; > + > + ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt, > + data.bin_args); > + > + tracing_mark_write(data.buf); > + > + bpf_bprintf_cleanup(&data); > + > + return ret; > +} > + > /** > * bpf_dynptr_slice() - Obtain a read-only pointer to the dynptr data. > * @p: The dynptr whose data slice to retrieve @@ -3090,6 +3123,7 @@ > BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW) BTF_ID_FLAGS(func, > bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL) BTF_ID_FLAGS(func, > bpf_iter_bits_destroy, KF_ITER_DESTROY) BTF_ID_FLAGS(func, > bpf_copy_from_user_str, KF_SLEEPABLE) > +BTF_ID_FLAGS(func, bpf_ptrace_vprintk) > BTF_KFUNCS_END(common_btf_ids) Why new kfunc? Use bpf_snprintf() and follow with bpf_trace_printk() ?
On Mon, Sep 30, 2024 at 1:29 AM 燕青洲(Eric Yan) <eric.yan@oppo.com> wrote: > > This patch is mainly considered based on the Android Perfetto (A powerful trace collection and analysis tool, support ftrace data source). > The output of bpf_trace_printk and bpf_vtrace_printk in ftrace is like: > app-12345 [001] d... 654321.1970001: bpf_trace_printk: blabla.. > > FUNCTION field of this kind of message is 'bpf_trace_printk', and there's no standard syntax format for it. > Currently, Perfetto doesn't collect 'bpf_trace/bpf_trace_printk' trace event by default, but does support > 'tracing_mark_write' function style by default, such as: > app-3151 [000] d.h1. 6059.904239: tracing_mark_write: B|2491|BPRF-3151|TracingFunc > app-3151 [000] d.h1. 6059.904239: tracing_mark_write: E|2491 > > Therefore, it's considered to add this kfunc to output formatted BPF messages to ftrace like trace_marker, > allowing perfetto to collect and parse 'tracing_mark_write' events by default and eventually visualize them in the perfetto UI. This does seem like a bit of an overkill to add a new kfunc just to have "tracing_mark_write" instead of "bpf_trace_printk". Is there any chance that perfetto can be changed to also track bpf_trace_printk, perhaps with some pre-agreed upon prefix or something? E.g, app-3151 [000] d.h1. 6059.904239: bpf_trace_printk: !B|2491|BPRF-3151|TracingFunc app-3151 [000] d.h1. 6059.904239: bpf_trace_printk: !E|2491 Generally speaking, bpf_trace_printk() shouldn't be used in production setup (much), so perhaps parsing everything from bpf_trace_printk() is OK (assuming it follows this vertical bar syntax)? > > -----邮件原件----- > 发件人: Alexei Starovoitov <alexei.starovoitov@gmail.com> > 发送时间: 2024年9月30日 1:10 > 收件人: 燕青洲(Eric Yan) <eric.yan@oppo.com> > 抄送: kbuild test robot <lkp@intel.com>; Andrii Nakryiko <andrii@kernel.org>; Alexei Starovoitov <ast@kernel.org>; bpf <bpf@vger.kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Hao Luo <haoluo@google.com>; John Fastabend <john.fastabend@gmail.com>; Jiri Olsa <jolsa@kernel.org>; KP Singh <kpsingh@kernel.org>; LKML <linux-kernel@vger.kernel.org>; Martin KaFai Lau <martin.lau@linux.dev>; oe-kbuild-all@lists.linux.dev; Stanislav Fomichev <sdf@fomichev.me>; Song Liu <song@kernel.org>; Yonghong Song <yonghong.song@linux.dev> > 主题: Re: [PATCH v2] Add BPF Kernel Function bpf_ptrace_vprintk > > On Thu, Sep 26, 2024 at 12:28 AM Eric Yan <eric.yan@oppo.com> wrote: > > > > add a kfunc 'bpf_ptrace_vprintk' printing bpf msg with trace_marker > > format requirement so that these msgs can be retrieved by android > > perfetto by default and well represented in perfetto UI. > > > > [testing prog] > > const volatile bool ptrace_enabled = true; extern int > > bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const void *args, u32 > > args__sz) __ksym; > > > > ({ \ > > if (!ptrace_enabled) { \ > > bpf_printk(fmt, __VA_ARGS__); \ > > } else { \ > > char __fmt[] = fmt; \ > > _Pragma("GCC diagnostic push") \ > > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > > u64 __params[] = { __VA_ARGS__ }; \ > > _Pragma("GCC diagnostic pop") \ > > bpf_ptrace_vprintk(__fmt, sizeof(__fmt), __params, sizeof(__params)); \ > > } \ > > }) > > > > SEC("perf_event") > > int do_sample(struct bpf_perf_event_data *ctx) { > > u64 ip = PT_REGS_IP(&ctx->regs); > > u64 id = bpf_get_current_pid_tgid(); > > s32 pid = id >> 32; > > s32 tid = id; > > debug_printk("N|%d|BPRF-%d|BPRF:%llx", pid, tid, ip); > > return 0; > > } > > > > [output]: > > app-3151 [000] d.h1. 6059.904239: tracing_mark_write: N|2491|BPRF-3151|BPRF:58750d0eec > > > > Signed-off-by: Eric Yan <eric.yan@oppo.com> > > --- > > kernel/bpf/helpers.c | 34 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index > > 1a43d06eab28..1e37dae74ca6 100644 > > --- a/kernel/bpf/helpers.c > > +++ b/kernel/bpf/helpers.c > > @@ -2521,6 +2521,39 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid) > > return p; > > } > > > > +static noinline void tracing_mark_write(char *buf) { > > + trace_printk(buf); > > +} > > + > > +/* same as bpf_trace_vprintk, only with a trace_marker format > > +requirement > > + * @fmt: Format string, e.g. <B|E|C|N>|<%d:pid>|<%s:TAG>... > > + */ > > +__bpf_kfunc int bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const > > +void *args, u32 args__sz) { > > + struct bpf_bprintf_data data = { > > + .get_bin_args = true, > > + .get_buf = true, > > + }; > > + int ret, num_args; > > + > > + if (args__sz & 7 || args__sz > MAX_BPRINTF_VARARGS * 8 || (args__sz && !args)) > > + return -EINVAL; > > + num_args = args__sz / 8; > > + > > + ret = bpf_bprintf_prepare(fmt, fmt_size, args, num_args, &data); > > + if (ret < 0) > > + return ret; > > + > > + ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt, > > + data.bin_args); > > + > > + tracing_mark_write(data.buf); > > + > > + bpf_bprintf_cleanup(&data); > > + > > + return ret; > > +} > > + > > /** > > * bpf_dynptr_slice() - Obtain a read-only pointer to the dynptr data. > > * @p: The dynptr whose data slice to retrieve @@ -3090,6 +3123,7 @@ > > BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW) BTF_ID_FLAGS(func, > > bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL) BTF_ID_FLAGS(func, > > bpf_iter_bits_destroy, KF_ITER_DESTROY) BTF_ID_FLAGS(func, > > bpf_copy_from_user_str, KF_SLEEPABLE) > > +BTF_ID_FLAGS(func, bpf_ptrace_vprintk) > > BTF_KFUNCS_END(common_btf_ids) > > Why new kfunc? > Use bpf_snprintf() and follow with bpf_trace_printk() ?
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 1a43d06eab28..1e37dae74ca6 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -2521,6 +2521,39 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid) return p; } +static noinline void tracing_mark_write(char *buf) +{ + trace_printk(buf); +} + +/* same as bpf_trace_vprintk, only with a trace_marker format requirement + * @fmt: Format string, e.g. <B|E|C|N>|<%d:pid>|<%s:TAG>... + */ +__bpf_kfunc int bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const void *args, u32 args__sz) +{ + struct bpf_bprintf_data data = { + .get_bin_args = true, + .get_buf = true, + }; + int ret, num_args; + + if (args__sz & 7 || args__sz > MAX_BPRINTF_VARARGS * 8 || (args__sz && !args)) + return -EINVAL; + num_args = args__sz / 8; + + ret = bpf_bprintf_prepare(fmt, fmt_size, args, num_args, &data); + if (ret < 0) + return ret; + + ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt, data.bin_args); + + tracing_mark_write(data.buf); + + bpf_bprintf_cleanup(&data); + + return ret; +} + /** * bpf_dynptr_slice() - Obtain a read-only pointer to the dynptr data. * @p: The dynptr whose data slice to retrieve @@ -3090,6 +3123,7 @@ BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW) BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL) BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY) BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE) +BTF_ID_FLAGS(func, bpf_ptrace_vprintk) BTF_KFUNCS_END(common_btf_ids) static const struct btf_kfunc_id_set common_kfunc_set = {
add a kfunc 'bpf_ptrace_vprintk' printing bpf msg with trace_marker format requirement so that these msgs can be retrieved by android perfetto by default and well represented in perfetto UI. [testing prog] const volatile bool ptrace_enabled = true; extern int bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const void *args, u32 args__sz) __ksym; ({ \ if (!ptrace_enabled) { \ bpf_printk(fmt, __VA_ARGS__); \ } else { \ char __fmt[] = fmt; \ _Pragma("GCC diagnostic push") \ _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ u64 __params[] = { __VA_ARGS__ }; \ _Pragma("GCC diagnostic pop") \ bpf_ptrace_vprintk(__fmt, sizeof(__fmt), __params, sizeof(__params)); \ } \ }) SEC("perf_event") int do_sample(struct bpf_perf_event_data *ctx) { u64 ip = PT_REGS_IP(&ctx->regs); u64 id = bpf_get_current_pid_tgid(); s32 pid = id >> 32; s32 tid = id; debug_printk("N|%d|BPRF-%d|BPRF:%llx", pid, tid, ip); return 0; } [output]: app-3151 [000] d.h1. 6059.904239: tracing_mark_write: N|2491|BPRF-3151|BPRF:58750d0eec Signed-off-by: Eric Yan <eric.yan@oppo.com> --- kernel/bpf/helpers.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)