* [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output
@ 2022-03-04 14:36 Dmitrii Dolgov
2022-03-04 16:21 ` Yonghong Song
2022-03-08 1:41 ` Andrii Nakryiko
0 siblings, 2 replies; 6+ messages in thread
From: Dmitrii Dolgov @ 2022-03-04 14:36 UTC (permalink / raw)
To: bpf, ast, daniel, andrii, quentin, yhs; +Cc: Dmitrii Dolgov
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.
Implemented using the pid iterator BPF program to actually fetch
bpf_cookies, which allows constraining code changes only to bpftool.
$ bpftool link
1: type 7 prog 5
bpf_cookie 123
pids bootstrap(81)
Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
Changes in v5:
- Remove unneeded cookie assigns
Changes in v4:
- Fetch cookies only for bpf_perf_link
- Signal about bpf_cookie via the flag, instead of deducing it from
the object and link type
- Reset pid_iter_entry to avoid invalid indirect read from stack
Changes in v3:
- Use pid iterator to fetch bpf_cookie
Changes in v2:
- Display bpf_cookie in bpftool link command instead perf
Previous discussion: https://lore.kernel.org/bpf/20220225152802.20957-1-9erthalion6@gmail.com/
tools/bpf/bpftool/main.h | 2 ++
tools/bpf/bpftool/pids.c | 8 ++++++++
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 25 +++++++++++++++++++++++
tools/bpf/bpftool/skeleton/pid_iter.h | 2 ++
4 files changed, 37 insertions(+)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 0c3840596b5a..1bb76aa1f3b2 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -114,6 +114,8 @@ struct obj_ref {
struct obj_refs {
int ref_cnt;
struct obj_ref *refs;
+ bool bpf_cookie_set;
+ __u64 bpf_cookie;
};
struct btf;
diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
index 7c384d10e95f..6c6e7c90cc3d 100644
--- a/tools/bpf/bpftool/pids.c
+++ b/tools/bpf/bpftool/pids.c
@@ -78,6 +78,8 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
ref->pid = e->pid;
memcpy(ref->comm, e->comm, sizeof(ref->comm));
refs->ref_cnt = 1;
+ refs->bpf_cookie_set = e->bpf_cookie_set;
+ refs->bpf_cookie = e->bpf_cookie;
err = hashmap__append(map, u32_as_hash_field(e->id), refs);
if (err)
@@ -205,6 +207,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
if (refs->ref_cnt == 0)
break;
+ if (refs->bpf_cookie_set)
+ jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);
+
jsonw_name(json_writer, "pids");
jsonw_start_array(json_writer);
for (i = 0; i < refs->ref_cnt; i++) {
@@ -234,6 +239,9 @@ void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix)
if (refs->ref_cnt == 0)
break;
+ if (refs->bpf_cookie_set)
+ printf("\n\tbpf_cookie %llu", refs->bpf_cookie);
+
printf("%s", prefix);
for (i = 0; i < refs->ref_cnt; i++) {
struct obj_ref *ref = &refs->refs[i];
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
index f70702fcb224..91366ce33717 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
+++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
@@ -38,6 +38,18 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
}
}
+/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
+static __always_inline __u64 get_bpf_cookie(struct bpf_link *link)
+{
+ struct bpf_perf_link *perf_link;
+ struct perf_event *event;
+
+ perf_link = container_of(link, struct bpf_perf_link, link);
+ event = BPF_CORE_READ(perf_link, perf_file, private_data);
+ return BPF_CORE_READ(event, bpf_cookie);
+}
+
+
SEC("iter/task_file")
int iter(struct bpf_iter__task_file *ctx)
{
@@ -69,8 +81,21 @@ int iter(struct bpf_iter__task_file *ctx)
if (file->f_op != fops)
return 0;
+ __builtin_memset(&e, 0, sizeof(e));
e.pid = task->tgid;
e.id = get_obj_id(file->private_data, obj_type);
+ e.bpf_cookie = 0;
+ e.bpf_cookie_set = false;
+
+ if (obj_type == BPF_OBJ_LINK) {
+ struct bpf_link *link = (struct bpf_link *) file->private_data;
+
+ if (BPF_CORE_READ(link, type) == BPF_LINK_TYPE_PERF_EVENT) {
+ e.bpf_cookie_set = true;
+ e.bpf_cookie = get_bpf_cookie(link);
+ }
+ }
+
bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
task->group_leader->comm);
bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
diff --git a/tools/bpf/bpftool/skeleton/pid_iter.h b/tools/bpf/bpftool/skeleton/pid_iter.h
index 5692cf257adb..2676cece58d7 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.h
+++ b/tools/bpf/bpftool/skeleton/pid_iter.h
@@ -6,6 +6,8 @@
struct pid_iter_entry {
__u32 id;
int pid;
+ __u64 bpf_cookie;
+ bool bpf_cookie_set;
char comm[16];
};
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output
2022-03-04 14:36 [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output Dmitrii Dolgov
@ 2022-03-04 16:21 ` Yonghong Song
2022-03-05 12:44 ` Dmitry Dolgov
2022-03-08 1:41 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2022-03-04 16:21 UTC (permalink / raw)
To: Dmitrii Dolgov, bpf, ast, daniel, andrii, quentin
On 3/4/22 6:36 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.
>
> Implemented using the pid iterator BPF program to actually fetch
> bpf_cookies, which allows constraining code changes only to bpftool.
>
> $ bpftool link
> 1: type 7 prog 5
> bpf_cookie 123
> pids bootstrap(81)
>
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
LGTM with a few nits below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> Changes in v5:
> - Remove unneeded cookie assigns
>
> Changes in v4:
> - Fetch cookies only for bpf_perf_link
> - Signal about bpf_cookie via the flag, instead of deducing it from
> the object and link type
> - Reset pid_iter_entry to avoid invalid indirect read from stack
>
> Changes in v3:
> - Use pid iterator to fetch bpf_cookie
>
> Changes in v2:
> - Display bpf_cookie in bpftool link command instead perf
>
> Previous discussion: https://lore.kernel.org/bpf/20220225152802.20957-1-9erthalion6@gmail.com/
>
>
> tools/bpf/bpftool/main.h | 2 ++
> tools/bpf/bpftool/pids.c | 8 ++++++++
> tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 25 +++++++++++++++++++++++
> tools/bpf/bpftool/skeleton/pid_iter.h | 2 ++
> 4 files changed, 37 insertions(+)
>
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0c3840596b5a..1bb76aa1f3b2 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -114,6 +114,8 @@ struct obj_ref {
> struct obj_refs {
> int ref_cnt;
> struct obj_ref *refs;
> + bool bpf_cookie_set;
> + __u64 bpf_cookie;
> };
>
> struct btf;
> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> index 7c384d10e95f..6c6e7c90cc3d 100644
> --- a/tools/bpf/bpftool/pids.c
> +++ b/tools/bpf/bpftool/pids.c
> @@ -78,6 +78,8 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
> ref->pid = e->pid;
> memcpy(ref->comm, e->comm, sizeof(ref->comm));
> refs->ref_cnt = 1;
> + refs->bpf_cookie_set = e->bpf_cookie_set;
> + refs->bpf_cookie = e->bpf_cookie;
>
> err = hashmap__append(map, u32_as_hash_field(e->id), refs);
> if (err)
> @@ -205,6 +207,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
> if (refs->ref_cnt == 0)
> break;
>
> + if (refs->bpf_cookie_set)
> + jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);
The original motivation for 'bpf_cookie' is for kprobe to get function
addresses. In that case, printing with llx (0x...) is better than llu
since people can easily search it with /proc/kallsyms to get what the
function it attached to. But on the other hand, other use cases might
be simply just wanting an int.
I don't have a strong opinion here. Just to speak out loud so other
people can comment on this too.
> +
> jsonw_name(json_writer, "pids");
> jsonw_start_array(json_writer);
> for (i = 0; i < refs->ref_cnt; i++) {
> @@ -234,6 +239,9 @@ void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix)
> if (refs->ref_cnt == 0)
> break;
>
> + if (refs->bpf_cookie_set)
> + printf("\n\tbpf_cookie %llu", refs->bpf_cookie);
> +
> printf("%s", prefix);
> for (i = 0; i < refs->ref_cnt; i++) {
> struct obj_ref *ref = &refs->refs[i];
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> index f70702fcb224..91366ce33717 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> @@ -38,6 +38,18 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
> }
> }
>
> +/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
> +static __always_inline __u64 get_bpf_cookie(struct bpf_link *link)
> +{
> + struct bpf_perf_link *perf_link;
> + struct perf_event *event;
> +
> + perf_link = container_of(link, struct bpf_perf_link, link);
> + event = BPF_CORE_READ(perf_link, perf_file, private_data);
> + return BPF_CORE_READ(event, bpf_cookie);
> +}
> +
> +
> SEC("iter/task_file")
> int iter(struct bpf_iter__task_file *ctx)
> {
> @@ -69,8 +81,21 @@ int iter(struct bpf_iter__task_file *ctx)
> if (file->f_op != fops)
> return 0;
>
> + __builtin_memset(&e, 0, sizeof(e));
> e.pid = task->tgid;
> e.id = get_obj_id(file->private_data, obj_type);
> + e.bpf_cookie = 0;
> + e.bpf_cookie_set = false;
We already have __builtin_memset(&e, 0, sizeof(e)) in the above, so
the above e.bpf_cookie and e.bpf_cookie_set assignment is not
necessary.
> +
> + if (obj_type == BPF_OBJ_LINK) {
> + struct bpf_link *link = (struct bpf_link *) file->private_data;
> +
> + if (BPF_CORE_READ(link, type) == BPF_LINK_TYPE_PERF_EVENT) {
> + e.bpf_cookie_set = true;
> + e.bpf_cookie = get_bpf_cookie(link);
> + }
> + }
> +
> bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
> task->group_leader->comm);
> bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.h b/tools/bpf/bpftool/skeleton/pid_iter.h
> index 5692cf257adb..2676cece58d7 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.h
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.h
> @@ -6,6 +6,8 @@
> struct pid_iter_entry {
> __u32 id;
> int pid;
> + __u64 bpf_cookie;
> + bool bpf_cookie_set;
> char comm[16];
> };
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output
2022-03-04 16:21 ` Yonghong Song
@ 2022-03-05 12:44 ` Dmitry Dolgov
2022-03-08 1:42 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Dolgov @ 2022-03-05 12:44 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, ast, daniel, andrii, quentin
On Fri, Mar 04, 2022 at 08:21:34AM -0800, Yonghong Song wrote:
>
> > diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> > index 7c384d10e95f..6c6e7c90cc3d 100644
> > --- a/tools/bpf/bpftool/pids.c
> > +++ b/tools/bpf/bpftool/pids.c
> > @@ -78,6 +78,8 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
> > ref->pid = e->pid;
> > memcpy(ref->comm, e->comm, sizeof(ref->comm));
> > refs->ref_cnt = 1;
> > + refs->bpf_cookie_set = e->bpf_cookie_set;
> > + refs->bpf_cookie = e->bpf_cookie;
> > err = hashmap__append(map, u32_as_hash_field(e->id), refs);
> > if (err)
> > @@ -205,6 +207,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
> > if (refs->ref_cnt == 0)
> > break;
> > + if (refs->bpf_cookie_set)
> > + jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);
>
> The original motivation for 'bpf_cookie' is for kprobe to get function
> addresses. In that case, printing with llx (0x...) is better than llu
> since people can easily search it with /proc/kallsyms to get what the
> function it attached to. But on the other hand, other use cases might
> be simply just wanting an int.
>
> I don't have a strong opinion here. Just to speak out loud so other
> people can comment on this too.
Interesting, I didn't know that. The current implementation of
'bpf_cookie' seems to be quite opaque, with no assumptions about what
does it contain, probably it makes sense to keep it like that. But I
don't have a strong opinion here either, would love to hear what others
think.
> > diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > index f70702fcb224..91366ce33717 100644
> > --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > @@ -38,6 +38,18 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
> > }
> > }
> > +/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
> > +static __always_inline __u64 get_bpf_cookie(struct bpf_link *link)
> > +{
> > + struct bpf_perf_link *perf_link;
> > + struct perf_event *event;
> > +
> > + perf_link = container_of(link, struct bpf_perf_link, link);
> > + event = BPF_CORE_READ(perf_link, perf_file, private_data);
> > + return BPF_CORE_READ(event, bpf_cookie);
> > +}
> > +
> > +
> > SEC("iter/task_file")
> > int iter(struct bpf_iter__task_file *ctx)
> > {
> > @@ -69,8 +81,21 @@ int iter(struct bpf_iter__task_file *ctx)
> > if (file->f_op != fops)
> > return 0;
> > + __builtin_memset(&e, 0, sizeof(e));
> > e.pid = task->tgid;
> > e.id = get_obj_id(file->private_data, obj_type);
> > + e.bpf_cookie = 0;
> > + e.bpf_cookie_set = false;
>
> We already have __builtin_memset(&e, 0, sizeof(e)) in the above, so
> the above e.bpf_cookie and e.bpf_cookie_set assignment is not
> necessary.
Good point, will remote this.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output
2022-03-04 14:36 [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output Dmitrii Dolgov
2022-03-04 16:21 ` Yonghong Song
@ 2022-03-08 1:41 ` Andrii Nakryiko
2022-03-09 16:11 ` Dmitry Dolgov
1 sibling, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2022-03-08 1:41 UTC (permalink / raw)
To: Dmitrii Dolgov
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Quentin Monnet, Yonghong Song
On Fri, Mar 4, 2022 at 6:36 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.
>
> Implemented using the pid iterator BPF program to actually fetch
> bpf_cookies, which allows constraining code changes only to bpftool.
>
> $ bpftool link
> 1: type 7 prog 5
> bpf_cookie 123
> pids bootstrap(81)
>
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
> Changes in v5:
> - Remove unneeded cookie assigns
>
> Changes in v4:
> - Fetch cookies only for bpf_perf_link
> - Signal about bpf_cookie via the flag, instead of deducing it from
> the object and link type
> - Reset pid_iter_entry to avoid invalid indirect read from stack
>
> Changes in v3:
> - Use pid iterator to fetch bpf_cookie
>
> Changes in v2:
> - Display bpf_cookie in bpftool link command instead perf
>
> Previous discussion: https://lore.kernel.org/bpf/20220225152802.20957-1-9erthalion6@gmail.com/
>
>
> tools/bpf/bpftool/main.h | 2 ++
> tools/bpf/bpftool/pids.c | 8 ++++++++
> tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 25 +++++++++++++++++++++++
> tools/bpf/bpftool/skeleton/pid_iter.h | 2 ++
> 4 files changed, 37 insertions(+)
>
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0c3840596b5a..1bb76aa1f3b2 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -114,6 +114,8 @@ struct obj_ref {
> struct obj_refs {
> int ref_cnt;
> struct obj_ref *refs;
> + bool bpf_cookie_set;
> + __u64 bpf_cookie;
> };
>
> struct btf;
> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> index 7c384d10e95f..6c6e7c90cc3d 100644
> --- a/tools/bpf/bpftool/pids.c
> +++ b/tools/bpf/bpftool/pids.c
> @@ -78,6 +78,8 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
> ref->pid = e->pid;
> memcpy(ref->comm, e->comm, sizeof(ref->comm));
> refs->ref_cnt = 1;
> + refs->bpf_cookie_set = e->bpf_cookie_set;
> + refs->bpf_cookie = e->bpf_cookie;
>
> err = hashmap__append(map, u32_as_hash_field(e->id), refs);
> if (err)
> @@ -205,6 +207,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
> if (refs->ref_cnt == 0)
> break;
>
> + if (refs->bpf_cookie_set)
> + jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);
> +
> jsonw_name(json_writer, "pids");
> jsonw_start_array(json_writer);
> for (i = 0; i < refs->ref_cnt; i++) {
> @@ -234,6 +239,9 @@ void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix)
> if (refs->ref_cnt == 0)
> break;
>
> + if (refs->bpf_cookie_set)
> + printf("\n\tbpf_cookie %llu", refs->bpf_cookie);
__u64 is not always %llu on all architectures. Best to cast it to
(unsigned long long) here to avoid compilation warnings.
> +
> printf("%s", prefix);
> for (i = 0; i < refs->ref_cnt; i++) {
> struct obj_ref *ref = &refs->refs[i];
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> index f70702fcb224..91366ce33717 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> @@ -38,6 +38,18 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
> }
> }
>
> +/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
> +static __always_inline __u64 get_bpf_cookie(struct bpf_link *link)
no need for __always_inline
> +{
> + struct bpf_perf_link *perf_link;
> + struct perf_event *event;
> +
> + perf_link = container_of(link, struct bpf_perf_link, link);
> + event = BPF_CORE_READ(perf_link, perf_file, private_data);
> + return BPF_CORE_READ(event, bpf_cookie);
> +}
> +
> +
nit: why double empty line?
> SEC("iter/task_file")
> int iter(struct bpf_iter__task_file *ctx)
> {
> @@ -69,8 +81,21 @@ int iter(struct bpf_iter__task_file *ctx)
> if (file->f_op != fops)
> return 0;
>
> + __builtin_memset(&e, 0, sizeof(e));
> e.pid = task->tgid;
> e.id = get_obj_id(file->private_data, obj_type);
> + e.bpf_cookie = 0;
> + e.bpf_cookie_set = false;
> +
> + if (obj_type == BPF_OBJ_LINK) {
> + struct bpf_link *link = (struct bpf_link *) file->private_data;
> +
> + if (BPF_CORE_READ(link, type) == BPF_LINK_TYPE_PERF_EVENT) {
> + e.bpf_cookie_set = true;
> + e.bpf_cookie = get_bpf_cookie(link);
> + }
> + }
> +
> bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
> task->group_leader->comm);
> bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.h b/tools/bpf/bpftool/skeleton/pid_iter.h
> index 5692cf257adb..2676cece58d7 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.h
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.h
> @@ -6,6 +6,8 @@
> struct pid_iter_entry {
> __u32 id;
> int pid;
> + __u64 bpf_cookie;
> + bool bpf_cookie_set;
naming nit: either "bpf_cookie_is_set" or "has_bpf_cookie"?
"bpf_cookie_set" is read either as a verb or as "a set of bpf_cookies"
> char comm[16];
> };
>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output
2022-03-05 12:44 ` Dmitry Dolgov
@ 2022-03-08 1:42 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2022-03-08 1:42 UTC (permalink / raw)
To: Dmitry Dolgov
Cc: Yonghong Song, bpf, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Quentin Monnet
On Sat, Mar 5, 2022 at 4:44 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> On Fri, Mar 04, 2022 at 08:21:34AM -0800, Yonghong Song wrote:
> >
> > > diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> > > index 7c384d10e95f..6c6e7c90cc3d 100644
> > > --- a/tools/bpf/bpftool/pids.c
> > > +++ b/tools/bpf/bpftool/pids.c
> > > @@ -78,6 +78,8 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
> > > ref->pid = e->pid;
> > > memcpy(ref->comm, e->comm, sizeof(ref->comm));
> > > refs->ref_cnt = 1;
> > > + refs->bpf_cookie_set = e->bpf_cookie_set;
> > > + refs->bpf_cookie = e->bpf_cookie;
> > > err = hashmap__append(map, u32_as_hash_field(e->id), refs);
> > > if (err)
> > > @@ -205,6 +207,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
> > > if (refs->ref_cnt == 0)
> > > break;
> > > + if (refs->bpf_cookie_set)
> > > + jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);
> >
> > The original motivation for 'bpf_cookie' is for kprobe to get function
> > addresses. In that case, printing with llx (0x...) is better than llu
> > since people can easily search it with /proc/kallsyms to get what the
> > function it attached to. But on the other hand, other use cases might
> > be simply just wanting an int.
> >
> > I don't have a strong opinion here. Just to speak out loud so other
> > people can comment on this too.
>
> Interesting, I didn't know that. The current implementation of
> 'bpf_cookie' seems to be quite opaque, with no assumptions about what
> does it contain, probably it makes sense to keep it like that. But I
> don't have a strong opinion here either, would love to hear what others
> think.
There is no assumption that it's going to be an address. I actually
expect that usually it will be a small index into an additional array
of configuration values. So keeping it decimal makes more sense to me.
>
> > > diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > > index f70702fcb224..91366ce33717 100644
> > > --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > > +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > > @@ -38,6 +38,18 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
> > > }
> > > }
> > > +/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
> > > +static __always_inline __u64 get_bpf_cookie(struct bpf_link *link)
> > > +{
> > > + struct bpf_perf_link *perf_link;
> > > + struct perf_event *event;
> > > +
> > > + perf_link = container_of(link, struct bpf_perf_link, link);
> > > + event = BPF_CORE_READ(perf_link, perf_file, private_data);
> > > + return BPF_CORE_READ(event, bpf_cookie);
> > > +}
> > > +
> > > +
> > > SEC("iter/task_file")
> > > int iter(struct bpf_iter__task_file *ctx)
> > > {
> > > @@ -69,8 +81,21 @@ int iter(struct bpf_iter__task_file *ctx)
> > > if (file->f_op != fops)
> > > return 0;
> > > + __builtin_memset(&e, 0, sizeof(e));
> > > e.pid = task->tgid;
> > > e.id = get_obj_id(file->private_data, obj_type);
> > > + e.bpf_cookie = 0;
> > > + e.bpf_cookie_set = false;
> >
> > We already have __builtin_memset(&e, 0, sizeof(e)) in the above, so
> > the above e.bpf_cookie and e.bpf_cookie_set assignment is not
> > necessary.
>
> Good point, will remote this.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output
2022-03-08 1:41 ` Andrii Nakryiko
@ 2022-03-09 16:11 ` Dmitry Dolgov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Dolgov @ 2022-03-09 16:11 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Quentin Monnet, Yonghong Song
> On Mon, Mar 07, 2022 at 05:41:24PM -0800, Andrii Nakryiko wrote:
> On Fri, Mar 4, 2022 at 6:36 AM Dmitrii Dolgov <9erthalion6@gmail.com> wrote:
> >
> > @@ -234,6 +239,9 @@ void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix)
> > if (refs->ref_cnt == 0)
> > break;
> >
> > + if (refs->bpf_cookie_set)
> > + printf("\n\tbpf_cookie %llu", refs->bpf_cookie);
>
> __u64 is not always %llu on all architectures. Best to cast it to
> (unsigned long long) here to avoid compilation warnings.
>
> > @@ -38,6 +38,18 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
> > }
> > }
> >
> > +/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
> > +static __always_inline __u64 get_bpf_cookie(struct bpf_link *link)
>
> no need for __always_inline
>
> > +{
> > + struct bpf_perf_link *perf_link;
> > + struct perf_event *event;
> > +
> > + perf_link = container_of(link, struct bpf_perf_link, link);
> > + event = BPF_CORE_READ(perf_link, perf_file, private_data);
> > + return BPF_CORE_READ(event, bpf_cookie);
> > +}
> > +
> > +
>
> nit: why double empty line?
>
> > diff --git a/tools/bpf/bpftool/skeleton/pid_iter.h b/tools/bpf/bpftool/skeleton/pid_iter.h
> > index 5692cf257adb..2676cece58d7 100644
> > --- a/tools/bpf/bpftool/skeleton/pid_iter.h
> > +++ b/tools/bpf/bpftool/skeleton/pid_iter.h
> > @@ -6,6 +6,8 @@
> > struct pid_iter_entry {
> > __u32 id;
> > int pid;
> > + __u64 bpf_cookie;
> > + bool bpf_cookie_set;
>
> naming nit: either "bpf_cookie_is_set" or "has_bpf_cookie"?
> "bpf_cookie_set" is read either as a verb or as "a set of bpf_cookies"
Yep, makes sense (this and the rest of the comments above), will post a
new version with the cumulative changes soon.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-09 16:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-04 14:36 [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output Dmitrii Dolgov
2022-03-04 16:21 ` Yonghong Song
2022-03-05 12:44 ` Dmitry Dolgov
2022-03-08 1:42 ` Andrii Nakryiko
2022-03-08 1:41 ` Andrii Nakryiko
2022-03-09 16:11 ` Dmitry Dolgov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox