Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752447AbcDVVhm (ORCPT ); Fri, 22 Apr 2016 17:37:42 -0400 Received: from mail-pf0-f171.google.com ([209.85.192.171]:36686 "EHLO mail-pf0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751742AbcDVVhk (ORCPT ); Fri, 22 Apr 2016 17:37:40 -0400 Date: Fri, 22 Apr 2016 14:37:35 -0700 From: Alexei Starovoitov To: Arnaldo Carvalho de Melo Cc: Peter Zijlstra , Frederic Weisbecker , Ingo Molnar , Adrian Hunter , Brendan Gregg , Alexander Shishkin , Alexei Starovoitov , David Ahern , He Kuang , Jiri Olsa , Masami Hiramatsu , Milian Wolff , Namhyung Kim , Stephane Eranian , Thomas Gleixner , Vince Weaver , Wang Nan , Zefan Li , Linux Kernel Mailing List 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 List-ID: X-Mailing-List: linux-kernel@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.