| Message ID | 20230528142027.5585-4-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Support ->show_fdinfo and ->fill_link_info for kprobe prog | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain_full }} |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for ShellCheck |
| bpf/vmtest-bpf-next-VM_Test-3 | fail | Logs for build for aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | fail | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-5 | fail | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | fail | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for veristat |
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for bpf-next, async |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
2023-05-28 14:20 UTC+0000 ~ Yafang Shao <laoar.shao@gmail.com> > Show the already expose kprobe_multi link info in bpftool. The result as > follows, > > $ bpftool link show > 2: kprobe_multi prog 11 > func_cnt 4 addrs ffffffffaad475c0 ffffffffaad47600 > ffffffffaad47640 ffffffffaad47680 > pids trace(10936) > > $ bpftool link show -j > [{"id":1,"type":"perf_event","prog_id":5,"bpf_cookie":0,"pids":[{"pid":10658,"comm":"trace"}]},{"id":2,"type":"kprobe_multi","prog_id":11,"func_cnt":4,"addrs":[18446744072280634816,18446744072280634880,18446744072280634944,18446744072280635008],"pids":[{"pid":10936,"comm":"trace"}]},{"id":120,"type":"iter","prog_id":266,"target_name":"bpf_map"},{"id":121,"type":"iter","prog_id":267,"target_name":"bpf_prog"}] > > $ bpftool link show | grep -A 1 "func_cnt" | \ > awk '{if (NR == 1) {print $4; print $5;} else {print $1; print $2} }' | \ > awk '{"grep " $1 " /proc/kallsyms" | getline f; print f;}' > ffffffffaad475c0 T schedule_timeout_interruptible > ffffffffaad47600 T schedule_timeout_killable > ffffffffaad47640 T schedule_timeout_uninterruptible > ffffffffaad47680 T schedule_timeout_idle Looks nice, thank you! The address is a useful addition, but I feel like most of the time, this is the actual function name that we'd like to see. We could maybe print it directly in bpftool, what do you think? We already parse /proc/kallsyms elsewhere (to get the address of __bpf_call_base()). If we can parse the file only once for all func_cnt function, the overhead is maybe acceptable? > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > tools/bpf/bpftool/link.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > index 2d78607..76f1bb2 100644 > --- a/tools/bpf/bpftool/link.c > +++ b/tools/bpf/bpftool/link.c > @@ -218,6 +218,20 @@ static int show_link_close_json(int fd, struct bpf_link_info *info) > jsonw_uint_field(json_wtr, "map_id", > info->struct_ops.map_id); > break; > + case BPF_LINK_TYPE_KPROBE_MULTI: > + const __u64 *addrs; > + __u32 i; > + > + jsonw_uint_field(json_wtr, "func_cnt", info->kprobe_multi.count); > + if (!info->kprobe_multi.count) > + break; I'd as well avoid having conditional entries in the JSON output. Let's just keep 0 and empty array in this case? > + jsonw_name(json_wtr, "addrs"); > + jsonw_start_array(json_wtr); > + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); > + for (i = 0; i < info->kprobe_multi.count; i++) > + jsonw_lluint(json_wtr, addrs[i]); > + jsonw_end_array(json_wtr); > + break; > default: > break; > } > @@ -396,6 +410,24 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) > case BPF_LINK_TYPE_NETFILTER: > netfilter_dump_plain(info); > break; > + case BPF_LINK_TYPE_KPROBE_MULTI: > + __u32 indent, cnt, i; > + const __u64 *addrs; > + > + cnt = info->kprobe_multi.count; > + if (!cnt) > + break; > + printf("\n\tfunc_cnt %d addrs", cnt); > + for (i = 0; cnt; i++) > + cnt /= 10; > + indent = strlen("func_cnt ") + i + strlen(" addrs"); > + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); > + for (i = 0; i < info->kprobe_multi.count; i++) { > + if (i && !(i & 0x1)) > + printf("\n\t%*s", indent, ""); > + printf(" %0*llx", 16, addrs[i]); > + } > + break; > default: > break; > } > @@ -417,7 +449,9 @@ static int do_show_link(int fd) > { > struct bpf_link_info info; > __u32 len = sizeof(info); > + __u64 *addrs = NULL; > char buf[256]; > + int count; > int err; > > memset(&info, 0, sizeof(info)); > @@ -441,12 +475,28 @@ static int do_show_link(int fd) > info.iter.target_name_len = sizeof(buf); > goto again; > } > + if (info.type == BPF_LINK_TYPE_KPROBE_MULTI && > + !info.kprobe_multi.addrs) { > + count = info.kprobe_multi.count; > + if (count) { > + addrs = malloc(count * sizeof(__u64)); Nit: calloc() instead? > + if (!addrs) { > + p_err("mem alloc failed"); > + close(fd); > + return -1; > + } > + info.kprobe_multi.addrs = (unsigned long)addrs; > + goto again; > + } > + } > > if (json_output) > show_link_close_json(fd, &info); > else > show_link_close_plain(fd, &info); > > + if (addrs) > + free(addrs); > close(fd); > return 0; > } The other bpftool patch (perf_event link) looks good to me. Thanks, Quentin
On Sun, May 28, 2023 at 02:20:22PM +0000, Yafang Shao wrote: > Show the already expose kprobe_multi link info in bpftool. The result as > follows, > > $ bpftool link show > 2: kprobe_multi prog 11 > func_cnt 4 addrs ffffffffaad475c0 ffffffffaad47600 > ffffffffaad47640 ffffffffaad47680 > pids trace(10936) > > $ bpftool link show -j > [{"id":1,"type":"perf_event","prog_id":5,"bpf_cookie":0,"pids":[{"pid":10658,"comm":"trace"}]},{"id":2,"type":"kprobe_multi","prog_id":11,"func_cnt":4,"addrs":[18446744072280634816,18446744072280634880,18446744072280634944,18446744072280635008],"pids":[{"pid":10936,"comm":"trace"}]},{"id":120,"type":"iter","prog_id":266,"target_name":"bpf_map"},{"id":121,"type":"iter","prog_id":267,"target_name":"bpf_prog"}] > > $ bpftool link show | grep -A 1 "func_cnt" | \ > awk '{if (NR == 1) {print $4; print $5;} else {print $1; print $2} }' | \ > awk '{"grep " $1 " /proc/kallsyms" | getline f; print f;}' This is not human friendly. Either make bpftool print complete info or don't do it all. > ffffffffaad475c0 T schedule_timeout_interruptible > ffffffffaad47600 T schedule_timeout_killable > ffffffffaad47640 T schedule_timeout_uninterruptible > ffffffffaad47680 T schedule_timeout_idle > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > tools/bpf/bpftool/link.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > index 2d78607..76f1bb2 100644 > --- a/tools/bpf/bpftool/link.c > +++ b/tools/bpf/bpftool/link.c > @@ -218,6 +218,20 @@ static int show_link_close_json(int fd, struct bpf_link_info *info) > jsonw_uint_field(json_wtr, "map_id", > info->struct_ops.map_id); > break; > + case BPF_LINK_TYPE_KPROBE_MULTI: > + const __u64 *addrs; > + __u32 i; > + > + jsonw_uint_field(json_wtr, "func_cnt", info->kprobe_multi.count); > + if (!info->kprobe_multi.count) > + break; > + jsonw_name(json_wtr, "addrs"); > + jsonw_start_array(json_wtr); > + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); > + for (i = 0; i < info->kprobe_multi.count; i++) > + jsonw_lluint(json_wtr, addrs[i]); > + jsonw_end_array(json_wtr); > + break; > default: > break; > } > @@ -396,6 +410,24 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) > case BPF_LINK_TYPE_NETFILTER: > netfilter_dump_plain(info); > break; > + case BPF_LINK_TYPE_KPROBE_MULTI: > + __u32 indent, cnt, i; > + const __u64 *addrs; > + > + cnt = info->kprobe_multi.count; > + if (!cnt) > + break; > + printf("\n\tfunc_cnt %d addrs", cnt); > + for (i = 0; cnt; i++) > + cnt /= 10; > + indent = strlen("func_cnt ") + i + strlen(" addrs"); > + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); > + for (i = 0; i < info->kprobe_multi.count; i++) { > + if (i && !(i & 0x1)) > + printf("\n\t%*s", indent, ""); > + printf(" %0*llx", 16, addrs[i]); > + } > + break; > default: > break; > } > @@ -417,7 +449,9 @@ static int do_show_link(int fd) > { > struct bpf_link_info info; > __u32 len = sizeof(info); > + __u64 *addrs = NULL; > char buf[256]; > + int count; > int err; > > memset(&info, 0, sizeof(info)); > @@ -441,12 +475,28 @@ static int do_show_link(int fd) > info.iter.target_name_len = sizeof(buf); > goto again; > } > + if (info.type == BPF_LINK_TYPE_KPROBE_MULTI && > + !info.kprobe_multi.addrs) { > + count = info.kprobe_multi.count; > + if (count) { > + addrs = malloc(count * sizeof(__u64)); > + if (!addrs) { > + p_err("mem alloc failed"); > + close(fd); > + return -1; > + } > + info.kprobe_multi.addrs = (unsigned long)addrs; > + goto again; > + } > + } > > if (json_output) > show_link_close_json(fd, &info); > else > show_link_close_plain(fd, &info); > > + if (addrs) > + free(addrs); > close(fd); > return 0; > } > -- > 1.8.3.1 >
On Tue, May 30, 2023 at 7:16 PM Quentin Monnet <quentin@isovalent.com> wrote: > > 2023-05-28 14:20 UTC+0000 ~ Yafang Shao <laoar.shao@gmail.com> > > Show the already expose kprobe_multi link info in bpftool. The result as > > follows, > > > > $ bpftool link show > > 2: kprobe_multi prog 11 > > func_cnt 4 addrs ffffffffaad475c0 ffffffffaad47600 > > ffffffffaad47640 ffffffffaad47680 > > pids trace(10936) > > > > $ bpftool link show -j > > [{"id":1,"type":"perf_event","prog_id":5,"bpf_cookie":0,"pids":[{"pid":10658,"comm":"trace"}]},{"id":2,"type":"kprobe_multi","prog_id":11,"func_cnt":4,"addrs":[18446744072280634816,18446744072280634880,18446744072280634944,18446744072280635008],"pids":[{"pid":10936,"comm":"trace"}]},{"id":120,"type":"iter","prog_id":266,"target_name":"bpf_map"},{"id":121,"type":"iter","prog_id":267,"target_name":"bpf_prog"}] > > > > $ bpftool link show | grep -A 1 "func_cnt" | \ > > awk '{if (NR == 1) {print $4; print $5;} else {print $1; print $2} }' | \ > > awk '{"grep " $1 " /proc/kallsyms" | getline f; print f;}' > > ffffffffaad475c0 T schedule_timeout_interruptible > > ffffffffaad47600 T schedule_timeout_killable > > ffffffffaad47640 T schedule_timeout_uninterruptible > > ffffffffaad47680 T schedule_timeout_idle > > Looks nice, thank you! > > The address is a useful addition, but I feel like most of the time, this > is the actual function name that we'd like to see. We could maybe print > it directly in bpftool, what do you think? We already parse > /proc/kallsyms elsewhere (to get the address of __bpf_call_base()). If > we can parse the file only once for all func_cnt function, the overhead > is maybe acceptable? > Thanks for your suggestion. Will change it. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > tools/bpf/bpftool/link.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > > index 2d78607..76f1bb2 100644 > > --- a/tools/bpf/bpftool/link.c > > +++ b/tools/bpf/bpftool/link.c > > @@ -218,6 +218,20 @@ static int show_link_close_json(int fd, struct bpf_link_info *info) > > jsonw_uint_field(json_wtr, "map_id", > > info->struct_ops.map_id); > > break; > > + case BPF_LINK_TYPE_KPROBE_MULTI: > > + const __u64 *addrs; > > + __u32 i; > > + > > + jsonw_uint_field(json_wtr, "func_cnt", info->kprobe_multi.count); > > + if (!info->kprobe_multi.count) > > + break; > > I'd as well avoid having conditional entries in the JSON output. Let's > just keep 0 and empty array in this case? > Will do it. > > + jsonw_name(json_wtr, "addrs"); > > + jsonw_start_array(json_wtr); > > + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); > > + for (i = 0; i < info->kprobe_multi.count; i++) > > + jsonw_lluint(json_wtr, addrs[i]); > > + jsonw_end_array(json_wtr); > > + break; > > default: > > break; > > } > > @@ -396,6 +410,24 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) > > case BPF_LINK_TYPE_NETFILTER: > > netfilter_dump_plain(info); > > break; > > + case BPF_LINK_TYPE_KPROBE_MULTI: > > + __u32 indent, cnt, i; > > + const __u64 *addrs; > > + > > + cnt = info->kprobe_multi.count; > > + if (!cnt) > > + break; > > + printf("\n\tfunc_cnt %d addrs", cnt); > > + for (i = 0; cnt; i++) > > + cnt /= 10; > > + indent = strlen("func_cnt ") + i + strlen(" addrs"); > > + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); > > + for (i = 0; i < info->kprobe_multi.count; i++) { > > + if (i && !(i & 0x1)) > > + printf("\n\t%*s", indent, ""); > > + printf(" %0*llx", 16, addrs[i]); > > + } > > + break; > > default: > > break; > > } > > @@ -417,7 +449,9 @@ static int do_show_link(int fd) > > { > > struct bpf_link_info info; > > __u32 len = sizeof(info); > > + __u64 *addrs = NULL; > > char buf[256]; > > + int count; > > int err; > > > > memset(&info, 0, sizeof(info)); > > @@ -441,12 +475,28 @@ static int do_show_link(int fd) > > info.iter.target_name_len = sizeof(buf); > > goto again; > > } > > + if (info.type == BPF_LINK_TYPE_KPROBE_MULTI && > > + !info.kprobe_multi.addrs) { > > + count = info.kprobe_multi.count; > > + if (count) { > > + addrs = malloc(count * sizeof(__u64)); > > Nit: calloc() instead? Good point. Will do it. > > > + if (!addrs) { > > + p_err("mem alloc failed"); > > + close(fd); > > + return -1; > > + } > > + info.kprobe_multi.addrs = (unsigned long)addrs; > > + goto again; > > + } > > + } > > > > if (json_output) > > show_link_close_json(fd, &info); > > else > > show_link_close_plain(fd, &info); > > > > + if (addrs) > > + free(addrs); > > close(fd); > > return 0; > > } > > The other bpftool patch (perf_event link) looks good to me. > Thanks for your review.
On Wed, May 31, 2023 at 8:31 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sun, May 28, 2023 at 02:20:22PM +0000, Yafang Shao wrote: > > Show the already expose kprobe_multi link info in bpftool. The result as > > follows, > > > > $ bpftool link show > > 2: kprobe_multi prog 11 > > func_cnt 4 addrs ffffffffaad475c0 ffffffffaad47600 > > ffffffffaad47640 ffffffffaad47680 > > pids trace(10936) > > > > $ bpftool link show -j > > [{"id":1,"type":"perf_event","prog_id":5,"bpf_cookie":0,"pids":[{"pid":10658,"comm":"trace"}]},{"id":2,"type":"kprobe_multi","prog_id":11,"func_cnt":4,"addrs":[18446744072280634816,18446744072280634880,18446744072280634944,18446744072280635008],"pids":[{"pid":10936,"comm":"trace"}]},{"id":120,"type":"iter","prog_id":266,"target_name":"bpf_map"},{"id":121,"type":"iter","prog_id":267,"target_name":"bpf_prog"}] > > > > $ bpftool link show | grep -A 1 "func_cnt" | \ > > awk '{if (NR == 1) {print $4; print $5;} else {print $1; print $2} }' | \ > > awk '{"grep " $1 " /proc/kallsyms" | getline f; print f;}' > > This is not human friendly. > Either make bpftool print complete info or don't do it all. > I will make bpftool print complete info in the next version.
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c index 2d78607..76f1bb2 100644 --- a/tools/bpf/bpftool/link.c +++ b/tools/bpf/bpftool/link.c @@ -218,6 +218,20 @@ static int show_link_close_json(int fd, struct bpf_link_info *info) jsonw_uint_field(json_wtr, "map_id", info->struct_ops.map_id); break; + case BPF_LINK_TYPE_KPROBE_MULTI: + const __u64 *addrs; + __u32 i; + + jsonw_uint_field(json_wtr, "func_cnt", info->kprobe_multi.count); + if (!info->kprobe_multi.count) + break; + jsonw_name(json_wtr, "addrs"); + jsonw_start_array(json_wtr); + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); + for (i = 0; i < info->kprobe_multi.count; i++) + jsonw_lluint(json_wtr, addrs[i]); + jsonw_end_array(json_wtr); + break; default: break; } @@ -396,6 +410,24 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) case BPF_LINK_TYPE_NETFILTER: netfilter_dump_plain(info); break; + case BPF_LINK_TYPE_KPROBE_MULTI: + __u32 indent, cnt, i; + const __u64 *addrs; + + cnt = info->kprobe_multi.count; + if (!cnt) + break; + printf("\n\tfunc_cnt %d addrs", cnt); + for (i = 0; cnt; i++) + cnt /= 10; + indent = strlen("func_cnt ") + i + strlen(" addrs"); + addrs = (const __u64 *)u64_to_ptr(info->kprobe_multi.addrs); + for (i = 0; i < info->kprobe_multi.count; i++) { + if (i && !(i & 0x1)) + printf("\n\t%*s", indent, ""); + printf(" %0*llx", 16, addrs[i]); + } + break; default: break; } @@ -417,7 +449,9 @@ static int do_show_link(int fd) { struct bpf_link_info info; __u32 len = sizeof(info); + __u64 *addrs = NULL; char buf[256]; + int count; int err; memset(&info, 0, sizeof(info)); @@ -441,12 +475,28 @@ static int do_show_link(int fd) info.iter.target_name_len = sizeof(buf); goto again; } + if (info.type == BPF_LINK_TYPE_KPROBE_MULTI && + !info.kprobe_multi.addrs) { + count = info.kprobe_multi.count; + if (count) { + addrs = malloc(count * sizeof(__u64)); + if (!addrs) { + p_err("mem alloc failed"); + close(fd); + return -1; + } + info.kprobe_multi.addrs = (unsigned long)addrs; + goto again; + } + } if (json_output) show_link_close_json(fd, &info); else show_link_close_plain(fd, &info); + if (addrs) + free(addrs); close(fd); return 0; }
Show the already expose kprobe_multi link info in bpftool. The result as follows, $ bpftool link show 2: kprobe_multi prog 11 func_cnt 4 addrs ffffffffaad475c0 ffffffffaad47600 ffffffffaad47640 ffffffffaad47680 pids trace(10936) $ bpftool link show -j [{"id":1,"type":"perf_event","prog_id":5,"bpf_cookie":0,"pids":[{"pid":10658,"comm":"trace"}]},{"id":2,"type":"kprobe_multi","prog_id":11,"func_cnt":4,"addrs":[18446744072280634816,18446744072280634880,18446744072280634944,18446744072280635008],"pids":[{"pid":10936,"comm":"trace"}]},{"id":120,"type":"iter","prog_id":266,"target_name":"bpf_map"},{"id":121,"type":"iter","prog_id":267,"target_name":"bpf_prog"}] $ bpftool link show | grep -A 1 "func_cnt" | \ awk '{if (NR == 1) {print $4; print $5;} else {print $1; print $2} }' | \ awk '{"grep " $1 " /proc/kallsyms" | getline f; print f;}' ffffffffaad475c0 T schedule_timeout_interruptible ffffffffaad47600 T schedule_timeout_killable ffffffffaad47640 T schedule_timeout_uninterruptible ffffffffaad47680 T schedule_timeout_idle Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- tools/bpf/bpftool/link.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)