Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751518AbaLPVAx (ORCPT ); Tue, 16 Dec 2014 16:00:53 -0500 Received: from mail-ob0-f172.google.com ([209.85.214.172]:45480 "EHLO mail-ob0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750976AbaLPVAw (ORCPT ); Tue, 16 Dec 2014 16:00:52 -0500 MIME-Version: 1.0 In-Reply-To: <20141216204449.GA9327@redhat.com> References: <20141216165358.GA9310@thinkpad> <20141216204449.GA9327@redhat.com> Date: Tue, 16 Dec 2014 16:00:51 -0500 Message-ID: Subject: Re: [PATCH] perf mem: enable simultaneous sampling of loads/stores From: Stephane Eranian To: Arnaldo Carvalho de Melo Cc: LKML , Peter Zijlstra , "mingo@elte.hu" , "ak@linux.intel.com" , Jiri Olsa , David Ahern , Don Zickus , Namhyung Kim , Joe Mario , Richard Fowles Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 16, 2014 at 3:44 PM, Arnaldo Carvalho de Melo wrote: > > Em Tue, Dec 16, 2014 at 05:53:58PM +0100, Stephane Eranian escreveu: > > > > This patch modifies perf mem to default to sampling loads > > and stores simultaneously. It could only do one or the other > > before yet there was not hardware restriction preventing > > simultaneous collection. With this patch, one run is sufficient > > to collect both. > > > > By default load/stores are sampled now: > > $ perf mem rec > > $ perf mem rep > > > > It is still possible to sample only loads or stores by using the > > -t option: > > $ perf mem -t load rec > > $ perf mem -t load rep > > Or > > $ perf mem -t store rec > > $ perf mem -t store rep > > > > The perf report TUI will show one event at a time. The store > > output will contain a Weight column which will be empty. > > Can you please update the man page and resubmit? > Ok, will do that. > > Are those unchecked strdups really needed? > Well, it all boils down to how cmd_xx() declares argv: - const char **argv - char *argv If the latter, then cannot simply do rec[i++] = "report" otherwise we may crash it the ever decides to modify that string. Given that cmd_record() and cmd_report() use const char *argv, then I can use literals. Right? > > - Arnaldo > > > Signed-off-by: Stephane Eranian > > --- > > tools/perf/builtin-mem.c | 107 ++++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 92 insertions(+), 15 deletions(-) > > > > diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c > > index 24db6ff..52dbaae 100644 > > --- a/tools/perf/builtin-mem.c > > +++ b/tools/perf/builtin-mem.c > > @@ -7,10 +7,13 @@ > > #include "util/session.h" > > #include "util/data.h" > > > > -#define MEM_OPERATION_LOAD "load" > > -#define MEM_OPERATION_STORE "store" > > +#define MEM_OPERATION_LOAD 0x1 > > +#define MEM_OPERATION_STORE 0x2 > > > > -static const char *mem_operation = MEM_OPERATION_LOAD; > > +/* > > + * default to both load an store sampling > > + */ > > +static int mem_operation = MEM_OPERATION_LOAD | MEM_OPERATION_STORE; > > > > struct perf_mem { > > struct perf_tool tool; > > @@ -25,26 +28,30 @@ static int __cmd_record(int argc, const char **argv) > > { > > int rec_argc, i = 0, j; > > const char **rec_argv; > > - char event[64]; > > int ret; > > > > - rec_argc = argc + 4; > > + rec_argc = argc + 7; /* max number of arguments */ > > rec_argv = calloc(rec_argc + 1, sizeof(char *)); > > if (!rec_argv) > > return -1; > > > > rec_argv[i++] = strdup("record"); > > - if (!strcmp(mem_operation, MEM_OPERATION_LOAD)) > > + > > + if (mem_operation & MEM_OPERATION_LOAD) > > rec_argv[i++] = strdup("-W"); > > + > > rec_argv[i++] = strdup("-d"); > > - rec_argv[i++] = strdup("-e"); > > > > - if (strcmp(mem_operation, MEM_OPERATION_LOAD)) > > - sprintf(event, "cpu/mem-stores/pp"); > > - else > > - sprintf(event, "cpu/mem-loads/pp"); > > + if (mem_operation & MEM_OPERATION_LOAD) { > > + rec_argv[i++] = strdup("-e"); > > + rec_argv[i++] = strdup("cpu/mem-loads/pp"); > > + } > > + > > + if (mem_operation & MEM_OPERATION_STORE) { > > + rec_argv[i++] = strdup("-e"); > > + rec_argv[i++] = strdup("cpu/mem-stores/pp"); > > + } > > > > - rec_argv[i++] = strdup(event); > > for (j = 1; j < argc; j++, i++) > > rec_argv[i] = argv[j]; > > > > @@ -170,7 +177,7 @@ static int report_events(int argc, const char **argv, struct perf_mem *mem) > > * there is no weight (cost) associated with stores, so don't print > > * the column > > */ > > - if (strcmp(mem_operation, MEM_OPERATION_LOAD)) > > + if (!(mem_operation & MEM_OPERATION_LOAD)) > > rep_argv[i++] = strdup("--sort=mem,sym,dso,symbol_daddr," > > "dso_daddr,tlb,locked"); > > > > @@ -182,6 +189,75 @@ static int report_events(int argc, const char **argv, struct perf_mem *mem) > > return ret; > > } > > > > +struct mem_mode { > > + const char *name; > > + int mode; > > +}; > > + > > +#define MEM_OPT(n, m) \ > > + { .name = n, .mode = (m) } > > + > > +#define MEM_END { .name = NULL } > > + > > +static const struct mem_mode mem_modes[]={ > > + MEM_OPT("load", MEM_OPERATION_LOAD), > > + MEM_OPT("store", MEM_OPERATION_STORE), > > + MEM_END > > +}; > > + > > +static int > > +parse_mem_ops(const struct option *opt, const char *str, int unset) > > +{ > > + int *mode = (int *)opt->value; > > + const struct mem_mode *m; > > + char *s, *os = NULL, *p; > > + int ret = -1; > > + > > + if (unset) > > + return 0; > > + > > + /* str may be NULL in case no arg is passed to -t */ > > + if (str) { > > + /* because str is read-only */ > > + s = os = strdup(str); > > + if (!s) > > + return -1; > > + > > + /* reset mode */ > > + *mode = 0; > > + > > + for (;;) { > > + p = strchr(s, ','); > > + if (p) > > + *p = '\0'; > > + > > + for (m = mem_modes; m->name; m++) { > > + if (!strcasecmp(s, m->name)) > > + break; > > + } > > + if (!m->name) { > > + fprintf(stderr, "unknown sampling op %s," > > + " check man page\n", s); > > + goto error; > > + } > > + > > + *mode |= m->mode; > > + > > + if (!p) > > + break; > > + > > + s = p + 1; > > + } > > + } > > + ret = 0; > > + > > + if (*mode == 0) > > + *mode = MEM_OPERATION_LOAD; > > +error: > > + free(os); > > + return ret; > > +} > > + > > int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused) > > { > > struct stat st; > > @@ -199,8 +275,9 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused) > > .input_name = "perf.data", > > }; > > const struct option mem_options[] = { > > - OPT_STRING('t', "type", &mem_operation, > > - "type", "memory operations(load/store)"), > > + OPT_CALLBACK('t', "type", &mem_operation, > > + "type", "memory operations(load,store) Default load,store", > > + parse_mem_ops), > > OPT_BOOLEAN('D', "dump-raw-samples", &mem.dump_raw, > > "dump raw samples in ASCII"), > > OPT_BOOLEAN('U', "hide-unresolved", &mem.hide_unresolved, > > -- > > 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/