On Fri, Sep 8, 2023 at 4:22 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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
Right...
It is also tricky to add it to the union, right? We cannot tell whether
the kernel supports missed stats based on sizeof(struct bpf_link_info).
I guess this is also problematic?
Thanks,
Song
[...]
