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

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Sep 07, 2023 at 11:40:46AM -0700, Song Liu wrote:
> On Thu, Sep 7, 2023 at 12:13 AM Jiri Olsa <jolsa@kernel.org> 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@kernel.org>
> 
> [...]
> 
> 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 */

hm, there's lot of links under bpf_link_info, can't really tell if
all could gather 'missed' data.. like I don't think we have any for
standard perf event or perf tracepoint

> 
> 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?

right, OTOH there's recursion_misses/run_time_ns/run_cnt in bpf_prog_info

the bpf link has access to its attach layer, like perf event for kprobe
in perf_link or fprobe for kprobe_multi link... so it's convenient to
reach out from link for these stats and make them available through
bpf_link_info

also there's no other way to get these data for some links

like we could perhaps add some perf event specific interface to retrieve
these stats for kprobes, because we have access to the perf event in user
space, but that's not the case for kprobe_multi link, because there's no
other way to reach the fprobe object

jirka