* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-06 23:41 [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl Daniel Xu
@ 2019-08-07 5:51 ` Yonghong Song
2019-08-07 18:16 ` Song Liu
2019-08-07 21:07 ` Daniel Xu
2019-08-07 6:06 ` Yonghong Song
2019-08-07 18:14 ` Song Liu
2 siblings, 2 replies; 9+ messages in thread
From: Yonghong Song @ 2019-08-07 5:51 UTC (permalink / raw)
To: Daniel Xu, Song Liu, Andrii Nakryiko; +Cc: bpf, Kernel Team
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@infradead.org> 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.
I suspect this should also work for debugfs trace event based kprobe,
but I did not verify it through codes.
> +
> + return perf_event_query_kprobe(event, (void __user *)arg);
> +#endif /* CONFIG_KPROBE_EVENTS */
> default:
> return -ENOTTY;
> }
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 9d483ad9bb6c..5449182f3056 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -196,6 +196,31 @@ bool trace_kprobe_error_injectable(struct trace_event_call *call)
> return within_error_injection_list(trace_kprobe_address(tk));
> }
>
> +int perf_event_query_kprobe(struct perf_event *event, void __user *info)
> +{
> + struct perf_event_query_kprobe __user *uquery = info;
> + struct perf_event_query_kprobe query = {};
> + struct trace_event_call *call = event->tp_event;
> + struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
> + u64 nmissed, nhit;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + if (copy_from_user(&query, uquery, sizeof(query)))
> + return -EFAULT;
> + if (query.size != sizeof(query))
> + return -EINVAL;
> +
> + nhit = trace_kprobe_nhit(tk);
> + nmissed = tk->rp.kp.nmissed;
> +
> + if (copy_to_user(&uquery->nmissed, &nmissed, sizeof(nmissed)) ||
> + copy_to_user(&uquery->nhit, &nhit, sizeof(nhit)))
> + return -EFAULT;
You can use put_user() instead of copy_to_user() to simplify the code.
> +
> + return 0;
> +}
> +
> static int register_kprobe_event(struct trace_kprobe *tk);
> static int unregister_kprobe_event(struct trace_kprobe *tk);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-07 5:51 ` Yonghong Song
@ 2019-08-07 18:16 ` Song Liu
2019-08-07 21:07 ` Daniel Xu
1 sibling, 0 replies; 9+ messages in thread
From: Song Liu @ 2019-08-07 18:16 UTC (permalink / raw)
To: Yonghong Song; +Cc: Daniel Xu, Song Liu, Andrii Nakryiko, bpf, Kernel Team
On Tue, Aug 6, 2019 at 10:52 PM Yonghong Song <yhs@fb.com> 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@infradead.org> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-07 5:51 ` Yonghong Song
2019-08-07 18:16 ` Song Liu
@ 2019-08-07 21:07 ` Daniel Xu
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Xu @ 2019-08-07 21:07 UTC (permalink / raw)
To: Yonghong Song, Song Liu, Andrii Nakryiko; +Cc: bpf, Kernel Team
On Tue, Aug 6, 2019, at 10:52 PM, Yonghong Song 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.
[...]
> > +/*
> > + * Structure used by below PERF_EVENT_IOC_QUERY_KPROE command
>
> typo PERF_EVENT_IOC_QUERY_KPROE => PERF_EVENT_IOC_QUERY_KPROBE
Ok. Whoops.
>
> > + * 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@infradead.org> as well?
Ok. Will add in V2.
>
> 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?
The idea is we can extend this API if more kprobe stats are added. Similar
to how perf_event_open(2) has a size field. Not sure if it's a silly idea but
we could also make the size field a u64 to fill the hole.
>
> This will only handle FD based kprobe. If this is the intention, best to
> clearly state it in the cover letter as well.
>
> I suspect this should also work for debugfs trace event based kprobe,
> but I did not verify it through codes.
>
Ok. Will update cover letter in v2.
[...]
> > +
> > + if (copy_to_user(&uquery->nmissed, &nmissed, sizeof(nmissed)) ||
> > + copy_to_user(&uquery->nhit, &nhit, sizeof(nhit)))
> > + return -EFAULT;
>
> You can use put_user() instead of copy_to_user() to simplify the code.
>
Ok.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-06 23:41 [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl Daniel Xu
2019-08-07 5:51 ` Yonghong Song
@ 2019-08-07 6:06 ` Yonghong Song
2019-08-07 21:15 ` Daniel Xu
2019-08-07 18:14 ` Song Liu
2 siblings, 1 reply; 9+ messages in thread
From: Yonghong Song @ 2019-08-07 6:06 UTC (permalink / raw)
To: Daniel Xu, Song Liu, Andrii Nakryiko; +Cc: bpf, Kernel Team
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
> + * 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;
> + /*
> + * 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;
> +
> + return perf_event_query_kprobe(event, (void __user *)arg);
> +#endif /* CONFIG_KPROBE_EVENTS */
> default:
> return -ENOTTY;
> }
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 9d483ad9bb6c..5449182f3056 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -196,6 +196,31 @@ bool trace_kprobe_error_injectable(struct trace_event_call *call)
> return within_error_injection_list(trace_kprobe_address(tk));
> }
>
> +int perf_event_query_kprobe(struct perf_event *event, void __user *info)
> +{
> + struct perf_event_query_kprobe __user *uquery = info;
> + struct perf_event_query_kprobe query = {};
> + struct trace_event_call *call = event->tp_event;
> + struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
> + u64 nmissed, nhit;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + if (copy_from_user(&query, uquery, sizeof(query)))
> + return -EFAULT;
> + if (query.size != sizeof(query))
> + return -EINVAL;
Note that here we did not handle any backward or forward compatibility.
> +
> + nhit = trace_kprobe_nhit(tk);
> + nmissed = tk->rp.kp.nmissed;
> +
> + if (copy_to_user(&uquery->nmissed, &nmissed, sizeof(nmissed)) ||
> + copy_to_user(&uquery->nhit, &nhit, sizeof(nhit)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> static int register_kprobe_event(struct trace_kprobe *tk);
> static int unregister_kprobe_event(struct trace_kprobe *tk);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-07 6:06 ` Yonghong Song
@ 2019-08-07 21:15 ` Daniel Xu
2019-08-07 21:46 ` Andrii Nakryiko
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Xu @ 2019-08-07 21:15 UTC (permalink / raw)
To: Yonghong Song, Song Liu, Andrii Nakryiko; +Cc: bpf, Kernel Team
On Tue, Aug 6, 2019, at 11:06 PM, Yonghong Song wrote:
[...]
> > +int perf_event_query_kprobe(struct perf_event *event, void __user *info)
> > +{
> > + struct perf_event_query_kprobe __user *uquery = info;
> > + struct perf_event_query_kprobe query = {};
> > + struct trace_event_call *call = event->tp_event;
> > + struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
> > + u64 nmissed, nhit;
> > +
> > + if (!capable(CAP_SYS_ADMIN))
> > + return -EPERM;
> > + if (copy_from_user(&query, uquery, sizeof(query)))
> > + return -EFAULT;
> > + if (query.size != sizeof(query))
> > + return -EINVAL;
>
> Note that here we did not handle any backward or forward compatibility.
>
I intended this to be reserved for future changes. Sort of like how new syscalls
will check for unknown flags. I can remove this if it's a problem.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-07 21:15 ` Daniel Xu
@ 2019-08-07 21:46 ` Andrii Nakryiko
0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-08-07 21:46 UTC (permalink / raw)
To: Daniel Xu; +Cc: Yonghong Song, Song Liu, Andrii Nakryiko, bpf, Kernel Team
On Wed, Aug 7, 2019 at 2:33 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
>
> On Tue, Aug 6, 2019, at 11:06 PM, Yonghong Song wrote:
> [...]
> > > +int perf_event_query_kprobe(struct perf_event *event, void __user *info)
> > > +{
> > > + struct perf_event_query_kprobe __user *uquery = info;
> > > + struct perf_event_query_kprobe query = {};
> > > + struct trace_event_call *call = event->tp_event;
> > > + struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
> > > + u64 nmissed, nhit;
> > > +
> > > + if (!capable(CAP_SYS_ADMIN))
> > > + return -EPERM;
> > > + if (copy_from_user(&query, uquery, sizeof(query)))
> > > + return -EFAULT;
> > > + if (query.size != sizeof(query))
> > > + return -EINVAL;
> >
> > Note that here we did not handle any backward or forward compatibility.
> >
>
> I intended this to be reserved for future changes. Sort of like how new syscalls
> will check for unknown flags. I can remove this if it's a problem.
I think what Yonghong meant was that you should probably allow
query.size > sizeof(query), but make sure that all the extra stuff is
zeroed, similar to how it's done elsewhere (e.g.,
kernel/bpf/syscall.c).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-06 23:41 [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl Daniel Xu
2019-08-07 5:51 ` Yonghong Song
2019-08-07 6:06 ` Yonghong Song
@ 2019-08-07 18:14 ` Song Liu
2019-08-07 21:08 ` Daniel Xu
2 siblings, 1 reply; 9+ messages in thread
From: Song Liu @ 2019-08-07 18:14 UTC (permalink / raw)
To: Daniel Xu; +Cc: Song Liu, Yonghong Song, Andrii Nakryiko, bpf, Kernel Team
On Tue, Aug 6, 2019 at 4:41 PM Daniel Xu <dxu@dxuuu.xyz> 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.
> ---
[...]
> +
> /*
> * 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 *)
This should be _IOR().
[...]
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 9d483ad9bb6c..5449182f3056 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -196,6 +196,31 @@ bool trace_kprobe_error_injectable(struct trace_event_call *call)
> return within_error_injection_list(trace_kprobe_address(tk));
> }
>
> +int perf_event_query_kprobe(struct perf_event *event, void __user *info)
I think perf_kprobe_event_query() would be a better name, especially that we
have struct perf_event_query_kprobe.
Thanks,
Song
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
2019-08-07 18:14 ` Song Liu
@ 2019-08-07 21:08 ` Daniel Xu
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Xu @ 2019-08-07 21:08 UTC (permalink / raw)
To: Song Liu; +Cc: Song Liu, Yonghong Song, Andrii Nakryiko, bpf, Kernel Team
On Wed, Aug 7, 2019, at 11:14 AM, Song Liu wrote:
> On Tue, Aug 6, 2019 at 4:41 PM Daniel Xu <dxu@dxuuu.xyz> 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.
> > ---
>
> [...]
> > +
> > /*
> > * 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 *)
>
> This should be _IOR().
Ok.
>
> [...]
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 9d483ad9bb6c..5449182f3056 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -196,6 +196,31 @@ bool trace_kprobe_error_injectable(struct trace_event_call *call)
> > return within_error_injection_list(trace_kprobe_address(tk));
> > }
> >
> > +int perf_event_query_kprobe(struct perf_event *event, void __user *info)
>
> I think perf_kprobe_event_query() would be a better name, especially that we
> have struct perf_event_query_kprobe.
Ok.
^ permalink raw reply [flat|nested] 9+ messages in thread