On Fri, Sep 8, 2023 at 4:44 AM Jiri Olsa <olsajiri@xxxxxxxxx> 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@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 */
>
> 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?
>
> 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
>
