On Tue, Aug 6, 2019 at 10:52 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 8/6/19 4:41 PM, Daniel Xu wrote:
> > It's useful to know kprobe's nmissed and nhit stats. For example with
> > tracing tools, it's important to know when events may have been lost.
> > There is currently no way to get that information from the perf API.
> > This patch adds a new ioctl that lets users query this information.
> > ---
> > include/linux/trace_events.h | 6 ++++++
> > include/uapi/linux/perf_event.h | 23 +++++++++++++++++++++++
> > kernel/events/core.c | 11 +++++++++++
> > kernel/trace/trace_kprobe.c | 25 +++++++++++++++++++++++++
> > 4 files changed, 65 insertions(+)
> >
> > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> > index 5150436783e8..28faf115e0b8 100644
> > --- a/include/linux/trace_events.h
> > +++ b/include/linux/trace_events.h
> > @@ -586,6 +586,12 @@ extern int bpf_get_kprobe_info(const struct perf_event *event,
> > u32 *fd_type, const char **symbol,
> > u64 *probe_offset, u64 *probe_addr,
> > bool perf_type_tracepoint);
> > +extern int perf_event_query_kprobe(struct perf_event *event, void __user *info);
> > +#else
> > +int perf_event_query_kprobe(struct perf_event *event, void __user *info)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > #endif
> > #ifdef CONFIG_UPROBE_EVENTS
> > extern int perf_uprobe_init(struct perf_event *event,
> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > index 7198ddd0c6b1..4a5e18606baf 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -447,6 +447,28 @@ struct perf_event_query_bpf {
> > __u32 ids[0];
> > };
> >
> > +/*
> > + * Structure used by below PERF_EVENT_IOC_QUERY_KPROE command
>
> typo PERF_EVENT_IOC_QUERY_KPROE => PERF_EVENT_IOC_QUERY_KPROBE
>
> > + * to query information about the kprobe attached to the perf
> > + * event.
> > + */
> > +struct perf_event_query_kprobe {
> > + /*
> > + * Size of structure for forward/backward compatibility
> > + */
> > + __u32 size;
>
> Since this is perf_event UAPI change, could you cc to
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> as well?
>
> We have 32 bit hole here. For UAPI, it would be best to remove
> the hole or make it explicit. So in this case, maybe something like
> __u32 :32;
>
> Also, what is in your mind for potential future extension?
>
> > + /*
> > + * Set by the kernel to indicate number of times this kprobe
> > + * was temporarily disabled
> > + */
> > + __u64 nmissed;
> > + /*
> > + * Set by the kernel to indicate number of times this kprobe
> > + * was hit
> > + */
> > + __u64 nhit;
> > +};
> > +
> > /*
> > * Ioctls that can be done on a perf event fd:
> > */
> > @@ -462,6 +484,7 @@ struct perf_event_query_bpf {
> > #define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32)
> > #define PERF_EVENT_IOC_QUERY_BPF _IOWR('$', 10, struct perf_event_query_bpf *)
> > #define PERF_EVENT_IOC_MODIFY_ATTRIBUTES _IOW('$', 11, struct perf_event_attr *)
> > +#define PERF_EVENT_IOC_QUERY_KPROBE _IOWR('$', 12, struct perf_event_query_kprobe *)
> >
> > enum perf_event_ioc_flags {
> > PERF_IOC_FLAG_GROUP = 1U << 0,
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 026a14541a38..d61c3ac5da4f 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -5061,6 +5061,10 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
> > static int perf_copy_attr(struct perf_event_attr __user *uattr,
> > struct perf_event_attr *attr);
> >
> > +#ifdef CONFIG_KPROBE_EVENTS
> > +static struct pmu perf_kprobe;
> > +#endif /* CONFIG_KPROBE_EVENTS */
> > +
> > static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned long arg)
> > {
> > void (*func)(struct perf_event *);
> > @@ -5143,6 +5147,13 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
> >
> > return perf_event_modify_attr(event, &new_attr);
> > }
> > +#ifdef CONFIG_KPROBE_EVENTS
> > + case PERF_EVENT_IOC_QUERY_KPROBE:
> > + if (event->attr.type != perf_kprobe.type)
> > + return -EINVAL;
>
> This will only handle FD based kprobe. If this is the intention, best to
> clearly state it in the cover letter as well.
Agreed, we should highlight this is for fd-based kprobes.
>
> I suspect this should also work for debugfs trace event based kprobe,
> but I did not verify it through codes.
>
This won't work for kprobes created in debugfs. The attr.type will be different.
Thanks,
Song
