* [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one
@ 2024-08-13 20:34 Andrii Nakryiko
2024-08-21 16:38 ` Andrii Nakryiko
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2024-08-13 20:34 UTC (permalink / raw)
To: linux-trace-kernel, rostedt, mhiramat
Cc: peterz, oleg, bpf, linux-kernel, jolsa, Andrii Nakryiko
trace_uprobe->nhit counter is not incremented atomically, so its value
is questionable in when uprobe is hit on multiple CPUs simultaneously.
Also, doing this shared counter increment across many CPUs causes heavy
cache line bouncing, limiting uprobe/uretprobe performance scaling with
number of CPUs.
Solve both problems by making this a per-CPU counter.
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/trace/trace_uprobe.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index c98e3b3386ba..c3df411a2684 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -17,6 +17,7 @@
#include <linux/string.h>
#include <linux/rculist.h>
#include <linux/filter.h>
+#include <linux/percpu.h>
#include "trace_dynevent.h"
#include "trace_probe.h"
@@ -62,7 +63,7 @@ struct trace_uprobe {
char *filename;
unsigned long offset;
unsigned long ref_ctr_offset;
- unsigned long nhit;
+ unsigned long __percpu *nhits;
struct trace_probe tp;
};
@@ -337,6 +338,12 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
if (!tu)
return ERR_PTR(-ENOMEM);
+ tu->nhits = alloc_percpu(unsigned long);
+ if (!tu->nhits) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
ret = trace_probe_init(&tu->tp, event, group, true, nargs);
if (ret < 0)
goto error;
@@ -349,6 +356,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
return tu;
error:
+ free_percpu(tu->nhits);
kfree(tu);
return ERR_PTR(ret);
@@ -362,6 +370,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
path_put(&tu->path);
trace_probe_cleanup(&tu->tp);
kfree(tu->filename);
+ free_percpu(tu->nhits);
kfree(tu);
}
@@ -815,13 +824,21 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
{
struct dyn_event *ev = v;
struct trace_uprobe *tu;
+ unsigned long nhits;
+ int cpu;
if (!is_trace_uprobe(ev))
return 0;
tu = to_trace_uprobe(ev);
+
+ nhits = 0;
+ for_each_possible_cpu(cpu) {
+ nhits += per_cpu(*tu->nhits, cpu);
+ }
+
seq_printf(m, " %s %-44s %15lu\n", tu->filename,
- trace_probe_name(&tu->tp), tu->nhit);
+ trace_probe_name(&tu->tp), nhits);
return 0;
}
@@ -1512,7 +1529,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
int ret = 0;
tu = container_of(con, struct trace_uprobe, consumer);
- tu->nhit++;
+
+ this_cpu_inc(*tu->nhits);
udd.tu = tu;
udd.bp_addr = instruction_pointer(regs);
--
2.43.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one
2024-08-13 20:34 [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one Andrii Nakryiko
@ 2024-08-21 16:38 ` Andrii Nakryiko
2024-08-26 13:50 ` Jiri Olsa
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2024-08-21 16:38 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: linux-trace-kernel, rostedt, mhiramat, peterz, oleg, bpf,
linux-kernel, jolsa
On Tue, Aug 13, 2024 at 1:34 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> trace_uprobe->nhit counter is not incremented atomically, so its value
> is questionable in when uprobe is hit on multiple CPUs simultaneously.
>
> Also, doing this shared counter increment across many CPUs causes heavy
> cache line bouncing, limiting uprobe/uretprobe performance scaling with
> number of CPUs.
>
> Solve both problems by making this a per-CPU counter.
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> kernel/trace/trace_uprobe.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
Is there anything else I'm expected to do about this patch? If not,
can this please be applied? Thanks!
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..c3df411a2684 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -17,6 +17,7 @@
> #include <linux/string.h>
> #include <linux/rculist.h>
> #include <linux/filter.h>
> +#include <linux/percpu.h>
>
> #include "trace_dynevent.h"
> #include "trace_probe.h"
> @@ -62,7 +63,7 @@ struct trace_uprobe {
> char *filename;
> unsigned long offset;
> unsigned long ref_ctr_offset;
> - unsigned long nhit;
> + unsigned long __percpu *nhits;
> struct trace_probe tp;
> };
>
> @@ -337,6 +338,12 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> if (!tu)
> return ERR_PTR(-ENOMEM);
>
> + tu->nhits = alloc_percpu(unsigned long);
> + if (!tu->nhits) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> ret = trace_probe_init(&tu->tp, event, group, true, nargs);
> if (ret < 0)
> goto error;
> @@ -349,6 +356,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> return tu;
>
> error:
> + free_percpu(tu->nhits);
> kfree(tu);
>
> return ERR_PTR(ret);
> @@ -362,6 +370,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
> path_put(&tu->path);
> trace_probe_cleanup(&tu->tp);
> kfree(tu->filename);
> + free_percpu(tu->nhits);
> kfree(tu);
> }
>
> @@ -815,13 +824,21 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
> {
> struct dyn_event *ev = v;
> struct trace_uprobe *tu;
> + unsigned long nhits;
> + int cpu;
>
> if (!is_trace_uprobe(ev))
> return 0;
>
> tu = to_trace_uprobe(ev);
> +
> + nhits = 0;
> + for_each_possible_cpu(cpu) {
> + nhits += per_cpu(*tu->nhits, cpu);
> + }
> +
> seq_printf(m, " %s %-44s %15lu\n", tu->filename,
> - trace_probe_name(&tu->tp), tu->nhit);
> + trace_probe_name(&tu->tp), nhits);
> return 0;
> }
>
> @@ -1512,7 +1529,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> int ret = 0;
>
> tu = container_of(con, struct trace_uprobe, consumer);
> - tu->nhit++;
> +
> + this_cpu_inc(*tu->nhits);
>
> udd.tu = tu;
> udd.bp_addr = instruction_pointer(regs);
> --
> 2.43.5
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one
2024-08-13 20:34 [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one Andrii Nakryiko
2024-08-21 16:38 ` Andrii Nakryiko
@ 2024-08-26 13:50 ` Jiri Olsa
2024-08-28 3:53 ` Masami Hiramatsu
2024-08-28 3:54 ` Masami Hiramatsu
3 siblings, 0 replies; 7+ messages in thread
From: Jiri Olsa @ 2024-08-26 13:50 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: linux-trace-kernel, rostedt, mhiramat, peterz, oleg, bpf,
linux-kernel
On Tue, Aug 13, 2024 at 01:34:09PM -0700, Andrii Nakryiko wrote:
> trace_uprobe->nhit counter is not incremented atomically, so its value
> is questionable in when uprobe is hit on multiple CPUs simultaneously.
>
> Also, doing this shared counter increment across many CPUs causes heavy
> cache line bouncing, limiting uprobe/uretprobe performance scaling with
> number of CPUs.
>
> Solve both problems by making this a per-CPU counter.
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
lgtm, fwiw
Reviewed-by: Jiri Olsa <jolsa@kernel.org>
jirka
> ---
> kernel/trace/trace_uprobe.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..c3df411a2684 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -17,6 +17,7 @@
> #include <linux/string.h>
> #include <linux/rculist.h>
> #include <linux/filter.h>
> +#include <linux/percpu.h>
>
> #include "trace_dynevent.h"
> #include "trace_probe.h"
> @@ -62,7 +63,7 @@ struct trace_uprobe {
> char *filename;
> unsigned long offset;
> unsigned long ref_ctr_offset;
> - unsigned long nhit;
> + unsigned long __percpu *nhits;
> struct trace_probe tp;
> };
>
> @@ -337,6 +338,12 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> if (!tu)
> return ERR_PTR(-ENOMEM);
>
> + tu->nhits = alloc_percpu(unsigned long);
> + if (!tu->nhits) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> ret = trace_probe_init(&tu->tp, event, group, true, nargs);
> if (ret < 0)
> goto error;
> @@ -349,6 +356,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> return tu;
>
> error:
> + free_percpu(tu->nhits);
> kfree(tu);
>
> return ERR_PTR(ret);
> @@ -362,6 +370,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
> path_put(&tu->path);
> trace_probe_cleanup(&tu->tp);
> kfree(tu->filename);
> + free_percpu(tu->nhits);
> kfree(tu);
> }
>
> @@ -815,13 +824,21 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
> {
> struct dyn_event *ev = v;
> struct trace_uprobe *tu;
> + unsigned long nhits;
> + int cpu;
>
> if (!is_trace_uprobe(ev))
> return 0;
>
> tu = to_trace_uprobe(ev);
> +
> + nhits = 0;
> + for_each_possible_cpu(cpu) {
> + nhits += per_cpu(*tu->nhits, cpu);
> + }
> +
> seq_printf(m, " %s %-44s %15lu\n", tu->filename,
> - trace_probe_name(&tu->tp), tu->nhit);
> + trace_probe_name(&tu->tp), nhits);
> return 0;
> }
>
> @@ -1512,7 +1529,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> int ret = 0;
>
> tu = container_of(con, struct trace_uprobe, consumer);
> - tu->nhit++;
> +
> + this_cpu_inc(*tu->nhits);
>
> udd.tu = tu;
> udd.bp_addr = instruction_pointer(regs);
> --
> 2.43.5
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one
2024-08-13 20:34 [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one Andrii Nakryiko
2024-08-21 16:38 ` Andrii Nakryiko
2024-08-26 13:50 ` Jiri Olsa
@ 2024-08-28 3:53 ` Masami Hiramatsu
2024-08-28 3:54 ` Masami Hiramatsu
3 siblings, 0 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2024-08-28 3:53 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: linux-trace-kernel, rostedt, peterz, oleg, bpf, linux-kernel,
jolsa
On Tue, 13 Aug 2024 13:34:09 -0700
Andrii Nakryiko <andrii@kernel.org> wrote:
> trace_uprobe->nhit counter is not incremented atomically, so its value
> is questionable in when uprobe is hit on multiple CPUs simultaneously.
>
> Also, doing this shared counter increment across many CPUs causes heavy
> cache line bouncing, limiting uprobe/uretprobe performance scaling with
> number of CPUs.
>
> Solve both problems by making this a per-CPU counter.
>
Looks good to me. Let me pick it.
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> kernel/trace/trace_uprobe.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..c3df411a2684 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -17,6 +17,7 @@
> #include <linux/string.h>
> #include <linux/rculist.h>
> #include <linux/filter.h>
> +#include <linux/percpu.h>
>
> #include "trace_dynevent.h"
> #include "trace_probe.h"
> @@ -62,7 +63,7 @@ struct trace_uprobe {
> char *filename;
> unsigned long offset;
> unsigned long ref_ctr_offset;
> - unsigned long nhit;
> + unsigned long __percpu *nhits;
> struct trace_probe tp;
> };
>
> @@ -337,6 +338,12 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> if (!tu)
> return ERR_PTR(-ENOMEM);
>
> + tu->nhits = alloc_percpu(unsigned long);
> + if (!tu->nhits) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> ret = trace_probe_init(&tu->tp, event, group, true, nargs);
> if (ret < 0)
> goto error;
> @@ -349,6 +356,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> return tu;
>
> error:
> + free_percpu(tu->nhits);
> kfree(tu);
>
> return ERR_PTR(ret);
> @@ -362,6 +370,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
> path_put(&tu->path);
> trace_probe_cleanup(&tu->tp);
> kfree(tu->filename);
> + free_percpu(tu->nhits);
> kfree(tu);
> }
>
> @@ -815,13 +824,21 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
> {
> struct dyn_event *ev = v;
> struct trace_uprobe *tu;
> + unsigned long nhits;
> + int cpu;
>
> if (!is_trace_uprobe(ev))
> return 0;
>
> tu = to_trace_uprobe(ev);
> +
> + nhits = 0;
> + for_each_possible_cpu(cpu) {
> + nhits += per_cpu(*tu->nhits, cpu);
> + }
> +
> seq_printf(m, " %s %-44s %15lu\n", tu->filename,
> - trace_probe_name(&tu->tp), tu->nhit);
> + trace_probe_name(&tu->tp), nhits);
> return 0;
> }
>
> @@ -1512,7 +1529,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> int ret = 0;
>
> tu = container_of(con, struct trace_uprobe, consumer);
> - tu->nhit++;
> +
> + this_cpu_inc(*tu->nhits);
>
> udd.tu = tu;
> udd.bp_addr = instruction_pointer(regs);
> --
> 2.43.5
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one
2024-08-13 20:34 [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one Andrii Nakryiko
` (2 preceding siblings ...)
2024-08-28 3:53 ` Masami Hiramatsu
@ 2024-08-28 3:54 ` Masami Hiramatsu
2024-08-29 17:28 ` Andrii Nakryiko
3 siblings, 1 reply; 7+ messages in thread
From: Masami Hiramatsu @ 2024-08-28 3:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: linux-trace-kernel, rostedt, peterz, oleg, bpf, linux-kernel,
jolsa
On Tue, 13 Aug 2024 13:34:09 -0700
Andrii Nakryiko <andrii@kernel.org> wrote:
> trace_uprobe->nhit counter is not incremented atomically, so its value
> is questionable in when uprobe is hit on multiple CPUs simultaneously.
>
> Also, doing this shared counter increment across many CPUs causes heavy
> cache line bouncing, limiting uprobe/uretprobe performance scaling with
> number of CPUs.
>
> Solve both problems by making this a per-CPU counter.
>
Looks good to me. Let me pick it to linux-trace probes/for-next.
Thank you,
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> kernel/trace/trace_uprobe.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..c3df411a2684 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -17,6 +17,7 @@
> #include <linux/string.h>
> #include <linux/rculist.h>
> #include <linux/filter.h>
> +#include <linux/percpu.h>
>
> #include "trace_dynevent.h"
> #include "trace_probe.h"
> @@ -62,7 +63,7 @@ struct trace_uprobe {
> char *filename;
> unsigned long offset;
> unsigned long ref_ctr_offset;
> - unsigned long nhit;
> + unsigned long __percpu *nhits;
> struct trace_probe tp;
> };
>
> @@ -337,6 +338,12 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> if (!tu)
> return ERR_PTR(-ENOMEM);
>
> + tu->nhits = alloc_percpu(unsigned long);
> + if (!tu->nhits) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> ret = trace_probe_init(&tu->tp, event, group, true, nargs);
> if (ret < 0)
> goto error;
> @@ -349,6 +356,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> return tu;
>
> error:
> + free_percpu(tu->nhits);
> kfree(tu);
>
> return ERR_PTR(ret);
> @@ -362,6 +370,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
> path_put(&tu->path);
> trace_probe_cleanup(&tu->tp);
> kfree(tu->filename);
> + free_percpu(tu->nhits);
> kfree(tu);
> }
>
> @@ -815,13 +824,21 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
> {
> struct dyn_event *ev = v;
> struct trace_uprobe *tu;
> + unsigned long nhits;
> + int cpu;
>
> if (!is_trace_uprobe(ev))
> return 0;
>
> tu = to_trace_uprobe(ev);
> +
> + nhits = 0;
> + for_each_possible_cpu(cpu) {
> + nhits += per_cpu(*tu->nhits, cpu);
> + }
> +
> seq_printf(m, " %s %-44s %15lu\n", tu->filename,
> - trace_probe_name(&tu->tp), tu->nhit);
> + trace_probe_name(&tu->tp), nhits);
> return 0;
> }
>
> @@ -1512,7 +1529,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> int ret = 0;
>
> tu = container_of(con, struct trace_uprobe, consumer);
> - tu->nhit++;
> +
> + this_cpu_inc(*tu->nhits);
>
> udd.tu = tu;
> udd.bp_addr = instruction_pointer(regs);
> --
> 2.43.5
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one
2024-08-28 3:54 ` Masami Hiramatsu
@ 2024-08-29 17:28 ` Andrii Nakryiko
2024-09-04 15:02 ` Masami Hiramatsu
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2024-08-29 17:28 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Andrii Nakryiko, linux-trace-kernel, rostedt, peterz, oleg, bpf,
linux-kernel, jolsa
On Tue, Aug 27, 2024 at 8:55 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 13 Aug 2024 13:34:09 -0700
> Andrii Nakryiko <andrii@kernel.org> wrote:
>
> > trace_uprobe->nhit counter is not incremented atomically, so its value
> > is questionable in when uprobe is hit on multiple CPUs simultaneously.
> >
> > Also, doing this shared counter increment across many CPUs causes heavy
> > cache line bouncing, limiting uprobe/uretprobe performance scaling with
> > number of CPUs.
> >
> > Solve both problems by making this a per-CPU counter.
> >
>
> Looks good to me. Let me pick it to linux-trace probes/for-next.
>
Thanks! I just checked linux-trace repo, doesn't seem like this was
applied yet, is that right? Or am I checking in the wrong place?
> Thank you,
>
>
> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> > kernel/trace/trace_uprobe.c | 24 +++++++++++++++++++++---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index c98e3b3386ba..c3df411a2684 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -17,6 +17,7 @@
> > #include <linux/string.h>
> > #include <linux/rculist.h>
> > #include <linux/filter.h>
> > +#include <linux/percpu.h>
> >
> > #include "trace_dynevent.h"
> > #include "trace_probe.h"
> > @@ -62,7 +63,7 @@ struct trace_uprobe {
> > char *filename;
> > unsigned long offset;
> > unsigned long ref_ctr_offset;
> > - unsigned long nhit;
> > + unsigned long __percpu *nhits;
> > struct trace_probe tp;
> > };
> >
> > @@ -337,6 +338,12 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> > if (!tu)
> > return ERR_PTR(-ENOMEM);
> >
> > + tu->nhits = alloc_percpu(unsigned long);
> > + if (!tu->nhits) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > ret = trace_probe_init(&tu->tp, event, group, true, nargs);
> > if (ret < 0)
> > goto error;
> > @@ -349,6 +356,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> > return tu;
> >
> > error:
> > + free_percpu(tu->nhits);
> > kfree(tu);
> >
> > return ERR_PTR(ret);
> > @@ -362,6 +370,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
> > path_put(&tu->path);
> > trace_probe_cleanup(&tu->tp);
> > kfree(tu->filename);
> > + free_percpu(tu->nhits);
> > kfree(tu);
> > }
> >
> > @@ -815,13 +824,21 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
> > {
> > struct dyn_event *ev = v;
> > struct trace_uprobe *tu;
> > + unsigned long nhits;
> > + int cpu;
> >
> > if (!is_trace_uprobe(ev))
> > return 0;
> >
> > tu = to_trace_uprobe(ev);
> > +
> > + nhits = 0;
> > + for_each_possible_cpu(cpu) {
> > + nhits += per_cpu(*tu->nhits, cpu);
> > + }
> > +
> > seq_printf(m, " %s %-44s %15lu\n", tu->filename,
> > - trace_probe_name(&tu->tp), tu->nhit);
> > + trace_probe_name(&tu->tp), nhits);
> > return 0;
> > }
> >
> > @@ -1512,7 +1529,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> > int ret = 0;
> >
> > tu = container_of(con, struct trace_uprobe, consumer);
> > - tu->nhit++;
> > +
> > + this_cpu_inc(*tu->nhits);
> >
> > udd.tu = tu;
> > udd.bp_addr = instruction_pointer(regs);
> > --
> > 2.43.5
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one
2024-08-29 17:28 ` Andrii Nakryiko
@ 2024-09-04 15:02 ` Masami Hiramatsu
0 siblings, 0 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2024-09-04 15:02 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, linux-trace-kernel, rostedt, peterz, oleg, bpf,
linux-kernel, jolsa
On Thu, 29 Aug 2024 10:28:24 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> On Tue, Aug 27, 2024 at 8:55 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Tue, 13 Aug 2024 13:34:09 -0700
> > Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > > trace_uprobe->nhit counter is not incremented atomically, so its value
> > > is questionable in when uprobe is hit on multiple CPUs simultaneously.
> > >
> > > Also, doing this shared counter increment across many CPUs causes heavy
> > > cache line bouncing, limiting uprobe/uretprobe performance scaling with
> > > number of CPUs.
> > >
> > > Solve both problems by making this a per-CPU counter.
> > >
> >
> > Looks good to me. Let me pick it to linux-trace probes/for-next.
> >
>
> Thanks! I just checked linux-trace repo, doesn't seem like this was
> applied yet, is that right? Or am I checking in the wrong place?
Sorry, I missed to push probes/for-next. Let me push it.
Thank you,
>
> > Thank you,
> >
> >
> > > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > > kernel/trace/trace_uprobe.c | 24 +++++++++++++++++++++---
> > > 1 file changed, 21 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > > index c98e3b3386ba..c3df411a2684 100644
> > > --- a/kernel/trace/trace_uprobe.c
> > > +++ b/kernel/trace/trace_uprobe.c
> > > @@ -17,6 +17,7 @@
> > > #include <linux/string.h>
> > > #include <linux/rculist.h>
> > > #include <linux/filter.h>
> > > +#include <linux/percpu.h>
> > >
> > > #include "trace_dynevent.h"
> > > #include "trace_probe.h"
> > > @@ -62,7 +63,7 @@ struct trace_uprobe {
> > > char *filename;
> > > unsigned long offset;
> > > unsigned long ref_ctr_offset;
> > > - unsigned long nhit;
> > > + unsigned long __percpu *nhits;
> > > struct trace_probe tp;
> > > };
> > >
> > > @@ -337,6 +338,12 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> > > if (!tu)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > + tu->nhits = alloc_percpu(unsigned long);
> > > + if (!tu->nhits) {
> > > + ret = -ENOMEM;
> > > + goto error;
> > > + }
> > > +
> > > ret = trace_probe_init(&tu->tp, event, group, true, nargs);
> > > if (ret < 0)
> > > goto error;
> > > @@ -349,6 +356,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> > > return tu;
> > >
> > > error:
> > > + free_percpu(tu->nhits);
> > > kfree(tu);
> > >
> > > return ERR_PTR(ret);
> > > @@ -362,6 +370,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
> > > path_put(&tu->path);
> > > trace_probe_cleanup(&tu->tp);
> > > kfree(tu->filename);
> > > + free_percpu(tu->nhits);
> > > kfree(tu);
> > > }
> > >
> > > @@ -815,13 +824,21 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
> > > {
> > > struct dyn_event *ev = v;
> > > struct trace_uprobe *tu;
> > > + unsigned long nhits;
> > > + int cpu;
> > >
> > > if (!is_trace_uprobe(ev))
> > > return 0;
> > >
> > > tu = to_trace_uprobe(ev);
> > > +
> > > + nhits = 0;
> > > + for_each_possible_cpu(cpu) {
> > > + nhits += per_cpu(*tu->nhits, cpu);
> > > + }
> > > +
> > > seq_printf(m, " %s %-44s %15lu\n", tu->filename,
> > > - trace_probe_name(&tu->tp), tu->nhit);
> > > + trace_probe_name(&tu->tp), nhits);
> > > return 0;
> > > }
> > >
> > > @@ -1512,7 +1529,8 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
> > > int ret = 0;
> > >
> > > tu = container_of(con, struct trace_uprobe, consumer);
> > > - tu->nhit++;
> > > +
> > > + this_cpu_inc(*tu->nhits);
> > >
> > > udd.tu = tu;
> > > udd.bp_addr = instruction_pointer(regs);
> > > --
> > > 2.43.5
> > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-04 15:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-13 20:34 [PATCH v3] uprobes: turn trace_uprobe's nhit counter to be per-CPU one Andrii Nakryiko
2024-08-21 16:38 ` Andrii Nakryiko
2024-08-26 13:50 ` Jiri Olsa
2024-08-28 3:53 ` Masami Hiramatsu
2024-08-28 3:54 ` Masami Hiramatsu
2024-08-29 17:28 ` Andrii Nakryiko
2024-09-04 15:02 ` Masami Hiramatsu
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).