perf core: Allow setting up max frame stack depth via sysctl

The default remains 127, which is good for most cases, and not even hit most of the time, but then for some cases, as reported by Brendan, 1024+ deep frames are appearing on the radar for things like groovy, ruby.

And in some workloads putting a _lower_ cap on this may make sense. One that is per event still needs to be put in place tho.

The new file is:

  # cat /proc/sys/kernel/perf_event_max_stack
  127

Chaging it:

  # echo 256 > /proc/sys/kernel/perf_event_max_stack
  # cat /proc/sys/kernel/perf_event_max_stack
  256

But as soon as there is some event using callchains we get:

  # echo 512 > /proc/sys/kernel/perf_event_max_stack
  -bash: echo: write error: Device or resource busy
  #

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-by Brendan Gregg <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

The new file is:

   # cat /proc/sys/kernel/perf_event_max_stack
   127

Chaging it:

   # echo 256 > /proc/sys/kernel/perf_event_max_stack
   # cat /proc/sys/kernel/perf_event_max_stack
   256

But as soon as there is some event using callchains we get:

   # echo 512 > /proc/sys/kernel/perf_event_max_stack
   -bash: echo: write error: Device or resource busy
   #

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-by Brendan Gregg <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

Quoted 3 lines

  # echo 256 > /proc/sys/kernel/perf_event_max_stack
  # cat /proc/sys/kernel/perf_event_max_stack
  256

I would love to see something like this go in. Right now I have to recompile the kernel because I want a lower max count.

In the past we talked about about making this part of the attribute with separate controls for both kernel stack and userspace stack. Have you given up on that option?

The default remains 127, which is good for most cases, and not even hit most of the time, but then for some cases, as reported by Brendan, 1024+ deep frames are appearing on the radar for things like groovy, ruby.

index 343c22f5e867..1390747b2195 100644

kernel/events/callchain.c +8 -0

@@ -18,6 +18,14 @@ struct callchain_cpus_entries {
 	struct perf_callchain_entry	*cpu_entries[0];
 };
 
+int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
+
+static size_t perf_callchain_entry__sizeof(void)
+{
+	return sizeof(struct perf_callchain_entry) +
+	       sizeof(__u64) * sysctl_perf_event_max_stack;
+}
+
 static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
 static atomic_t nr_callchain_events;
 static DEFINE_MUTEX(callchain_mutex);

kernel/events/callchain.c +14 -0

+	ret = proc_dointvec_minmax(&new_table, write, buffer, lenp, ppos);
+	if (ret || !write)
+		return ret;
+
+	mutex_lock(&callchain_mutex);
+	if (atomic_read(&nr_callchain_events))
+		ret = -EBUSY;
+	else
+		sysctl_perf_event_max_stack = new_value;
+	
+	mutex_unlock(&callchain_mutex);
+
+	return ret;
+}

Quoted 3 lines

The default remains 127, which is good for most cases, and not even hit most of the time, but then for some cases, as reported by Brendan, 1024+ deep frames are appearing on the radar for things like groovy, ruby.

yea gawds ;-)

Quoted 9 lines

+++ b/kernel/events/callchain.c

kernel/events/callchain.c +1 -1

@@ -73,7 +81,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 
-	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;
 

kernel/events/callchain.c static int alloc_callchain_buffers(void) · line 87

Quoted 2 lines

kernel/events/callchain.c +0 -0

 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

And this alloc _will_ fail if you put in a decent sized value..

Should we put in a dmesg WARN if this alloc fails and perf_event_max_stack is 'large' ?

kernel/events/callchain.c exit_put: · line 233

Quoted 12 lines

kernel/events/callchain.c +8 -0

@@ -215,3 +223,25 @@ exit_put:
 
 	return entry;
 }
+
+int perf_event_max_stack_handler(struct ctl_table *table, int write,
+				 void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	int new_value = sysctl_perf_event_max_stack, ret;
+	struct ctl_table new_table = *table;
+
+	new_table.data = &new_value;

cute :-)

Quoted 14 lines

kernel/events/callchain.c +14 -0

+	ret = proc_dointvec_minmax(&new_table, write, buffer, lenp, ppos);
+	if (ret || !write)
+		return ret;
+
+	mutex_lock(&callchain_mutex);
+	if (atomic_read(&nr_callchain_events))
+		ret = -EBUSY;
+	else
+		sysctl_perf_event_max_stack = new_value;
+	
+	mutex_unlock(&callchain_mutex);
+
+	return ret;
+}

Em Thu, Apr 21, 2016 at 12:48:58PM +0200, Peter Zijlstra escreveu:

Quoted 12 lines · Arnaldo Carvalho de Melo

Quoted 11 lines

+++ b/kernel/events/callchain.c

kernel/events/callchain.c +1 -1

@@ -73,7 +81,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 
-	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;
 
 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

Quoted 4 lines

And this alloc _will_ fail if you put in a decent sized value..

Should we put in a dmesg WARN if this alloc fails and perf_event_max_stack is 'large' ?

Unsure, it already returns -ENOMEM, see, some lines above, i.e. it better have error handling up this, ho-hum, call chain, I'm checking...

Nice. I like it. That's a great approach to hard problem. Java guys will be happy too.
Please also adjust two places in kernel/bpf/stackmap.c

Review

Quoted 7 lines

(quoted diff) +7 -0

+	{
+		.procname	= "perf_event_max_stack",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(sysctl_perf_event_max_stack),
+		.mode		= 0644,
+		.proc_handler	= perf_event_max_stack_handler,
+		.extra1		= &zero,

zero seems to be the wrong minimum. I think it should be at least 2 to to fit user/kernel tags ? Probably needs to define max as well.

Em Wed, Apr 20, 2016 at 04:04:12PM -0700, Alexei Starovoitov escreveu:

Quoted 6 lines · Arnaldo Carvalho de Melo

Nice. I like it. That's a great approach to hard problem. Java guys will be happy too.
Please also adjust two places in kernel/bpf/stackmap.c

Quoted 10 lines

kernel/sysctl.c static struct ctl_table kern_table[] = { · line 1153

Quoted 7 lines

kernel/sysctl.c +7 -0

+	{
+		.procname	= "perf_event_max_stack",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(sysctl_perf_event_max_stack),
+		.mode		= 0644,
+		.proc_handler	= perf_event_max_stack_handler,
+		.extra1		= &zero,

zero seems to be the wrong minimum. I think it should be at least 2 to to fit user/kernel tags ? Probably needs to define max as well.

So, if someone asks for zero, it will not copy anything, but then, this would be what the user had asked for :-)

Ditto for the max, if someone asks for too big a callchain, then when allocating it it will fail and no callchain will be produced, that or it will be able to allocate but will take too long copying that many addresses, and we would be prevented from doing so by some other protection, iirc there is perf_cpu_time_max_percent, and then buffer space will run out.

So I think that leaving it as is is enough, no?

Can I keep your Acked-by? David, can I keep yours?

Em Wed, Apr 20, 2016 at 04:04:12PM -0700, Alexei Starovoitov escreveu:

Quoted 6 lines · Arnaldo Carvalho de Melo

Nice. I like it. That's a great approach to hard problem. Java guys will be happy too.
Please also adjust two places in kernel/bpf/stackmap.c

Quoted 10 lines

kernel/sysctl.c static struct ctl_table kern_table[] = { · line 1153

Quoted 7 lines

kernel/sysctl.c +7 -0

+	{
+		.procname	= "perf_event_max_stack",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(sysctl_perf_event_max_stack),
+		.mode		= 0644,
+		.proc_handler	= perf_event_max_stack_handler,
+		.extra1		= &zero,

zero seems to be the wrong minimum. I think it should be at least 2 to to fit user/kernel tags ? Probably needs to define max as well.

So, if someone asks for zero, it will not copy anything, but then, this would be what the user had asked for :-)

Ditto for the max, if someone asks for too big a callchain, then when allocating it it will fail and no callchain will be produced, that or it will be able to allocate but will take too long copying that many addresses, and we would be prevented from doing so by some other protection, iirc there is perf_cpu_time_max_percent, and then buffer space will run out.

So I think that leaving it as is is enough, no?

Can I keep your Acked-by? David, can I keep yours?

kernel/events/callchain.c +0 -0

index 343c22f5e867..6fe77349fa9d 100644
@@ -18,6 +18,14 @@ struct callchain_cpus_entries {
  	struct perf_callchain_entry	*cpu_entries[0];

kernel/events/callchain.c +1 -1

  static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
  static atomic_t nr_callchain_events;
  static DEFINE_MUTEX(callchain_mutex);

@@ -73,7 +81,7 @@ static int alloc_callchain_buffers(void)
  	if (!entries)
  		return -ENOMEM;
 
-	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;
 
  	for_each_possible_cpu(cpu) {
  		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

Quoted 28 lines · Arnaldo Carvalho de Melo

Em Wed, Apr 20, 2016 at 04:04:12PM -0700, Alexei Starovoitov escreveu:

Quoted 6 lines · Arnaldo Carvalho de Melo

Nice. I like it. That's a great approach to hard problem. Java guys will be happy too.
Please also adjust two places in kernel/bpf/stackmap.c

Quoted 10 lines

kernel/sysctl.c static struct ctl_table kern_table[] = { · line 1153

Quoted 7 lines

kernel/sysctl.c +7 -0

+	{
+		.procname	= "perf_event_max_stack",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(sysctl_perf_event_max_stack),
+		.mode		= 0644,
+		.proc_handler	= perf_event_max_stack_handler,
+		.extra1		= &zero,

zero seems to be the wrong minimum. I think it should be at least 2 to to fit user/kernel tags ? Probably needs to define max as well.

So, if someone asks for zero, it will not copy anything, but then, this would be what the user had asked for :-)

Ditto for the max, if someone asks for too big a callchain, then when allocating it it will fail and no callchain will be produced, that or it will be able to allocate but will take too long copying that many addresses, and we would be prevented from doing so by some other protection, iirc there is perf_cpu_time_max_percent, and then buffer space will run out.

Quoted 3 lines

So I think that leaving it as is is enough, no?

Can I keep your Acked-by? David, can I keep yours?

Yes

Quoted 6 lines

kernel/events/callchain.c +0 -0

index 343c22f5e867..6fe77349fa9d 100644
@@ -18,6 +18,14 @@ struct callchain_cpus_entries {
 	struct perf_callchain_entry	*cpu_entries[0];

kernel/events/callchain.c struct callchain_cpus_entries { · line 28

Quoted 10 lines

kernel/events/callchain.c +8 -0

 };
 
+int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
+
+static size_t perf_callchain_entry__sizeof(void)
+{
+	return sizeof(struct perf_callchain_entry) +
+	       sizeof(__u64) * sysctl_perf_event_max_stack;
+}
+

To Alexei's comment, a max_stack of 0 still has a non-zero alloc size so that should be ok.

Quoted 7 lines

kernel/events/callchain.c +0 -0

 static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
 static atomic_t nr_callchain_events;
 static DEFINE_MUTEX(callchain_mutex);

@@ -73,7 +81,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 

kernel/events/callchain.c static int alloc_callchain_buffers(void) · line 87

Quoted 5 lines

kernel/events/callchain.c +1 -1

-	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;
 
 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

Quoted 22 lines · Arnaldo Carvalho de Melo

kernel/events/callchain.c struct callchain_cpus_entries { · line 28

Quoted 8 lines

kernel/events/callchain.c +8 -0

+int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
+
+static size_t perf_callchain_entry__sizeof(void)
+{
+	return sizeof(struct perf_callchain_entry) +
+	       sizeof(__u64) * sysctl_perf_event_max_stack;
+}
+

To Alexei's comment, a max_stack of 0 still has a non-zero alloc size so that should be ok.

Quoted 9 lines

static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]); static atomic_t nr_callchain_events;
static DEFINE_MUTEX(callchain_mutex);

kernel/events/callchain.c +1 -1

@@ -73,7 +81,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 
-	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;

Quoted 2 lines

kernel/events/callchain.c static int alloc_callchain_buffers(void) · line 87

Quoted 2 lines

kernel/events/callchain.c +0 -0

 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

right... and looking into it further, realized that the patch is broken, since get_callchain_entry() is doing:
return &entries->cpu_entries[cpu][*rctx];
whereas it should be dynamic offset based on sysctl_perf_event_max_stack*8 So definitely needs another respin.

Em Fri, Apr 22, 2016 at 03:18:08PM -0700, Alexei Starovoitov escreveu:

Quoted 24 lines · David Ahern

Quoted 22 lines · Arnaldo Carvalho de Melo

kernel/events/callchain.c struct callchain_cpus_entries { · line 28

Quoted 8 lines

kernel/events/callchain.c +8 -0

+int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
+
+static size_t perf_callchain_entry__sizeof(void)
+{
+	return sizeof(struct perf_callchain_entry) +
+	       sizeof(__u64) * sysctl_perf_event_max_stack;
+}
+

To Alexei's comment, a max_stack of 0 still has a non-zero alloc size so that should be ok.

Quoted 9 lines

static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]); static atomic_t nr_callchain_events;
static DEFINE_MUTEX(callchain_mutex);

kernel/events/callchain.c +1 -1

@@ -73,7 +81,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 
-	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;

Quoted 8 lines

Quoted 2 lines

kernel/events/callchain.c static int alloc_callchain_buffers(void) · line 87

Quoted 2 lines

kernel/events/callchain.c +0 -0

 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

right... and looking into it further, realized that the patch is broken, since get_callchain_entry() is doing:
return &entries->cpu_entries[cpu][*rctx];
whereas it should be dynamic offset based on sysctl_perf_event_max_stack*8 So definitely needs another respin.

Huh? Can you elaborate a bit more?

Are you saying this is a bug introduced by this patch or something pre-existing?

Em Mon, Apr 25, 2016 at 01:14:25PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Fri, Apr 22, 2016 at 03:18:08PM -0700, Alexei Starovoitov escreveu:

Quoted 5 lines

right... and looking into it further, realized that the patch is broken, since get_callchain_entry() is doing:
return &entries->cpu_entries[cpu][*rctx];
whereas it should be dynamic offset based on sysctl_perf_event_max_stack*8 So definitely needs another respin.

struct perf_callchain_entry {
        __u64     nr;
        __u64     ip[0]; /* /proc/sys/kernel/perf_event_max_stack */
};

But perf_callchain_entry->ip is not a pointer... Got it ;-\

Em Mon, Apr 25, 2016 at 01:27:06PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Em Mon, Apr 25, 2016 at 01:14:25PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Fri, Apr 22, 2016 at 03:18:08PM -0700, Alexei Starovoitov escreveu:

Quoted 5 lines

right... and looking into it further, realized that the patch is broken, since get_callchain_entry() is doing:
return &entries->cpu_entries[cpu][*rctx];
whereas it should be dynamic offset based on sysctl_perf_event_max_stack*8 So definitely needs another respin.

Quoted 6 lines

struct perf_callchain_entry {
        __u64     nr;
        __u64     ip[0]; /* /proc/sys/kernel/perf_event_max_stack */
};

But perf_callchain_entry->ip is not a pointer... Got it ;-\

This is what I am building now, a patch on top of the previous, that will be combined and sent as v3, if I don't find any more funnies:

kernel/events/callchain.c +5 -6

index 6fe77349fa9d..40657892a7e0 100644
@@ -20,11 +20,10 @@ struct callchain_cpus_entries {
 
 int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
 
-static size_t perf_callchain_entry__sizeof(void)
-{
-	return sizeof(struct perf_callchain_entry) +
-	       sizeof(__u64) * sysctl_perf_event_max_stack;
-}
+#define __perf_callchain_entry_size(entries) \
+	(sizeof(struct perf_callchain_entry) + sizeof(__u64) * entries)
+
+static size_t perf_callchain_entry_size __read_mostly = __perf_callchain_entry_size(PERF_MAX_STACK_DEPTH);
 
 static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
 static atomic_t nr_callchain_events;

@@ -81,7 +80,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 
-	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry_size * PERF_NR_CONTEXTS;
 
 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

Quoted 49 lines

Em Mon, Apr 25, 2016 at 01:27:06PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Em Mon, Apr 25, 2016 at 01:14:25PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Fri, Apr 22, 2016 at 03:18:08PM -0700, Alexei Starovoitov escreveu:

Quoted 5 lines

right... and looking into it further, realized that the patch is broken, since get_callchain_entry() is doing:
return &entries->cpu_entries[cpu][*rctx];
whereas it should be dynamic offset based on sysctl_perf_event_max_stack*8 So definitely needs another respin.

Quoted 6 lines

struct perf_callchain_entry {
        __u64     nr;
        __u64     ip[0]; /* /proc/sys/kernel/perf_event_max_stack */
};

But perf_callchain_entry->ip is not a pointer... Got it ;-\

This is what I am building now, a patch on top of the previous, that will be combined and sent as v3, if I don't find any more funnies:

kernel/events/callchain.c +5 -6

index 6fe77349fa9d..40657892a7e0 100644
@@ -20,11 +20,10 @@ struct callchain_cpus_entries {
 
 int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
 
-static size_t perf_callchain_entry__sizeof(void)
-{
-	return sizeof(struct perf_callchain_entry) +
-	       sizeof(__u64) * sysctl_perf_event_max_stack;
-}
+#define __perf_callchain_entry_size(entries) \
+	(sizeof(struct perf_callchain_entry) + sizeof(__u64) * entries)
+
+static size_t perf_callchain_entry_size __read_mostly = __perf_callchain_entry_size(PERF_MAX_STACK_DEPTH);
 
 static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
 static atomic_t nr_callchain_events;

@@ -81,7 +80,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 
-	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry_size * PERF_NR_CONTEXTS;
 
 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

kernel/events/callchain.c static struct perf_callchain_entry *get_callchain_entry(int *rctx) · line 158

Quoted 6 lines

kernel/events/callchain.c +2 -1

@@ -155,7 +154,8 @@ static struct perf_callchain_entry *get_callchain_entry(int *rctx)
 
 	cpu = smp_processor_id();
 
-	return &entries->cpu_entries[cpu][*rctx];
+	return (((void *)&entries->cpu_entries[cpu][0]) +
+		(*rctx * perf_callchain_entry_size));

I think the following would be easier to read:

?
if didn't mixed up the ordering...

and probably we could do the math on the spot instead of introducing perf_callchain_entry_size global variable?

Em Mon, Apr 25, 2016 at 01:06:48PM -0700, Alexei Starovoitov escreveu:

Quoted 59 lines · Arnaldo Carvalho de Melo

Quoted 49 lines

Em Mon, Apr 25, 2016 at 01:27:06PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Em Mon, Apr 25, 2016 at 01:14:25PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 6 lines

Em Fri, Apr 22, 2016 at 03:18:08PM -0700, Alexei Starovoitov escreveu:

Quoted 5 lines

right... and looking into it further, realized that the patch is broken, since get_callchain_entry() is doing:
return &entries->cpu_entries[cpu][*rctx];
whereas it should be dynamic offset based on sysctl_perf_event_max_stack*8 So definitely needs another respin.

Quoted 6 lines

struct perf_callchain_entry {
        __u64     nr;
        __u64     ip[0]; /* /proc/sys/kernel/perf_event_max_stack */
};

But perf_callchain_entry->ip is not a pointer... Got it ;-\

This is what I am building now, a patch on top of the previous, that will be combined and sent as v3, if I don't find any more funnies:

kernel/events/callchain.c +5 -6

index 6fe77349fa9d..40657892a7e0 100644
@@ -20,11 +20,10 @@ struct callchain_cpus_entries {
 
 int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
 
-static size_t perf_callchain_entry__sizeof(void)
-{
-	return sizeof(struct perf_callchain_entry) +
-	       sizeof(__u64) * sysctl_perf_event_max_stack;
-}
+#define __perf_callchain_entry_size(entries) \
+	(sizeof(struct perf_callchain_entry) + sizeof(__u64) * entries)
+
+static size_t perf_callchain_entry_size __read_mostly = __perf_callchain_entry_size(PERF_MAX_STACK_DEPTH);
 
 static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
 static atomic_t nr_callchain_events;

@@ -81,7 +80,7 @@ static int alloc_callchain_buffers(void)
 	if (!entries)
 		return -ENOMEM;
 
-	size = perf_callchain_entry__sizeof() * PERF_NR_CONTEXTS;
+	size = perf_callchain_entry_size * PERF_NR_CONTEXTS;
 
 	for_each_possible_cpu(cpu) {
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,

kernel/events/callchain.c static struct perf_callchain_entry *get_callchain_entry(int *rctx) · line 158

Quoted 6 lines

kernel/events/callchain.c +2 -1

@@ -155,7 +154,8 @@ static struct perf_callchain_entry *get_callchain_entry(int *rctx)
 
 	cpu = smp_processor_id();
 
-	return &entries->cpu_entries[cpu][*rctx];
+	return (((void *)&entries->cpu_entries[cpu][0]) +
+		(*rctx * perf_callchain_entry_size));

I think the following would be easier to read:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	return (void *)entries->cpu_entries[cpu] +
+		*rctx * perf_callchain_entry_size;

Well, I thought that multiline expressions required parentheses, to make them easier to read for someone, maybe Ingo? ;-)

Whatever, both generate the same result, really want me to change this?

Quoted 2 lines

?
if didn't mixed up the ordering...

If you are not sure, then its not clearer, huh? ;-P

Quoted 2 lines

and probably we could do the math on the spot instead of introducing perf_callchain_entry_size global variable?

I was trying to avoid the calc for each alloc, just doing it when we change that number via the sysctl, probably not that big a deal, do you really think we should do the math per-alloc instead of per-sysctl-changing?

Quoted 6 lines

Em Mon, Apr 25, 2016 at 01:06:48PM -0700, Alexei Starovoitov escreveu:

Quoted 1 line · Arnaldo Carvalho de Melo

Well, I thought that multiline expressions required parentheses, to make them easier to read for someone, maybe Ingo? ;-)

Whatever, both generate the same result, really want me to change this?

I was mainly complaining about unnecessary &..[0]

Quoted 4 lines

Quoted 2 lines

?
if didn't mixed up the ordering...

If you are not sure, then its not clearer, huh? ;-P

matter of taste. No strong opinion

Quoted 7 lines

Quoted 2 lines

and probably we could do the math on the spot instead of introducing perf_callchain_entry_size global variable?

I was trying to avoid the calc for each alloc, just doing it when we change that number via the sysctl, probably not that big a deal, do you really think we should do the math per-alloc instead of per-sysctl-changing?

The math is trivial:
#define __perf_callchain_entry_size(entries) \
(sizeof(struct perf_callchain_entry) + sizeof(__u64) * entries)
shift and add after load is almost the same speed as load, since integer multiply right after is dominating the cost.
whereas updating two global variables can potentially cause race conditions that need to be analyzed.

kernel/events/callchain.c +7 -0

+int sysctl_perf_event_max_stack __read_mostly = PERF_MAX_STACK_DEPTH;
+
+static inline size_t perf_callchain_entry__sizeof(void)
+{
+	return (sizeof(struct perf_callchain_entry) +
+		sizeof(__u64) * sysctl_perf_event_max_stack);
+}

@@ -1144,6 +1144,14 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,

Quoted 5 lines

(quoted diff) +5 -0

+	{
+		.procname	= "perf_event_max_stack",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(sysctl_perf_event_max_stack),
+		.mode		= 0644,

Review

Quoted 3 lines

(quoted diff) +3 -0

+		.proc_handler	= perf_event_max_stack_handler,
+		.extra1		= &zero,
+	},

you need to define a max value otherwise perf_callchain_entry__sizeof will overflow. Sure it's root only facility, but still not nice. 1M? Anything above 1M stack frames would be insane anyway. The rest looks good. Thanks!

Em Mon, Apr 25, 2016 at 05:07:26PM -0700, Alexei Starovoitov escreveu:

Quoted 5 lines

Quoted 5 lines

kernel/sysctl.c +5 -0

+	{
+		.procname	= "perf_event_max_stack",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(sysctl_perf_event_max_stack),
+		.mode		= 0644,

Quoted 8 lines

kernel/sysctl.c static struct ctl_table kern_table[] = { · line 1158

Quoted 3 lines

kernel/sysctl.c +3 -0

+		.proc_handler	= perf_event_max_stack_handler,
+		.extra1		= &zero,
+	},

you need to define a max value otherwise perf_callchain_entry__sizeof will overflow. Sure it's root only facility, but still not nice. 1M? Anything above 1M stack frames would be insane anyway. The rest looks good. Thanks!

Something else? ;-)

commit cd544af4f7fede01cb512d52bb3efe62aa19271d

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Thu Apr 21 12:28:50 2016 -0300

perf core: Allow setting up max frame stack depth via sysctl

The default remains 127, which is good for most cases, and not even hit most of the time, but then for some cases, as reported by Brendan, 1024+ deep frames are appearing on the radar for things like groovy, ruby.

And in some workloads putting a _lower_ cap on this may make sense. One that is per event still needs to be put in place tho.

The new file is:

# cat /proc/sys/kernel/perf_event_max_stack
127

Chaging it:

# echo 256 > /proc/sys/kernel/perf_event_max_stack # cat /proc/sys/kernel/perf_event_max_stack 256

But as soon as there is some event using callchains we get:

# echo 512 > /proc/sys/kernel/perf_event_max_stack -bash: echo: write error: Device or resource busy #

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

Quoted 10 lines

Em Mon, Apr 25, 2016 at 05:07:26PM -0700, Alexei Starovoitov escreveu:

Quoted 8 lines

Quoted 8 lines

kernel/sysctl.c +8 -0

+	{
+		.procname	= "perf_event_max_stack",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(sysctl_perf_event_max_stack),
+		.mode		= 0644,
+		.proc_handler	= perf_event_max_stack_handler,
+		.extra1		= &zero,
+	},

Quoted 6 lines

Quoted 4 lines

you need to define a max value otherwise perf_callchain_entry__sizeof will overflow. Sure it's root only facility, but still not nice. 1M? Anything above 1M stack frames would be insane anyway. The rest looks good. Thanks!

Something else? ;-)

all looks good to me. Thanks a bunch!

Quoted 6 lines

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

yep :)
hopefully Brendan can give it another spin.

Em Mon, Apr 25, 2016 at 05:44:00PM -0700, Alexei Starovoitov escreveu:

Quoted 11 lines · Arnaldo Carvalho de Melo

Quoted 10 lines

Em Mon, Apr 25, 2016 at 05:07:26PM -0700, Alexei Starovoitov escreveu:

Quoted 8 lines

Quoted 8 lines

kernel/sysctl.c +8 -0

+       {
+               .procname       = "perf_event_max_stack",
+               .data           = NULL, /* filled in by handler */
+               .maxlen         = sizeof(sysctl_perf_event_max_stack),
+               .mode           = 0644,
+               .proc_handler   = perf_event_max_stack_handler,
+               .extra1         = &zero,
+       },

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

you need to define a max value otherwise perf_callchain_entry__sizeof will overflow. Sure it's root only facility, but still not nice. 1M? Anything above 1M stack frames would be insane anyway. The rest looks good. Thanks!

Something else? ;-)

all looks good to me. Thanks a bunch!

Thanks for checking!

Quoted 4 lines

Quoted 4 lines

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Quoted 5 lines

Quoted 2 lines

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

yep :)
hopefully Brendan can give it another spin.

Agreed, and I'm calling it a day anyway, Brendan, please consider retesting, thanks,

- Arnaldo

Quoted 12 lines · Arnaldo Carvalho de Melo

Em Mon, Apr 25, 2016 at 05:44:00PM -0700, Alexei Starovoitov escreveu:

Quoted 10 lines · Arnaldo Carvalho de Melo

Quoted 6 lines

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

yep :)
hopefully Brendan can give it another spin.

Quoted 2 lines

Agreed, and I'm calling it a day anyway, Brendan, please consider retesting, thanks,

Will do, thanks!

Brendan

So, for completeness, further testing it to see how far it goes on a 8GB machine I got:

[root@emilia ~]# echo 131100 > /proc/sys/kernel/perf_event_max_stack [root@emilia ~]# perf record -g ls
Error:
The sys_perf_event_open() syscall returned with 12 (Cannot allocate memory) for event (cycles). /bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

Quoted 30 lines · Arnaldo Carvalho de Melo

Em Mon, Apr 25, 2016 at 05:44:00PM -0700, Alexei Starovoitov escreveu:

Quoted 11 lines · Arnaldo Carvalho de Melo

Quoted 10 lines

Em Mon, Apr 25, 2016 at 05:07:26PM -0700, Alexei Starovoitov escreveu:

Quoted 8 lines

Quoted 8 lines

kernel/sysctl.c +8 -0

+       {
+               .procname       = "perf_event_max_stack",
+               .data           = NULL, /* filled in by handler */
+               .maxlen         = sizeof(sysctl_perf_event_max_stack),
+               .mode           = 0644,
+               .proc_handler   = perf_event_max_stack_handler,
+               .extra1         = &zero,
+       },

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

you need to define a max value otherwise perf_callchain_entry__sizeof will overflow. Sure it's root only facility, but still not nice. 1M? Anything above 1M stack frames would be insane anyway. The rest looks good. Thanks!

Something else? ;-)

all looks good to me. Thanks a bunch!

Thanks for checking!

Quoted 7 lines

Quoted 6 lines

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

Quoted 6 lines

Quoted 2 lines

yep :)
hopefully Brendan can give it another spin.

Agreed, and I'm calling it a day anyway, Brendan, please consider retesting, thanks,

Will do, thanks!

Quoted 9 lines · Arnaldo Carvalho de Melo

Quoted 6 lines

Em Mon, Apr 25, 2016 at 05:44:00PM -0700, Alexei Starovoitov escreveu:

Quoted 2 lines

yep :)
hopefully Brendan can give it another spin.

Agreed, and I'm calling it a day anyway, Brendan, please consider retesting, thanks,

Will do, thanks!

Looks good.

I started with max depth = 512, and even that was still truncated, and had to profile again at 1024 to capture the full stacks. Seems to generally match the flame graph I generated with V1, which made me want to check that I'm running the new patch, and am:

# grep six_hundred_forty_kb /proc/kallsyms
ffffffff81c431e0 d six_hundred_forty_kb

I was mucking around and was able to get "corrupted callchain. skipping..." errors, but these look to be expected -- that was

profiling a binary (bash) that doesn't have frame pointers. Some perf script -D output:

16 3204735442777 0x18f0d8 [0x2030]: PERF_RECORD_SAMPLE(IP, 0x1):
18134/18134: 0xffffffff8118b6a4 period: 1001001 addr: 0 ... FP chain: nr:1023

.....  0: ffffffffffffff80
.....  1: ffffffff8118b6a4
.....  2: ffffffff8118bc47
.....  3: ffffffff811d8c85
.....  4: ffffffff811b18f8
.....  5: ffffffff811b2a55
.....  6: ffffffff811b5ea0
.....  7: ffffffff810663c0
.....  8: ffffffff810666e0
.....  9: ffffffff817b9d28

..... 10: fffffffffffffe00
..... 11: 00000000004b45e2
..... 12: 000000000000610f
..... 13: 0000000000006110
..... 14: 0000000000006111
..... 15: 0000000000006112
..... 16: 0000000000006113
..... 17: 0000000000006114
..... 18: 0000000000006115
..... 19: 0000000000006116
..... 20: 0000000000006117
[...]
..... 1021: 000000000000650b
..... 1022: 000000000000650c
... thread: bash:18134
...... dso: /lib/modules/4.6.0-rc5-virtual/build/vmlinux
bash 18134 [016] 3204.735442: 1001001 cpu-clock:

Brendan

Quoted 2 lines

I started with max depth = 512, and even that was still truncated, and had to profile again at 1024 to capture the full stacks. Seems to

Quoted 6 lines

generally match the flame graph I generated with V1, which made me want to check that I'm running the new patch, and am:

# grep six_hundred_forty_kb /proc/kallsyms
ffffffff81c431e0 d six_hundred_forty_kb

Quoted 2 lines

I was mucking around and was able to get "corrupted callchain. skipping..." errors, but these look to be expected -- that was

Yeah, thanks for testing!

And since you talked about userspace without frame pointers, have you played with '--call-graph lbr'?

commit cd544af4f7fede01cb512d52bb3efe62aa19271d

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Thu Apr 21 12:28:50 2016 -0300

perf core: Allow setting up max frame stack depth via sysctl

The default remains 127, which is good for most cases, and not even hit most of the time, but then for some cases, as reported by Brendan, 1024+ deep frames are appearing on the radar for things like groovy, ruby.

And in some workloads putting a _lower_ cap on this may make sense. One that is per event still needs to be put in place tho.

The new file is:

# cat /proc/sys/kernel/perf_event_max_stack
127

Chaging it:

# echo 256 > /proc/sys/kernel/perf_event_max_stack # cat /proc/sys/kernel/perf_event_max_stack 256

But as soon as there is some event using callchains we get:

# echo 512 > /proc/sys/kernel/perf_event_max_stack -bash: echo: write error: Device or resource busy #

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

Acked-by David Ahern <[email protected]>

Em Tue, Apr 26, 2016 at 01:02:34PM -0700, Brendan Gregg escreveu:

Quoted 21 lines · Brendan Gregg

Quoted 9 lines · Arnaldo Carvalho de Melo

Quoted 6 lines

Em Mon, Apr 25, 2016 at 05:44:00PM -0700, Alexei Starovoitov escreveu:

Quoted 2 lines

yep :)
hopefully Brendan can give it another spin.

Agreed, and I'm calling it a day anyway, Brendan, please consider retesting, thanks,

Will do, thanks!

Looks good.

I started with max depth = 512, and even that was still truncated, and had to profile again at 1024 to capture the full stacks. Seems to generally match the flame graph I generated with V1, which made me want to check that I'm running the new patch, and am:

# grep six_hundred_forty_kb /proc/kallsyms
ffffffff81c431e0 d six_hundred_forty_kb

Quoted 2 lines

I was mucking around and was able to get "corrupted callchain. skipping..." errors, but these look to be expected -- that was

Yeah, thanks for testing!

And since you talked about userspace without frame pointers, have you played with '--call-graph lbr'?

BTS seemed more promising (deeper stacks), and there's already Xen support for it (need to boot the Xen host with vpmu=bts, preferably vpmu=bts,arch for some PMCs as well :).

Quoted 2 lines

Quoted 2 lines

I started with max depth = 512, and even that was still truncated, and had to profile again at 1024 to capture the full stacks. Seems to

                           ^^^^^^

Quoted 2 lines

And since you talked about userspace without frame pointers, have you played with '--call-graph lbr'?

That seems to be at odds with his requirements; he needs 1024 entries to capture full stacks, LBR is limited to 16/32 or so entries. That's 2 orders of magnitude difference.

Quoted 34 lines

commit cd544af4f7fede01cb512d52bb3efe62aa19271d

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Thu Apr 21 12:28:50 2016 -0300

perf core: Allow setting up max frame stack depth via sysctl

The default remains 127, which is good for most cases, and not even hit most of the time, but then for some cases, as reported by Brendan, 1024+ deep frames are appearing on the radar for things like groovy, ruby.

And in some workloads putting a _lower_ cap on this may make sense. One that is per event still needs to be put in place tho.

The new file is:

# cat /proc/sys/kernel/perf_event_max_stack
127

Chaging it:

# echo 256 > /proc/sys/kernel/perf_event_max_stack # cat /proc/sys/kernel/perf_event_max_stack 256

But as soon as there is some event using callchains we get:

# echo 512 > /proc/sys/kernel/perf_event_max_stack -bash: echo: write error: Device or resource busy #

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Quoted 3 lines

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

Acked-by David Ahern <[email protected]>

Reviewed-by Frederic Weisbecker <[email protected]>

I first thought that this should be a tunable per event instead of a global sysctl

but then I realized that we still need a root-only-tunable maximum limit value to oppose against any future per event limit and this sysctl value does the job.

Nice patch!

Quoted 25 lines · Arnaldo Carvalho de Melo

Em Tue, Apr 26, 2016 at 01:02:34PM -0700, Brendan Gregg escreveu:

Quoted 23 lines · Brendan Gregg

Quoted 9 lines · Arnaldo Carvalho de Melo

Quoted 6 lines

Em Mon, Apr 25, 2016 at 05:44:00PM -0700, Alexei Starovoitov escreveu:

Quoted 2 lines

yep :)
hopefully Brendan can give it another spin.

Agreed, and I'm calling it a day anyway, Brendan, please consider retesting, thanks,

Will do, thanks!

Looks good.

I started with max depth = 512, and even that was still truncated, and had to profile again at 1024 to capture the full stacks. Seems to generally match the flame graph I generated with V1, which made me want to check that I'm running the new patch, and am:

# grep six_hundred_forty_kb /proc/kallsyms
ffffffff81c431e0 d six_hundred_forty_kb

I was mucking around and was able to get "corrupted callchain. skipping..." errors, but these look to be expected -- that was

Quoted 4 lines

Yeah, thanks for testing!

And since you talked about userspace without frame pointers, have you played with '--call-graph lbr'?

Not really. Isn't it only 16 levels deep max?

Most of our Linux is Xen guests (EC2), and I'd have to check if the MSRs are available for LBR (perf record --call-graph lbr ... returns "The sys_perf_event_open() syscall returned with 95 (Operation not

supported) for event (cpu-clock).", so I'd guess not, although many other MSRs are exposed).

BTS seemed more promising (deeper stacks), and there's already Xen support for it (need to boot the Xen host with vpmu=bts, preferably vpmu=bts,arch for some PMCs as well :).

Quoted 9 lines

So, for completeness, further testing it to see how far it goes on a 8GB machine I got:

[root@emilia ~]# echo 131100 > /proc/sys/kernel/perf_event_max_stack [root@emilia ~]# perf record -g ls
Error:
The sys_perf_event_open() syscall returned with 12 (Cannot allocate memory) for event (cycles). /bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

I thought we fixed up the last line to not display for errors like this.

How about adding ENOMEM case to perf_evsel__open_strerror?

Em Tue, Apr 26, 2016 at 11:58:10PM +0200, Frederic Weisbecker escreveu:

Quoted 39 lines · Arnaldo Carvalho de Melo

Quoted 37 lines

commit cd544af4f7fede01cb512d52bb3efe62aa19271d

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Thu Apr 21 12:28:50 2016 -0300

perf core: Allow setting up max frame stack depth via sysctl

The default remains 127, which is good for most cases, and not even hit most of the time, but then for some cases, as reported by Brendan, 1024+ deep frames are appearing on the radar for things like groovy, ruby.

And in some workloads putting a _lower_ cap on this may make sense. One that is per event still needs to be put in place tho.

The new file is:

# cat /proc/sys/kernel/perf_event_max_stack
127

Chaging it:

# echo 256 > /proc/sys/kernel/perf_event_max_stack # cat /proc/sys/kernel/perf_event_max_stack 256

But as soon as there is some event using callchains we get:

# echo 512 > /proc/sys/kernel/perf_event_max_stack -bash: echo: write error: Device or resource busy #

Because we only allocate the callchain percpu data structures when there is a user, which allows for changing the max easily, its just a matter of having no callchain users at that point.

Reported-and-Tested-by Brendan Gregg <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

Acked-by David Ahern <[email protected]>

Quoted 3 lines

Reviewed-by Frederic Weisbecker <[email protected]>

I first thought that this should be a tunable per event instead of a global sysctl

Yeah, I'll work on that too.

Quoted 3 lines

Quoted 1 line

I first thought that this should be a tunable per event instead of a global sysctl

Yeah, I'll work on that too.

There is no rush though. The sysfs limit will probably be enough for most users. Unless someone requested it?