Re: [PATCHv2 bpf-next 3/9] bpf: Add missed value to kprobe perf link info

[Song Liu <song@xxxxxxxxxx>]

On Thu, Sep 7, 2023 at 12:13 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> Add missed value to kprobe attached through perf link info to
> hold the stats of missed kprobe handler execution.
>
> The kprobe's missed counter gets incremented when kprobe handler
> is not executed due to another kprobe running on the same cpu.
>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>

[...]

The code looks good to me. But I have two thoughts on this (and 2/9).

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index e5216420ec73..e824b0c50425 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -6546,6 +6546,7 @@ struct bpf_link_info {
>                                         __u32 name_len;
>                                         __u32 offset; /* offset from func_name */
>                                         __u64 addr;
> +                                       __u64 missed;
>                                 } kprobe; /* BPF_PERF_EVENT_KPROBE, BPF_PERF_EVENT_KRETPROBE */
>                                 struct {
>                                         __aligned_u64 tp_name;   /* in/out */

1) Shall we add missed for all bpf_link_info? Something like:

diff --git i/include/uapi/linux/bpf.h w/include/uapi/linux/bpf.h
index 5a39c7a13499..cf0b8b2a8b39 100644
--- i/include/uapi/linux/bpf.h
+++ w/include/uapi/linux/bpf.h
@@ -6465,6 +6465,7 @@ struct bpf_link_info {
        __u32 type;
        __u32 id;
        __u32 prog_id;
+       __u64 missed;
        union {
                struct {
                        __aligned_u64 tp_name; /* in/out: tp_name buffer ptr */

2) "missed" doesn't seem to fit well with other information in
struct bpf_link_info. Other information there are more like stable-ish
information; while missed is a stat that changes over time. Given we
have prog_id in bpf_link_info, do we really need "missed" here?

Thanks,
Song


[...]