All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf synthetic-events: save 4kb from 2 stack frames
@ 2020-03-27  6:36 Ian Rogers
  2020-03-27 13:23 ` Jiri Olsa
  0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2020-03-27  6:36 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	linux-kernel
  Cc: Stephane Eranian, Ian Rogers

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'.

Signed-off-by: Ian Rogers <irogers@google.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


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] perf synthetic-events: save 4kb from 2 stack frames
  2020-03-27  6:36 [PATCH] perf synthetic-events: save 4kb from 2 stack frames Ian Rogers
@ 2020-03-27 13:23 ` Jiri Olsa
  2020-03-27 17:33   ` Ian Rogers
  0 siblings, 1 reply; 3+ messages in thread
From: Jiri Olsa @ 2020-03-27 13:23 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, linux-kernel,
	Stephane Eranian

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

jirka

> 
> Signed-off-by: Ian Rogers <irogers@google.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
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] perf synthetic-events: save 4kb from 2 stack frames
  2020-03-27 13:23 ` Jiri Olsa
@ 2020-03-27 17:33   ` Ian Rogers
  0 siblings, 0 replies; 3+ messages in thread
From: Ian Rogers @ 2020-03-27 17:33 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, LKML,
	Stephane Eranian

On Fri, Mar 27, 2020 at 6:24 AM Jiri Olsa <jolsa@redhat.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@google.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
> >
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-27 17:33 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27  6:36 [PATCH] perf synthetic-events: save 4kb from 2 stack frames Ian Rogers
2020-03-27 13:23 ` Jiri Olsa
2020-03-27 17:33   ` Ian Rogers

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.