bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Quentin Monnet <quentin@isovalent.com>
To: Yafang Shao <laoar.shao@gmail.com>,
	ast@kernel.org, daniel@iogearbox.net, john.fastabend@gmail.com,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com, kpsingh@kernel.org, sdf@google.com,
	haoluo@google.com, jolsa@kernel.org
Cc: bpf@vger.kernel.org
Subject: Re: [RFC PATCH bpf-next 3/8] bpftool: Show probed function in kprobe_multi link info
Date: Tue, 30 May 2023 12:15:59 +0100	[thread overview]
Message-ID: <313a276f-aab9-42ed-e835-32261c25bb39@isovalent.com> (raw)
In-Reply-To: <20230528142027.5585-4-laoar.shao@gmail.com>

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

  reply	other threads:[~2023-05-30 11:16 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-28 14:20 [RFC PATCH bpf-next 0/8] bpf: Support ->show_fdinfo and ->fill_link_info for kprobe prog Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 1/8] bpf: Support ->show_fdinfo for kprobe_multi Yafang Shao
2023-05-29 12:06   ` Jiri Olsa
2023-05-30  1:39     ` Yafang Shao
2023-05-31  0:28       ` Alexei Starovoitov
2023-05-31  3:14         ` Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 2/8] bpf: Support ->fill_link_info " Yafang Shao
2023-05-29 12:49   ` Jiri Olsa
2023-05-30  1:41     ` Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 3/8] bpftool: Show probed function in kprobe_multi link info Yafang Shao
2023-05-30 11:15   ` Quentin Monnet [this message]
2023-05-31  3:16     ` Yafang Shao
2023-05-31  0:31   ` Alexei Starovoitov
2023-05-31  3:17     ` Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 4/8] bpf: Always expose the probed address Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 5/8] bpf: Support ->show_fdinfo for perf_event Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 6/8] bpf: Add a common helper bpf_copy_to_user() Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 7/8] bpf: Support ->fill_link_info for perf_event Yafang Shao
2023-05-31  0:37   ` Alexei Starovoitov
2023-05-31  3:24     ` Yafang Shao
2023-05-28 14:20 ` [RFC PATCH bpf-next 8/8] bpftool: Show probed function in perf_event link info Yafang Shao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=313a276f-aab9-42ed-e835-32261c25bb39@isovalent.com \
    --to=quentin@isovalent.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=laoar.shao@gmail.com \
    --cc=martin.lau@linux.dev \
    --cc=sdf@google.com \
    --cc=song@kernel.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).