Re: [RFC PATCH v2] bpftool: Add bpf_cookie to link output

[Yonghong Song <yhs@xxxxxx>]

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@xxxxxxxxx>
> ---
> Changes in v2:
>      - Display bpf_cookie in bpftool link command instead perf
>
>      Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@xxxxxxxxx
>
>   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;