[PATCH] perf/kprobe: maxactive for fd-based kprobe

[PATCH] perf/kprobe: maxactive for fd-based kprobe

[Dmitrii Dolgov]

Enable specifying maxactive for fd based kretprobe. This will be useful
for tracing tools like bcc and bpftrace (see for example discussion [1]).
Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.

The original patch [2] seems to be fallen through the cracks and wasn't
applied. I've merely rebased the work done by Song Liu and verififed it
still works.

[1]: https://github.com/iovisor/bpftrace/issues/835
[2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
 include/linux/trace_events.h    |  3 ++-
 kernel/events/core.c            | 20 ++++++++++++++++----
 kernel/trace/trace_event_perf.c |  5 +++--
 kernel/trace/trace_kprobe.c     |  4 ++--
 kernel/trace/trace_probe.h      |  2 +-
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index e6e95a9f07a5..7ca453a73252 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -850,7 +850,8 @@ extern void perf_trace_destroy(struct perf_event *event);
 extern int  perf_trace_add(struct perf_event *event, int flags);
 extern void perf_trace_del(struct perf_event *event, int flags);
 #ifdef CONFIG_KPROBE_EVENTS
-extern int  perf_kprobe_init(struct perf_event *event, bool is_retprobe);
+extern int  perf_kprobe_init(struct perf_event *event, bool is_retprobe,
+			     int max_active);
 extern void perf_kprobe_destroy(struct perf_event *event);
 extern int bpf_get_kprobe_info(const struct perf_event *event,
 			       u32 *fd_type, const char **symbol,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 23bb19716ad3..f0e936b3dfc4 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9809,24 +9809,34 @@ static struct pmu perf_tracepoint = {
  * PERF_PROBE_CONFIG_IS_RETPROBE if set, create kretprobe/uretprobe
  *                               if not set, create kprobe/uprobe
  *
- * The following values specify a reference counter (or semaphore in the
- * terminology of tools like dtrace, systemtap, etc.) Userspace Statically
- * Defined Tracepoints (USDT). Currently, we use 40 bit for the offset.
+ * PERF_UPROBE_REF_CTR_OFFSET_* specify a reference counter (or semaphore
+ * in the terminology of tools like dtrace, systemtap, etc.) Userspace
+ * Statically Defined Tracepoints (USDT). Currently, we use 40 bit for the
+ * offset.
  *
  * PERF_UPROBE_REF_CTR_OFFSET_BITS	# of bits in config as th offset
  * PERF_UPROBE_REF_CTR_OFFSET_SHIFT	# of bits to shift left
+ *
+ * PERF_KPROBE_MAX_ACTIVE_* defines max_active for kretprobe.
+ * KRETPROBE_MAXACTIVE_MAX is 4096. We allow 4095 here to save a bit.
  */
 enum perf_probe_config {
 	PERF_PROBE_CONFIG_IS_RETPROBE = 1U << 0,  /* [k,u]retprobe */
 	PERF_UPROBE_REF_CTR_OFFSET_BITS = 32,
 	PERF_UPROBE_REF_CTR_OFFSET_SHIFT = 64 - PERF_UPROBE_REF_CTR_OFFSET_BITS,
+	PERF_KPROBE_MAX_ACTIVE_BITS = 12,
+	PERF_KPROBE_MAX_ACTIVE_SHIFT = 64 - PERF_KPROBE_MAX_ACTIVE_BITS,
 };
 
 PMU_FORMAT_ATTR(retprobe, "config:0");
 #endif
 
 #ifdef CONFIG_KPROBE_EVENTS
+/* KRETPROBE_MAXACTIVE_MAX is 4096, only allow 4095 here to save a bit */
+PMU_FORMAT_ATTR(max_active, "config:52-63");
+
 static struct attribute *kprobe_attrs[] = {
+	&format_attr_max_active.attr,
 	&format_attr_retprobe.attr,
 	NULL,
 };
@@ -9857,6 +9867,7 @@ static int perf_kprobe_event_init(struct perf_event *event)
 {
 	int err;
 	bool is_retprobe;
+	int max_active;
 
 	if (event->attr.type != perf_kprobe.type)
 		return -ENOENT;
@@ -9871,7 +9882,8 @@ static int perf_kprobe_event_init(struct perf_event *event)
 		return -EOPNOTSUPP;
 
 	is_retprobe = event->attr.config & PERF_PROBE_CONFIG_IS_RETPROBE;
-	err = perf_kprobe_init(event, is_retprobe);
+	max_active = event->attr.config >> PERF_KPROBE_MAX_ACTIVE_SHIFT;
+	err = perf_kprobe_init(event, is_retprobe, max_active);
 	if (err)
 		return err;
 
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index a114549720d6..129000327809 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -245,7 +245,8 @@ void perf_trace_destroy(struct perf_event *p_event)
 }
 
 #ifdef CONFIG_KPROBE_EVENTS
-int perf_kprobe_init(struct perf_event *p_event, bool is_retprobe)
+int perf_kprobe_init(struct perf_event *p_event, bool is_retprobe,
+					 int max_active)
 {
 	int ret;
 	char *func = NULL;
@@ -271,7 +272,7 @@ int perf_kprobe_init(struct perf_event *p_event, bool is_retprobe)
 
 	tp_event = create_local_trace_kprobe(
 		func, (void *)(unsigned long)(p_event->attr.kprobe_addr),
-		p_event->attr.probe_offset, is_retprobe);
+		p_event->attr.probe_offset, is_retprobe, max_active);
 	if (IS_ERR(tp_event)) {
 		ret = PTR_ERR(tp_event);
 		goto out;
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 47cebef78532..3ad30cfce9c3 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1784,7 +1784,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
 /* create a trace_kprobe, but don't add it to global lists */
 struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
-			  bool is_return)
+			  bool is_return, int max_active)
 {
 	enum probe_print_type ptype;
 	struct trace_kprobe *tk;
@@ -1799,7 +1799,7 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 	event = func ? func : "DUMMY_EVENT";
 
 	tk = alloc_trace_kprobe(KPROBE_EVENT_SYSTEM, event, (void *)addr, func,
-				offs, 0 /* maxactive */, 0 /* nargs */,
+				offs, max_active, 0 /* nargs */,
 				is_return);
 
 	if (IS_ERR(tk)) {
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 92cc149af0fd..26fe21980793 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -376,7 +376,7 @@ extern int traceprobe_set_print_fmt(struct trace_probe *tp, enum probe_print_typ
 #ifdef CONFIG_PERF_EVENTS
 extern struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
-			  bool is_return);
+			  bool is_return, int max_active);
 extern void destroy_local_trace_kprobe(struct trace_event_call *event_call);
 
 extern struct trace_event_call *
-- 
2.32.0

Re: [PATCH] perf/kprobe: maxactive for fd-based kprobe

[Dmitry Dolgov]

> On Thu, Jun 09, 2022 at 09:29:36PM +0200, Dmitrii Dolgov wrote:
>
> Enable specifying maxactive for fd based kretprobe. This will be useful
> for tracing tools like bcc and bpftrace (see for example discussion [1]).
> Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.
>
> The original patch [2] seems to be fallen through the cracks and wasn't
> applied. I've merely rebased the work done by Song Liu and verififed it
> still works.
>
> [1]: https://github.com/iovisor/bpftrace/issues/835
> [2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/

I've recently stumbled upon this seemingly lost topic, and wanted to raise it
again. Please let me know if there is a more appropriate way to do so.

Re: [PATCH] perf/kprobe: maxactive for fd-based kprobe

[Alexei Starovoitov]

On Thu, Jun 9, 2022 at 1:14 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Thu, Jun 09, 2022 at 09:29:36PM +0200, Dmitrii Dolgov wrote:
> >
> > Enable specifying maxactive for fd based kretprobe. This will be useful
> > for tracing tools like bcc and bpftrace (see for example discussion [1]).
> > Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.
> >
> > The original patch [2] seems to be fallen through the cracks and wasn't
> > applied. I've merely rebased the work done by Song Liu and verififed it
> > still works.
> >
> > [1]: https://github.com/iovisor/bpftrace/issues/835
> > [2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/
>
> I've recently stumbled upon this seemingly lost topic, and wanted to raise it
> again. Please let me know if there is a more appropriate way to do so.

With kretprobe using rethook the maxactive limit is no longer used.
So we probably don't need this patch.

Masami, wdyt?

Re: [PATCH] perf/kprobe: maxactive for fd-based kprobe

[Masami Hiramatsu]

On Sat, 11 Jun 2022 11:28:36 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Thu, Jun 9, 2022 at 1:14 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> >
> > > On Thu, Jun 09, 2022 at 09:29:36PM +0200, Dmitrii Dolgov wrote:
> > >
> > > Enable specifying maxactive for fd based kretprobe. This will be useful
> > > for tracing tools like bcc and bpftrace (see for example discussion [1]).
> > > Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.
> > >
> > > The original patch [2] seems to be fallen through the cracks and wasn't
> > > applied. I've merely rebased the work done by Song Liu and verififed it
> > > still works.
> > >
> > > [1]: https://github.com/iovisor/bpftrace/issues/835
> > > [2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/
> >
> > I've recently stumbled upon this seemingly lost topic, and wanted to raise it
> > again. Please let me know if there is a more appropriate way to do so.
> 
> With kretprobe using rethook the maxactive limit is no longer used.
> So we probably don't need this patch.
> 
> Masami, wdyt?

No, rethook is just a library version of kretprobe return hook,
so the maxactive is still alive. I would like to make the rethook
to use(share with) function graph's per-task shadow stack. When
that is done, the maxactive will be removed.

Thank you,


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] perf/kprobe: maxactive for fd-based kprobe

[Song Liu]

> On Jun 9, 2022, at 12:29 PM, Dmitrii Dolgov <9erthalion6@gmail.com> wrote:
> 
> Enable specifying maxactive for fd based kretprobe. This will be useful
> for tracing tools like bcc and bpftrace (see for example discussion [1]).
> Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.
> 
> The original patch [2] seems to be fallen through the cracks and wasn't
> applied. I've merely rebased the work done by Song Liu and verififed it
> still works.
> 
> [1]: https://github.com/iovisor/bpftrace/issues/835
> [2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/

Thanks for pulling this out of the cracks. :)

Since there isn't much change from [2], I think this should still show 
"From:" me, and with "Signed-off-by" both of us. 

And a nit below:

Thanks,
Song

> 
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
> include/linux/trace_events.h    |  3 ++-
> kernel/events/core.c            | 20 ++++++++++++++++----
> kernel/trace/trace_event_perf.c |  5 +++--
> kernel/trace/trace_kprobe.c     |  4 ++--
> kernel/trace/trace_probe.h      |  2 +-
> 5 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index e6e95a9f07a5..7ca453a73252 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -850,7 +850,8 @@ extern void perf_trace_destroy(struct perf_event *event);
> extern int  perf_trace_add(struct perf_event *event, int flags);
> extern void perf_trace_del(struct perf_event *event, int flags);
> #ifdef CONFIG_KPROBE_EVENTS
> -extern int  perf_kprobe_init(struct perf_event *event, bool is_retprobe);
> +extern int  perf_kprobe_init(struct perf_event *event, bool is_retprobe,
> +			     int max_active);
> extern void perf_kprobe_destroy(struct perf_event *event);
> extern int bpf_get_kprobe_info(const struct perf_event *event,
> 			       u32 *fd_type, const char **symbol,
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 23bb19716ad3..f0e936b3dfc4 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9809,24 +9809,34 @@ static struct pmu perf_tracepoint = {
>  * PERF_PROBE_CONFIG_IS_RETPROBE if set, create kretprobe/uretprobe
>  *                               if not set, create kprobe/uprobe
>  *
> - * The following values specify a reference counter (or semaphore in the
> - * terminology of tools like dtrace, systemtap, etc.) Userspace Statically
> - * Defined Tracepoints (USDT). Currently, we use 40 bit for the offset.
> + * PERF_UPROBE_REF_CTR_OFFSET_* specify a reference counter (or semaphore
> + * in the terminology of tools like dtrace, systemtap, etc.) Userspace
> + * Statically Defined Tracepoints (USDT). Currently, we use 40 bit for the

40 bit is not accurate anymore, let's fix it (32). 

Otherwise, LGTM. 

[...]

Re: [PATCH] perf/kprobe: maxactive for fd-based kprobe

[Dmitry Dolgov]

> On Wed, Jun 15, 2022 at 06:20:05PM +0000, Song Liu wrote:
>
>
> > On Jun 9, 2022, at 12:29 PM, Dmitrii Dolgov <9erthalion6@gmail.com> wrote:
> >
> > Enable specifying maxactive for fd based kretprobe. This will be useful
> > for tracing tools like bcc and bpftrace (see for example discussion [1]).
> > Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.
> >
> > The original patch [2] seems to be fallen through the cracks and wasn't
> > applied. I've merely rebased the work done by Song Liu and verififed it
> > still works.
> >
> > [1]: https://github.com/iovisor/bpftrace/issues/835
> > [2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/
>
> Thanks for pulling this out of the cracks. :)
>
> Since there isn't much change from [2], I think this should still show
> "From:" me, and with "Signed-off-by" both of us.

Yep, sorry for that -- the second version should have all mentioned
correctly. Thanks!

Re: [PATCH] perf/kprobe: maxactive for fd-based kprobe

[Dmitry Dolgov]

> On Sun, Jun 12, 2022 at 09:12:32PM +0900, Masami Hiramatsu wrote:
> On Sat, 11 Jun 2022 11:28:36 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Thu, Jun 9, 2022 at 1:14 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> > >
> > > > On Thu, Jun 09, 2022 at 09:29:36PM +0200, Dmitrii Dolgov wrote:
> > > >
> > > > Enable specifying maxactive for fd based kretprobe. This will be useful
> > > > for tracing tools like bcc and bpftrace (see for example discussion [1]).
> > > > Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.
> > > >
> > > > The original patch [2] seems to be fallen through the cracks and wasn't
> > > > applied. I've merely rebased the work done by Song Liu and verififed it
> > > > still works.
> > > >
> > > > [1]: https://github.com/iovisor/bpftrace/issues/835
> > > > [2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/
> > >
> > > I've recently stumbled upon this seemingly lost topic, and wanted to raise it
> > > again. Please let me know if there is a more appropriate way to do so.
> >
> > With kretprobe using rethook the maxactive limit is no longer used.
> > So we probably don't need this patch.
> >
> > Masami, wdyt?
>
> No, rethook is just a library version of kretprobe return hook,
> so the maxactive is still alive. I would like to make the rethook
> to use(share with) function graph's per-task shadow stack. When
> that is done, the maxactive will be removed.

Thanks for clarification! Does it mean that the possibility of setting
maxactive still makes sense, until the rethook changes you've mentioned
will land?

On a side note, is it possible to somehow follow/review/test the work
about rethook and function graph's shadow stack?

Re: [PATCH] perf/kprobe: maxactive for fd-based kprobe

[Masami Hiramatsu]

On Wed, 15 Jun 2022 22:00:29 +0200
Dmitry Dolgov <9erthalion6@gmail.com> wrote:

> > On Sun, Jun 12, 2022 at 09:12:32PM +0900, Masami Hiramatsu wrote:
> > On Sat, 11 Jun 2022 11:28:36 -0700
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > > On Thu, Jun 9, 2022 at 1:14 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> > > >
> > > > > On Thu, Jun 09, 2022 at 09:29:36PM +0200, Dmitrii Dolgov wrote:
> > > > >
> > > > > Enable specifying maxactive for fd based kretprobe. This will be useful
> > > > > for tracing tools like bcc and bpftrace (see for example discussion [1]).
> > > > > Use highest 12 bit (bit 52-63) to allow maximal maxactive of 4095.
> > > > >
> > > > > The original patch [2] seems to be fallen through the cracks and wasn't
> > > > > applied. I've merely rebased the work done by Song Liu and verififed it
> > > > > still works.
> > > > >
> > > > > [1]: https://github.com/iovisor/bpftrace/issues/835
> > > > > [2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/
> > > >
> > > > I've recently stumbled upon this seemingly lost topic, and wanted to raise it
> > > > again. Please let me know if there is a more appropriate way to do so.
> > >
> > > With kretprobe using rethook the maxactive limit is no longer used.
> > > So we probably don't need this patch.
> > >
> > > Masami, wdyt?
> >
> > No, rethook is just a library version of kretprobe return hook,
> > so the maxactive is still alive. I would like to make the rethook
> > to use(share with) function graph's per-task shadow stack. When
> > that is done, the maxactive will be removed.
> 
> Thanks for clarification! Does it mean that the possibility of setting
> maxactive still makes sense, until the rethook changes you've mentioned
> will land?

Yes, unfortunatelly, there is no magic. We need a shadow stack without
runtime allocation for this purpose. Thus what we can do is to use
a per-task pre-allocated shadow stack page or make a common pool of nodes
for variable-length shadow-stack. Both has pros and cons (depends on the
system configuration).

> 
> On a side note, is it possible to somehow follow/review/test the work
> about rethook and function graph's shadow stack?

Hmm, good question. It should work theoletically, but it is easy to be
tested by ftrace tracefs interface on x86. Just enable function-graph
tracer and add a kretprobe event on some fucntion. E.g.

echo function-graph > current_tracer
echo r:test vfs_read >> kprobe_events
echo 1 > events/kprobes/enable

Thank you,


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>