On Fri, Sep 8, 2023 at 4:33 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> 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?
right, just checking size won't be reliable (it would be if missed is
added to largest substruct of a union). If it's important to know if
kernel reports missed, one would need to do a more proper feature
detection
>
> Thanks,
> Song
>
> [...]
