From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751445AbbCTQMj (ORCPT ); Fri, 20 Mar 2015 12:12:39 -0400 Received: from mail-ig0-f171.google.com ([209.85.213.171]:35336 "EHLO mail-ig0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750852AbbCTQMf (ORCPT ); Fri, 20 Mar 2015 12:12:35 -0400 Message-ID: <550C46F1.40103@gmail.com> Date: Fri, 20 Mar 2015 10:12:33 -0600 From: David Ahern User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Arnaldo Carvalho de Melo CC: linux-kernel@vger.kernel.org, Steven Rostedt , Namhyung Kim , Jiri Olsa Subject: Re: [PATCH] perf trace: Handle legacy syscalls References: <1426790181-19118-1-git-send-email-dsahern@gmail.com> <20150320153226.GI16485@kernel.org> <550C413A.2010301@gmail.com> <20150320160640.GL16485@kernel.org> In-Reply-To: <20150320160640.GL16485@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/20/15 10:06 AM, Arnaldo Carvalho de Melo wrote: > Em Fri, Mar 20, 2015 at 09:48:10AM -0600, David Ahern escreveu: >> On 3/20/15 9:32 AM, Arnaldo Carvalho de Melo wrote: >>> But then I don't think we need to do that, i.e. we can just have a >>> boolean we set at some point to tell that we need to skip the first >>> entry. >>> >>> I'll try to cook up a patch for that. >> >> why have a boolean that is checked every time through the loop when its >> value will always be the same for a give run? why not just remove the entry >> as I suggested? > > First gut reaction was: hey, we got that from a library, libtraceevent, > that could have other users, etc, better not to change it behind its > back. > > I.e. something like this, based on your approach, but not modifying the > data structures received from libtraceevent: > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c > index 6af6bcec930e..001c6ae9a1b1 100644 > --- a/tools/perf/builtin-trace.c > +++ b/tools/perf/builtin-trace.c > @@ -1135,6 +1135,8 @@ static struct syscall_fmt *syscall_fmt__find(const char *name) > > struct syscall { > struct event_format *tp_format; > + int nr_args; > + struct format_field *args; > const char *name; > bool filtered; > bool is_exit; > @@ -1442,14 +1444,14 @@ static int syscall__set_arg_fmts(struct syscall *sc) > struct format_field *field; > int idx = 0; > > - sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields - 1, sizeof(void *)); > + sc->arg_scnprintf = calloc(sc->nr_args, sizeof(void *)); > if (sc->arg_scnprintf == NULL) > return -1; > > if (sc->fmt) > sc->arg_parm = sc->fmt->arg_parm; > > - for (field = sc->tp_format->format.fields->next; field; field = field->next) { > + for (field = sc->args; field; field = field->next) { > if (sc->fmt && sc->fmt->arg_scnprintf[idx]) > sc->arg_scnprintf[idx] = sc->fmt->arg_scnprintf[idx]; > else if (field->flags & FIELD_IS_POINTER) > @@ -1515,6 +1517,14 @@ static int trace__read_syscall_info(struct trace *trace, int id) > if (sc->tp_format == NULL) > return -1; > > + sc->args = sc->tp_format->format.fields; > + sc->nr_args = sc->tp_format->format.nr_fields; > + /* drop nr field - not relevant here; does not exist on older kernels */ > + if (sc->args && strcmp(sc->args->name, "nr") == 0) { > + sc->args = sc->args->next; > + --sc->nr_args; > + } > + > sc->is_exit = !strcmp(name, "exit_group") || !strcmp(name, "exit"); > > return syscall__set_arg_fmts(sc); > @@ -1537,7 +1547,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size, > unsigned char *p; > unsigned long val; > > - if (sc->tp_format != NULL) { > + if (sc->args != NULL) { > struct format_field *field; > u8 bit = 1; > struct syscall_arg arg = { > @@ -1547,7 +1557,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size, > .thread = thread, > }; > > - for (field = sc->tp_format->format.fields->next; field; > + for (field = sc->args; field; > field = field->next, ++arg.idx, bit <<= 1) { > if (arg.mask & bit) > continue; > Seems reasonable.