Date:   Fri, 27 Mar 2020 10:33:07 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH] perf synthetic-events: save 4kb from 2 stack frames

On Fri, Mar 27, 2020 at 6:24 AM Jiri Olsa <jolsa@...hat.com> wrote:
>
> On Thu, Mar 26, 2020 at 11:36:51PM -0700, Ian Rogers wrote:
> > Reduce the scope of PATH_MAX sized char buffers so that the compiler can
> > overlap their usage.
> >
> > perf_event__synthesize_mmap_events before 'sub $0x45b8,%rsp' after
> > 'sub $0x35b8,%rsp'.
> >
> > perf_event__get_comm_ids before 'sub $0x2028,%rsp' after 'sub $0x1028,%rsp'.
>
> nice catch.. is this actualy problem somewhere? I thought
> we don't need to care that much about this in user space

I did perhaps a cleaner v2 here:
https://lore.kernel.org/lkml/20200327172914.28603-1-irogers@google.com/T/#u
I have some local changes that required some more arrays in
perf_event__synthesize_mmap_events, this hit a frame limit size with
our compiler set up. This change was the simplest workaround. I agree
this patch isn't essential but it should generally benefit performance
a little.

Thanks,
Ian

> jirka
>
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/util/synthetic-events.c | 46 ++++++++++++++++++------------
> >  1 file changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 3f28af39f9c6..9ff54707bb30 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -70,7 +70,6 @@ int perf_tool__process_synth_event(struct perf_tool *tool,
> >  static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
> >                                   pid_t *tgid, pid_t *ppid)
> >  {
> > -     char filename[PATH_MAX];
> >       char bf[4096];
> >       int fd;
> >       size_t size = 0;
> > @@ -80,12 +79,16 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
> >       *tgid = -1;
> >       *ppid = -1;
> >
> > -     snprintf(filename, sizeof(filename), "/proc/%d/status", pid);
> > +     {
> > +             char filename[PATH_MAX];
> >
> > -     fd = open(filename, O_RDONLY);
> > -     if (fd < 0) {
> > -             pr_debug("couldn't open %s\n", filename);
> > -             return -1;
> > +             snprintf(filename, sizeof(filename), "/proc/%d/status", pid);
> > +
> > +             fd = open(filename, O_RDONLY);
> > +             if (fd < 0) {
> > +                     pr_debug("couldn't open %s\n", filename);
> > +                     return -1;
> > +             }
> >       }
> >
> >       n = read(fd, bf, sizeof(bf) - 1);
> > @@ -280,7 +283,6 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
> >                                      struct machine *machine,
> >                                      bool mmap_data)
> >  {
> > -     char filename[PATH_MAX];
> >       FILE *fp;
> >       unsigned long long t;
> >       bool truncation = false;
> > @@ -292,18 +294,22 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
> >       if (machine__is_default_guest(machine))
> >               return 0;
> >
> > -     snprintf(filename, sizeof(filename), "%s/proc/%d/task/%d/maps",
> > -              machine->root_dir, pid, pid);
> > +#define FILENAME_FMT_STRING "%s/proc/%d/task/%d/maps"
> > +     {
> > +             char filename[PATH_MAX];
> >
> > -     fp = fopen(filename, "r");
> > -     if (fp == NULL) {
> > -             /*
> > -              * We raced with a task exiting - just return:
> > -              */
> > -             pr_debug("couldn't open %s\n", filename);
> > -             return -1;
> > -     }
> > +             snprintf(filename, sizeof(filename), FILENAME_FMT_STRING,
> > +                     machine->root_dir, pid, pid);
> >
> > +             fp = fopen(filename, "r");
> > +             if (fp == NULL) {
> > +                     /*
> > +                      * We raced with a task exiting - just return:
> > +                      */
> > +                     pr_debug("couldn't open %s\n", filename);
> > +                     return -1;
> > +             }
> > +     }
> >       event->header.type = PERF_RECORD_MMAP2;
> >       t = rdclock();
> >
> > @@ -320,10 +326,10 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
> >                       break;
> >
> >               if ((rdclock() - t) > timeout) {
> > -                     pr_warning("Reading %s time out. "
> > +                     pr_warning("Reading " FILENAME_FMT_STRING " time out. "
> >                                  "You may want to increase "
> >                                  "the time limit by --proc-map-timeout\n",
> > -                                filename);
> > +                                machine->root_dir, pid, pid);
> >                       truncation = true;
> >                       goto out;
> >               }
> > @@ -412,6 +418,8 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
> >
> >       fclose(fp);
> >       return rc;
> > +
> > +#undef FILENAME_FMT_STRING
> >  }
> >
> >  int perf_event__synthesize_modules(struct perf_tool *tool, perf_event__handler_t process,
> > --
> > 2.25.1.696.g5e7596f4ac-goog
> >
>

Powered by blists - more mailing lists