Date: Fri, 22 Apr 2016 14:37:35 -0700
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
        Frederic Weisbecker <fweisbec@gmail.com>,
        Ingo Molnar <mingo@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Brendan Gregg <brendan.d.gregg@gmail.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Alexei Starovoitov <ast@kernel.org>, David Ahern <dsahern@gmail.com>,
        He Kuang <hekuang@huawei.com>, Jiri Olsa <jolsa@redhat.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Milian Wolff <milian.wolff@kdab.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Stephane Eranian <eranian@google.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vince Weaver <vincent.weaver@maine.edu>,
        Wang Nan <wangnan0@huawei.com>, Zefan Li <lizefan@huawei.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH/RFC v2] perf core: Allow setting up max frame stack depth
 via sysctl
Message-ID: <20160422213733.GA61193@ast-mbp.thefacebook.com>
References: <20160420224730.GX3677@kernel.org>
 <20160420230410.GA41736@ast-mbp.thefacebook.com>
 <20160422205232.GB7004@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160422205232.GB7004@kernel.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1892
Lines: 42

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.