On Fri, Apr 22, 2016 at 05:52:32PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 20, 2016 at 04:04:12PM -0700, Alexei Starovoitov escreveu:
> > On Wed, Apr 20, 2016 at 07:47:30PM -0300, Arnaldo Carvalho de Melo wrote:
>
> > 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
>
> > > + {
> > > + .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?
I'm still a bit concerned with perf_event_max_stack==0, since it
means that alloc_callchain_buffers() will be failing,
so perf_event_open() will also be failing with ENOMEM which
will be hard to understand for any user and not clear whether
perf user space can print any hints, since such errno is ambiguous.
Also I'm concerned with very large perf_event_max_stack that
can cause integer overflow.
So I still think we have to set reasonable min and max.
Other than that it's ack.
