* [PATCH 1/3] tracing/kprobe: Add PERF_EVENT_IOC_QUERY_KPROBE ioctl
@ 2019-08-06 23:41 Daniel Xu
2019-08-07 5:51 ` Yonghong Song
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Daniel Xu @ 2019-08-06 23:41 UTC (permalink / raw)
To: songliubraving, yhs, andriin; +Cc: Daniel Xu, bpf, kernel-team
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;
+
+ 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);
--
2.20.1
^ permalink raw reply related [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 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-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-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 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-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
* 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
end of thread, other threads:[~2019-08-07 21:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 21:15 ` Daniel Xu
2019-08-07 21:46 ` Andrii Nakryiko
2019-08-07 18:14 ` Song Liu
2019-08-07 21:08 ` Daniel Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).