Re: [PATCH 12/19] perf, tool: Back [vdso] DSO with real data

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Mon, Jun 11, 2012 at 03:20:07PM +0200, Jiri Olsa escreveu:
> Storing data for VDSO shared object, because we need it for
> the unwind process.
> 
> The idea is that VDSO shared object is same for all process
> on a running system, so it makes no difference if we store
> it inside the tracer - perf.
> 
> The record command:
> When [vdso] map memory is hit, we retrieve [vdso] DSO image
> and store it into temporary file. During the build-id
> processing the [vdso] DSO image is stored in build-id db,
> and build-id refference is made inside perf.data. The temporary
> file is removed when record is finished.
> 
> The report command:
> We read build-id from perf.data and store [vdso] DSO object.
> This object is refferenced and attached to map when the MMAP
> events are processed. Thus during the SAMPLE event processing
> we have correct mmap/dso attached.
> 
> Adding following API for vdso object:
>   vdso__file
>     - vdso temp file path
> 
>   vdso__get_file
>     - finds and store VDSO image into temp file,
>       the temp file path is returned
> 
>   vdso__exit
>     - removes temporary VDSO image if there's any
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/Makefile       |    2 +
>  tools/perf/util/map.c     |   23 ++++++++++-
>  tools/perf/util/session.c |    2 +
>  tools/perf/util/vdso.c    |   90 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/vdso.h    |    8 ++++
>  5 files changed, 123 insertions(+), 2 deletions(-)
>  create mode 100644 tools/perf/util/vdso.c
>  create mode 100644 tools/perf/util/vdso.h
> 
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index 0eee64c..e48b969 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -319,6 +319,7 @@ LIB_H += $(ARCH_INCLUDE)
>  LIB_H += util/cgroup.h
>  LIB_H += $(TRACE_EVENT_DIR)event-parse.h
>  LIB_H += util/target.h
> +LIB_H += util/vdso.h
>  
>  LIB_OBJS += $(OUTPUT)util/abspath.o
>  LIB_OBJS += $(OUTPUT)util/alias.o
> @@ -382,6 +383,7 @@ LIB_OBJS += $(OUTPUT)util/xyarray.o
>  LIB_OBJS += $(OUTPUT)util/cpumap.o
>  LIB_OBJS += $(OUTPUT)util/cgroup.o
>  LIB_OBJS += $(OUTPUT)util/target.o
> +LIB_OBJS += $(OUTPUT)util/vdso.o
>  
>  BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o
>  
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 35ae568..1649ea0 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -7,6 +7,7 @@
>  #include <stdio.h>
>  #include <unistd.h>
>  #include "map.h"
> +#include "vdso.h"
>  
>  const char *map_type__name[MAP__NR_TYPES] = {
>  	[MAP__FUNCTION] = "Functions",
> @@ -18,10 +19,14 @@ static inline int is_anon_memory(const char *filename)
>  	return strcmp(filename, "//anon") == 0;
>  }
>  
> +static inline int is_vdso_memory(const char *filename)
> +{
> +	return !strcmp(filename, "[vdso]");
> +}
> +
>  static inline int is_no_dso_memory(const char *filename)
>  {
>  	return !strcmp(filename, "[stack]") ||
> -	       !strcmp(filename, "[vdso]")  ||
>  	       !strcmp(filename, "[heap]");
>  }
>  
> @@ -50,9 +55,10 @@ struct map *map__new(struct list_head *dsos__list, u64 start, u64 len,
>  	if (self != NULL) {
>  		char newfilename[PATH_MAX];
>  		struct dso *dso;
> -		int anon, no_dso;
> +		int anon, no_dso, vdso;
>  
>  		anon = is_anon_memory(filename);
> +		vdso = is_vdso_memory(filename);
>  		no_dso = is_no_dso_memory(filename);
>  
>  		if (anon) {
> @@ -60,10 +66,23 @@ struct map *map__new(struct list_head *dsos__list, u64 start, u64 len,
>  			filename = newfilename;
>  		}
>  
> +		if (vdso) {
> +			filename = (char *) vdso__file;
> +			pgoff = 0;
> +		}
> +
>  		dso = __dsos__findnew(dsos__list, filename);
>  		if (dso == NULL)
>  			goto out_delete;
>  
> +		if (vdso && !dso->has_build_id) {
> +			char *file_vdso = vdso__get_file();
> +			if (file_vdso)
> +				dso__set_long_name(dso, file_vdso);
> +			else
> +				no_dso = 1;
> +		}
> +
>  		map__init(self, type, start, start + len, pgoff, dso);
>  
>  		if (anon || no_dso) {
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 2785ce8..f400612 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -14,6 +14,7 @@
>  #include "sort.h"
>  #include "util.h"
>  #include "cpumap.h"
> +#include "vdso.h"
>  
>  static int perf_session__open(struct perf_session *self, bool force)
>  {
> @@ -209,6 +210,7 @@ void perf_session__delete(struct perf_session *self)
>  	machine__exit(&self->host_machine);
>  	close(self->fd);
>  	free(self);
> +	vdso__exit();
>  }
>  
>  void machine__remove_thread(struct machine *self, struct thread *th)
> diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
> new file mode 100644
> index 0000000..e964482
> --- /dev/null
> +++ b/tools/perf/util/vdso.c
> @@ -0,0 +1,90 @@
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <linux/kernel.h>
> +#include "vdso.h"
> +#include "util.h"
> +
> +const char vdso__file[] = "/tmp/vdso.so";

Oops, can you please use mkstemp()?

> +static bool vdso_found;
> +
> +static int find_vdso_map(void **start, void **end)
> +{
> +	FILE *maps;
> +	char line[128];
> +	int found = 0;
> +
> +	maps = fopen("/proc/self/maps", "r");
> +	if (!maps) {
> +		pr_err("vdso: cannot open maps\n");
> +		return -1;
> +	}
> +
> +	while (!found && fgets(line, sizeof(line), maps)) {
> +		int m = -1;
> +
> +		/* We care only about private r-x mappings. */
> +		if (2 != sscanf(line, "%p-%p r-xp %*x %*x:%*x %*u %n",
> +				start, end, &m))
> +			continue;
> +		if (m < 0)
> +			continue;
> +
> +		if (!strncmp(&line[m], "[vdso]", 6))
> +			found = 1;
> +	}
> +
> +	fclose(maps);
> +	return !found;
> +}
> +
> +char *vdso__get_file(void)
> +{
> +	char *vdso = NULL;
> +	char *buf = NULL;
> +	void *start, *end;
> +
> +	do {
> +		int fd, size;
> +
> +		if (vdso_found) {
> +			vdso = (char *) vdso__file;

codying stile bzzt, use:

			vdso = (char *)vdso__file;

> +			break;
> +		}
> +
> +		if (find_vdso_map(&start, &end))
> +			break;
> +
> +		size = end - start;
> +		buf = malloc(size);
> +		if (!buf)
> +			break;
> +
> +		memcpy(buf, start, size);

Introduce memdup, just like in the kernel we have kmemdup?

> +
> +		fd = open(vdso__file, O_CREAT|O_WRONLY|O_TRUNC, S_IRWXU);
> +		if (fd < 0)
> +			break;
> +
> +		if (size == write(fd, buf, size))
> +			vdso = (char *) vdso__file;

coding style

> +
> +		close(fd);
> +	} while (0);

And what is the point of this while construct?

> +
> +	if (buf)
> +		free(buf);

No need to check, free accepts NULL pointers.

> +
> +	vdso_found = (vdso != NULL);
> +	return vdso;
> +}
> +
> +void vdso__exit(void)
> +{
> +	if (vdso_found)
> +		unlink(vdso__file);

What is the point of using a global variable to check if you need to
call unlink if you don't check unlink's result? Just ditch vdso_found
and call unlink unconditionally, but on the temp file created.

And be careful how you use that temp filename...

> +}
> diff --git a/tools/perf/util/vdso.h b/tools/perf/util/vdso.h
> new file mode 100644
> index 0000000..908b041
> --- /dev/null
> +++ b/tools/perf/util/vdso.h
> @@ -0,0 +1,8 @@
> +#ifndef __VDSO__
> +#define __VDSO__

Better use __PERF_VDSO__

> +
> +extern const char vdso__file[];
> +char *vdso__get_file(void);
> +void  vdso__exit(void);
> +
> +#endif /* __VDSO__ */
> -- 
> 1.7.7.6