| Message ID | 20230528142027.5585-2-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 |
On Sun, May 28, 2023 at 02:20:20PM +0000, Yafang Shao wrote: > Currently, there is no way to check which functions are attached to a > kprobe_multi link, causing confusion for users. It is important that we > provide a means to expose these functions. The expected result is as follows, > > $ cat /proc/10936/fdinfo/9 > pos: 0 > flags: 02000000 > mnt_id: 15 > ino: 2094 > link_type: kprobe_multi > link_id: 2 > prog_tag: a04f5eef06a7f555 > prog_id: 11 > func_count: 4 > func_addrs: ffffffffaad475c0 > ffffffffaad47600 > ffffffffaad47640 > ffffffffaad47680 I like the idea of exposing this through the link_info syscall, but I'm bit concerned of potentially dumping thousands of addresses through fdinfo file, because I always thought of fdinfo as brief file info, but that might be just my problem ;-) jirka > > $ cat /proc/10936/fdinfo/9 | grep "func_addrs" -A 4 | \ > awk '{ if (NR ==1) {print $2} else {print $1}}' | \ > 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> > --- > kernel/trace/bpf_trace.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 2bc41e6..0d84a7a 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2548,9 +2548,26 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link) > kfree(kmulti_link); > } > > +static void bpf_kprobe_multi_link_show_fdinfo(const struct bpf_link *link, > + struct seq_file *seq) > +{ > + struct bpf_kprobe_multi_link *kmulti_link; > + int i; > + > + kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link); > + seq_printf(seq, "func_count:\t%d\n", kmulti_link->cnt); > + for (i = 0; i < kmulti_link->cnt; i++) { > + if (i == 0) > + seq_printf(seq, "func_addrs:\t%lx\n", kmulti_link->addrs[i]); > + else > + seq_printf(seq, " \t%lx\n", kmulti_link->addrs[i]); > + } > +} > + > static const struct bpf_link_ops bpf_kprobe_multi_link_lops = { > .release = bpf_kprobe_multi_link_release, > .dealloc = bpf_kprobe_multi_link_dealloc, > + .show_fdinfo = bpf_kprobe_multi_link_show_fdinfo, > }; > > static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void *priv) > -- > 1.8.3.1 >
On Mon, May 29, 2023 at 8:06 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Sun, May 28, 2023 at 02:20:20PM +0000, Yafang Shao wrote: > > Currently, there is no way to check which functions are attached to a > > kprobe_multi link, causing confusion for users. It is important that we > > provide a means to expose these functions. The expected result is as follows, > > > > $ cat /proc/10936/fdinfo/9 > > pos: 0 > > flags: 02000000 > > mnt_id: 15 > > ino: 2094 > > link_type: kprobe_multi > > link_id: 2 > > prog_tag: a04f5eef06a7f555 > > prog_id: 11 > > func_count: 4 > > func_addrs: ffffffffaad475c0 > > ffffffffaad47600 > > ffffffffaad47640 > > ffffffffaad47680 > > I like the idea of exposing this through the link_info syscall, > but I'm bit concerned of potentially dumping thousands of addresses > through fdinfo file, because I always thought of fdinfo as brief > file info, but that might be just my problem ;-) In most cases, there are only a few addresses, and it is uncommon to have thousands of addresses. To handle this, what about displaying a maximum of 16 addresses? For cases where the number of addresses exceeds 16, we can use '...' to represent the remaining addresses.
On Tue, May 30, 2023 at 09:39:01AM +0800, Yafang Shao wrote: > On Mon, May 29, 2023 at 8:06 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Sun, May 28, 2023 at 02:20:20PM +0000, Yafang Shao wrote: > > > Currently, there is no way to check which functions are attached to a > > > kprobe_multi link, causing confusion for users. It is important that we > > > provide a means to expose these functions. The expected result is as follows, > > > > > > $ cat /proc/10936/fdinfo/9 > > > pos: 0 > > > flags: 02000000 > > > mnt_id: 15 > > > ino: 2094 > > > link_type: kprobe_multi > > > link_id: 2 > > > prog_tag: a04f5eef06a7f555 > > > prog_id: 11 > > > func_count: 4 > > > func_addrs: ffffffffaad475c0 > > > ffffffffaad47600 > > > ffffffffaad47640 > > > ffffffffaad47680 > > > > I like the idea of exposing this through the link_info syscall, > > but I'm bit concerned of potentially dumping thousands of addresses > > through fdinfo file, because I always thought of fdinfo as brief > > file info, but that might be just my problem ;-) > > In most cases, there are only a few addresses, and it is uncommon to I doubt you have data to prove that kprobe_multi is "few addresses in most cases", so please don't throw such arguments without proof. > have thousands of addresses. To handle this, what about displaying a > maximum of 16 addresses? For cases where the number of addresses > exceeds 16, we can use '...' to represent the remaining addresses. at this point the kernel can pick random 16 kernel funcs and it won't be much worse. Asking users to do $ cat /proc/10936/fdinfo/9 | grep "func_addrs" -A 4 | \ awk '{ if (NR ==1) {print $2} else {print $1}}' | \ awk '{"grep " $1 " /proc/kallsyms"| getline f; print f}' ffffffffaad475c0 T schedule_timeout_interruptible ffffffffaad47600 T schedule_timeout_killable isn't a great interface either. The proper interface through fill_link_info and bpftool is good to have, but fdinfo shouldn't partially duplicate it. So drop this patch and others.
On Wed, May 31, 2023 at 8:29 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, May 30, 2023 at 09:39:01AM +0800, Yafang Shao wrote: > > On Mon, May 29, 2023 at 8:06 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > > > On Sun, May 28, 2023 at 02:20:20PM +0000, Yafang Shao wrote: > > > > Currently, there is no way to check which functions are attached to a > > > > kprobe_multi link, causing confusion for users. It is important that we > > > > provide a means to expose these functions. The expected result is as follows, > > > > > > > > $ cat /proc/10936/fdinfo/9 > > > > pos: 0 > > > > flags: 02000000 > > > > mnt_id: 15 > > > > ino: 2094 > > > > link_type: kprobe_multi > > > > link_id: 2 > > > > prog_tag: a04f5eef06a7f555 > > > > prog_id: 11 > > > > func_count: 4 > > > > func_addrs: ffffffffaad475c0 > > > > ffffffffaad47600 > > > > ffffffffaad47640 > > > > ffffffffaad47680 > > > > > > I like the idea of exposing this through the link_info syscall, > > > but I'm bit concerned of potentially dumping thousands of addresses > > > through fdinfo file, because I always thought of fdinfo as brief > > > file info, but that might be just my problem ;-) > > > > In most cases, there are only a few addresses, and it is uncommon to > > I doubt you have data to prove that kprobe_multi is "few addresses in most cases", > so please don't throw such arguments without proof. > > > have thousands of addresses. To handle this, what about displaying a > > maximum of 16 addresses? For cases where the number of addresses > > exceeds 16, we can use '...' to represent the remaining addresses. > > at this point the kernel can pick random 16 kernel funcs and it won't be > much worse. > > Asking users to do > $ cat /proc/10936/fdinfo/9 | grep "func_addrs" -A 4 | \ > awk '{ if (NR ==1) {print $2} else {print $1}}' | \ > awk '{"grep " $1 " /proc/kallsyms"| getline f; print f}' > ffffffffaad475c0 T schedule_timeout_interruptible > ffffffffaad47600 T schedule_timeout_killable > > isn't a great interface either. > > The proper interface through fill_link_info and bpftool is good to have, > but fdinfo shouldn't partially duplicate it. So drop this patch and others. Sure, I will drop the ->show_fdinfo patches.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 2bc41e6..0d84a7a 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2548,9 +2548,26 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link) kfree(kmulti_link); } +static void bpf_kprobe_multi_link_show_fdinfo(const struct bpf_link *link, + struct seq_file *seq) +{ + struct bpf_kprobe_multi_link *kmulti_link; + int i; + + kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link); + seq_printf(seq, "func_count:\t%d\n", kmulti_link->cnt); + for (i = 0; i < kmulti_link->cnt; i++) { + if (i == 0) + seq_printf(seq, "func_addrs:\t%lx\n", kmulti_link->addrs[i]); + else + seq_printf(seq, " \t%lx\n", kmulti_link->addrs[i]); + } +} + static const struct bpf_link_ops bpf_kprobe_multi_link_lops = { .release = bpf_kprobe_multi_link_release, .dealloc = bpf_kprobe_multi_link_dealloc, + .show_fdinfo = bpf_kprobe_multi_link_show_fdinfo, }; static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void *priv)
Currently, there is no way to check which functions are attached to a kprobe_multi link, causing confusion for users. It is important that we provide a means to expose these functions. The expected result is as follows, $ cat /proc/10936/fdinfo/9 pos: 0 flags: 02000000 mnt_id: 15 ino: 2094 link_type: kprobe_multi link_id: 2 prog_tag: a04f5eef06a7f555 prog_id: 11 func_count: 4 func_addrs: ffffffffaad475c0 ffffffffaad47600 ffffffffaad47640 ffffffffaad47680 $ cat /proc/10936/fdinfo/9 | grep "func_addrs" -A 4 | \ awk '{ if (NR ==1) {print $2} else {print $1}}' | \ 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> --- kernel/trace/bpf_trace.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)