| Message ID | 20220204181146.8429-1-9erthalion6@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [RFC,v2] bpftool: Add bpf_cookie to link output | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next | success | VM_Test |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| netdev/tree_selection | success | Not a local patch, async |
On 2/4/22 10:11 AM, Dmitrii Dolgov wrote: > Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for > BPF perf links") introduced the concept of user specified bpf_cookie, > which could be accessed by BPF programs using bpf_get_attach_cookie(). > For troubleshooting purposes it is convenient to expose bpf_cookie via > bpftool as well, so there is no need to meddle with the target BPF > program itself. > > $ bpftool link > 1: type 7 prog 5 bpf_cookie 123 > pids bootstrap(87) > > Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> > --- > Changes in v2: > - Display bpf_cookie in bpftool link command instead perf > > Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com > > include/uapi/linux/bpf.h | 3 +++ > kernel/bpf/syscall.c | 13 +++++++++++++ > tools/bpf/bpftool/link.c | 2 ++ > tools/include/uapi/linux/bpf.h | 3 +++ Could you change this patch into two separate ones? The subject 'bpftool' sounds like it is a bpftool change but actually it also includes kernel bpf change. Maybe: patch 1: prefix: bpf include/uapi/linux/bpf.h kernel/bpf/syscall.c tools/include/uapi/linux/bpf.h patch 2: prefix: bpftool tools/bpf/bpftool/link.c > 4 files changed, 21 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index a7f0ddedac1f..600da4496404 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -5850,6 +5850,9 @@ struct bpf_link_info { > __u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */ > __u32 target_btf_id; /* BTF type id inside the object */ > } tracing; > + struct { > + __u64 bpf_cookie; > + } perf; > struct { > __u64 cgroup_id; > __u32 attach_type; > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 72ce1edde950..94b7fa777fc7 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2948,6 +2948,7 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = { > struct bpf_perf_link { > struct bpf_link link; > struct file *perf_file; > + u64 bpf_cookie; > }; > > static void bpf_perf_link_release(struct bpf_link *link) > @@ -2966,9 +2967,20 @@ static void bpf_perf_link_dealloc(struct bpf_link *link) > kfree(perf_link); > } > > +static int bpf_perf_link_fill_link_info(const struct bpf_link *link, > + struct bpf_link_info *info) > +{ > + struct bpf_perf_link *perf_link = > + container_of(link, struct bpf_perf_link, link); > + > + info->perf.bpf_cookie = perf_link->bpf_cookie; I think we don't need bpf_cookie in bpf_perf_link. This is a low frequency event. You can get the information from perf_link->perf_file. Could you check whether the following works or not? struct perf_event *event; struct file *perf_file; perf_file = perf_link->perf_file; event = perf_file->private_data; info->perf.bpf_cookie = event->bpf_cookie; > + return 0; > +} > + > static const struct bpf_link_ops bpf_perf_link_lops = { > .release = bpf_perf_link_release, > .dealloc = bpf_perf_link_dealloc, > + .fill_link_info = bpf_perf_link_fill_link_info, > }; > > static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog) > @@ -2993,6 +3005,7 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro > } > bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog); > link->perf_file = perf_file; > + link->bpf_cookie = attr->link_create.perf_event.bpf_cookie; > > err = bpf_link_prime(&link->link, &link_primer); > if (err) { > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > index 97dec81950e5..3ddeacb3593f 100644 > --- a/tools/bpf/bpftool/link.c > +++ b/tools/bpf/bpftool/link.c > @@ -243,6 +243,8 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) > printf("\n\tnetns_ino %u ", info->netns.netns_ino); > show_link_attach_type_plain(info->netns.attach_type); > break; > + case BPF_LINK_TYPE_PERF_EVENT: > + printf("\n\tbpf_cookie %llu ", info->perf.bpf_cookie); > default: > break; > } > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index a7f0ddedac1f..600da4496404 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -5850,6 +5850,9 @@ struct bpf_link_info { > __u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */ > __u32 target_btf_id; /* BTF type id inside the object */ > } tracing; > + struct { > + __u64 bpf_cookie; > + } perf; > struct { > __u64 cgroup_id; > __u32 attach_type;
On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> wrote: > > Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for > BPF perf links") introduced the concept of user specified bpf_cookie, > which could be accessed by BPF programs using bpf_get_attach_cookie(). > For troubleshooting purposes it is convenient to expose bpf_cookie via > bpftool as well, so there is no need to meddle with the target BPF > program itself. > > $ bpftool link > 1: type 7 prog 5 bpf_cookie 123 > pids bootstrap(87) > > Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> > --- > Changes in v2: > - Display bpf_cookie in bpftool link command instead perf > > Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com So I think this change is pretty straightforward and I don't mind it, but I'm not clear how this approach will scale to multi-attach kprobe and fentry programs. For those, users will be specifying many bpf cookies, one per each target attach function. At that point we'll have a bunch of cookies sorted by the attach function IP (not necessarily in the original order). I don't think it will be all that useful and interesting to the end user. We won't be storing original function names (too much memory for storing something that most probably won't be ever queried), so restoring original order and original function names will be hard. If we don't think this through, we'll end up with kernel API that works for just one simple use case. Can you please describe your use case which motivated this feature in the first place to better understand the importance of this? BTW, bpftool can technically implement this today without kernel changes by fetching such bpf_cookies from the kernel using its pid iterator BPF program. See skeleton/pid_iter.bpf.c for pointers. I wonder if it would make more sense to start with doing this purely on the bpftool side first. As an aside (and probably something more generally useful), it seems like we have a bpf_iter__bpf_map iterator, but we don't have prog and link iterators implemented. Would it be a good idea to add that to the kernel? Yonghong, Alexei, any thoughts? > > include/uapi/linux/bpf.h | 3 +++ > kernel/bpf/syscall.c | 13 +++++++++++++ > tools/bpf/bpftool/link.c | 2 ++ > tools/include/uapi/linux/bpf.h | 3 +++ > 4 files changed, 21 insertions(+) > [...]
On 2/7/22 2:11 PM, Andrii Nakryiko wrote: > On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> wrote: >> >> Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for >> BPF perf links") introduced the concept of user specified bpf_cookie, >> which could be accessed by BPF programs using bpf_get_attach_cookie(). >> For troubleshooting purposes it is convenient to expose bpf_cookie via >> bpftool as well, so there is no need to meddle with the target BPF >> program itself. >> >> $ bpftool link >> 1: type 7 prog 5 bpf_cookie 123 >> pids bootstrap(87) >> >> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> >> --- >> Changes in v2: >> - Display bpf_cookie in bpftool link command instead perf >> >> Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com > > > So I think this change is pretty straightforward and I don't mind it, > but I'm not clear how this approach will scale to multi-attach kprobe > and fentry programs. For those, users will be specifying many bpf > cookies, one per each target attach function. At that point we'll have > a bunch of cookies sorted by the attach function IP (not necessarily > in the original order). I don't think it will be all that useful and > interesting to the end user. We won't be storing original function > names (too much memory for storing something that most probably won't > be ever queried), so restoring original order and original function > names will be hard. If we don't think this through, we'll end up with > kernel API that works for just one simple use case. The cookie for multi-attachment is indeed a problem. Some of original cookies may not be available any more. > > Can you please describe your use case which motivated this feature in > the first place to better understand the importance of this? > > BTW, bpftool can technically implement this today without kernel > changes by fetching such bpf_cookies from the kernel using its pid > iterator BPF program. See skeleton/pid_iter.bpf.c for pointers. I > wonder if it would make more sense to start with doing this purely on > the bpftool side first. > > As an aside (and probably something more generally useful), it seems > like we have a bpf_iter__bpf_map iterator, but we don't have prog and > link iterators implemented. Would it be a good idea to add that to the > kernel? Yonghong, Alexei, any thoughts? We already have program iterator. We have discussed link iterators for sometime. As more and more usages for links, a link iterator should be good to improve performance compared to generic 'task/file' iterator. > >> >> include/uapi/linux/bpf.h | 3 +++ >> kernel/bpf/syscall.c | 13 +++++++++++++ >> tools/bpf/bpftool/link.c | 2 ++ >> tools/include/uapi/linux/bpf.h | 3 +++ >> 4 files changed, 21 insertions(+) >> > > [...]
On Mon, Feb 7, 2022 at 9:46 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 2/7/22 2:11 PM, Andrii Nakryiko wrote: > > On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> wrote: > >> > >> Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for > >> BPF perf links") introduced the concept of user specified bpf_cookie, > >> which could be accessed by BPF programs using bpf_get_attach_cookie(). > >> For troubleshooting purposes it is convenient to expose bpf_cookie via > >> bpftool as well, so there is no need to meddle with the target BPF > >> program itself. > >> > >> $ bpftool link > >> 1: type 7 prog 5 bpf_cookie 123 > >> pids bootstrap(87) > >> > >> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> > >> --- > >> Changes in v2: > >> - Display bpf_cookie in bpftool link command instead perf > >> > >> Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com > > > > > > So I think this change is pretty straightforward and I don't mind it, > > but I'm not clear how this approach will scale to multi-attach kprobe > > and fentry programs. For those, users will be specifying many bpf > > cookies, one per each target attach function. At that point we'll have > > a bunch of cookies sorted by the attach function IP (not necessarily > > in the original order). I don't think it will be all that useful and > > interesting to the end user. We won't be storing original function > > names (too much memory for storing something that most probably won't > > be ever queried), so restoring original order and original function > > names will be hard. If we don't think this through, we'll end up with > > kernel API that works for just one simple use case. > > The cookie for multi-attachment is indeed a problem. Some of original > cookies may not be available any more. > > > > > Can you please describe your use case which motivated this feature in > > the first place to better understand the importance of this? > > > > BTW, bpftool can technically implement this today without kernel > > changes by fetching such bpf_cookies from the kernel using its pid > > iterator BPF program. See skeleton/pid_iter.bpf.c for pointers. I > > wonder if it would make more sense to start with doing this purely on > > the bpftool side first. > > > > As an aside (and probably something more generally useful), it seems > > like we have a bpf_iter__bpf_map iterator, but we don't have prog and > > link iterators implemented. Would it be a good idea to add that to the > > kernel? Yonghong, Alexei, any thoughts? > > We already have program iterator. We have discussed link iterators > for sometime. As more and more usages for links, a link iterator > should be good to improve performance compared to generic 'task/file' > iterator. Agree. We have iters for progs and maps. The iter for links is missing.
> On Mon, Feb 07, 2022 at 02:11:11PM -0800, Andrii Nakryiko wrote: > On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> wrote: > > > > Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for > > BPF perf links") introduced the concept of user specified bpf_cookie, > > which could be accessed by BPF programs using bpf_get_attach_cookie(). > > For troubleshooting purposes it is convenient to expose bpf_cookie via > > bpftool as well, so there is no need to meddle with the target BPF > > program itself. > > > > $ bpftool link > > 1: type 7 prog 5 bpf_cookie 123 > > pids bootstrap(87) > > > > Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> > > --- > > Changes in v2: > > - Display bpf_cookie in bpftool link command instead perf > > > > Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com > > > So I think this change is pretty straightforward and I don't mind it, > but I'm not clear how this approach will scale to multi-attach kprobe > and fentry programs. For those, users will be specifying many bpf > cookies, one per each target attach function. At that point we'll have > a bunch of cookies sorted by the attach function IP (not necessarily > in the original order). I don't think it will be all that useful and > interesting to the end user. We won't be storing original function > names (too much memory for storing something that most probably won't > be ever queried), so restoring original order and original function > names will be hard. If we don't think this through, we'll end up with > kernel API that works for just one simple use case. > > Can you please describe your use case which motivated this feature in > the first place to better understand the importance of this? The use case is pretty theoretical at the moment, I'm trying to understand how to get more visibility about bpf_cookie usage. Let me try to do the same only in bpftool, while pondering if multi-attach programs case could be somehow meaningfully addressed as well.
> On Mon, Feb 07, 2022 at 09:46:36PM -0800, Yonghong Song wrote: > > As an aside (and probably something more generally useful), it seems > > like we have a bpf_iter__bpf_map iterator, but we don't have prog and > > link iterators implemented. Would it be a good idea to add that to the > > kernel? Yonghong, Alexei, any thoughts? > > We already have program iterator. We have discussed link iterators > for sometime. As more and more usages for links, a link iterator > should be good to improve performance compared to generic 'task/file' > iterator. Are those discussions about link iterators captured somewhere in the mailing list, could you point me to them?
On 2/8/22 12:16 PM, Dmitry Dolgov wrote: >> On Mon, Feb 07, 2022 at 09:46:36PM -0800, Yonghong Song wrote: >>> As an aside (and probably something more generally useful), it seems >>> like we have a bpf_iter__bpf_map iterator, but we don't have prog and >>> link iterators implemented. Would it be a good idea to add that to the >>> kernel? Yonghong, Alexei, any thoughts? >> >> We already have program iterator. We have discussed link iterators >> for sometime. As more and more usages for links, a link iterator >> should be good to improve performance compared to generic 'task/file' >> iterator. > > Are those discussions about link iterators captured somewhere in the > mailing list, could you point me to them? We could have mentioned link iterators in various occasions as it is natural to extend beyond progs/maps, but I don't remember a dedicated discussion around this.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index a7f0ddedac1f..600da4496404 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5850,6 +5850,9 @@ struct bpf_link_info { __u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */ __u32 target_btf_id; /* BTF type id inside the object */ } tracing; + struct { + __u64 bpf_cookie; + } perf; struct { __u64 cgroup_id; __u32 attach_type; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 72ce1edde950..94b7fa777fc7 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2948,6 +2948,7 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = { struct bpf_perf_link { struct bpf_link link; struct file *perf_file; + u64 bpf_cookie; }; static void bpf_perf_link_release(struct bpf_link *link) @@ -2966,9 +2967,20 @@ static void bpf_perf_link_dealloc(struct bpf_link *link) kfree(perf_link); } +static int bpf_perf_link_fill_link_info(const struct bpf_link *link, + struct bpf_link_info *info) +{ + struct bpf_perf_link *perf_link = + container_of(link, struct bpf_perf_link, link); + + info->perf.bpf_cookie = perf_link->bpf_cookie; + return 0; +} + static const struct bpf_link_ops bpf_perf_link_lops = { .release = bpf_perf_link_release, .dealloc = bpf_perf_link_dealloc, + .fill_link_info = bpf_perf_link_fill_link_info, }; static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog) @@ -2993,6 +3005,7 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro } bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog); link->perf_file = perf_file; + link->bpf_cookie = attr->link_create.perf_event.bpf_cookie; err = bpf_link_prime(&link->link, &link_primer); if (err) { diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c index 97dec81950e5..3ddeacb3593f 100644 --- a/tools/bpf/bpftool/link.c +++ b/tools/bpf/bpftool/link.c @@ -243,6 +243,8 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) printf("\n\tnetns_ino %u ", info->netns.netns_ino); show_link_attach_type_plain(info->netns.attach_type); break; + case BPF_LINK_TYPE_PERF_EVENT: + printf("\n\tbpf_cookie %llu ", info->perf.bpf_cookie); default: break; } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index a7f0ddedac1f..600da4496404 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5850,6 +5850,9 @@ struct bpf_link_info { __u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */ __u32 target_btf_id; /* BTF type id inside the object */ } tracing; + struct { + __u64 bpf_cookie; + } perf; struct { __u64 cgroup_id; __u32 attach_type;
Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for BPF perf links") introduced the concept of user specified bpf_cookie, which could be accessed by BPF programs using bpf_get_attach_cookie(). For troubleshooting purposes it is convenient to expose bpf_cookie via bpftool as well, so there is no need to meddle with the target BPF program itself. $ bpftool link 1: type 7 prog 5 bpf_cookie 123 pids bootstrap(87) Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> --- Changes in v2: - Display bpf_cookie in bpftool link command instead perf Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com include/uapi/linux/bpf.h | 3 +++ kernel/bpf/syscall.c | 13 +++++++++++++ tools/bpf/bpftool/link.c | 2 ++ tools/include/uapi/linux/bpf.h | 3 +++ 4 files changed, 21 insertions(+)