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

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Sep 08, 2023 at 04:22:05PM -0700, Andrii Nakryiko wrote:
> On Fri, Sep 8, 2023 at 4:44 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > 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
> 
> even if missed for all link types would make sense, we can't add any
> field before union, this would be a breaking change
> 
> >
> > >
> > > 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
> 
> but what's confusing to me is that missed counter is per-program (at
> least in your patch #1), but you report it on  a link. But the same
> BPF program can be attached multiple times through independent links.
> So each link will report a shared misses count, which is quite
> confusing.
> 
> Have you thought about counting misses per link instead of per
> program? Is it possible?

I think recursion_misses makes sense for both program and link

currently we have recursion_misses per program which I think is
still useful even if the program is attached to multiple links

if the program is attached to multiple links it'd be useful to
have perf link stat as well

it is definitely possible for kprobe_multi link, which IIUC might
not be the main user here, because you can already attach program
to multiple functions

I guess perf_link would benefit more from this stats, but it
looks bit harder to add.. we'd need to add link pointer to
bpf_prog_array_item and add some extra logic, will check

jirka