Skip to contentSkip to conversation

perf tools: Decompress kernel module when reading DSO data

PATCH In review Namhyung Kim 2 participants 6 messages Jun 1, 2017 – Jun 3, 2017 perf events

lore
Share & tools
lore

Namhyung Kim

author maintainer
#0 lore raw

PATCH 1/2 perf tools: Decompress kernel module when reading DSO data

Currently perf decompresses kernel modules when loading symbol table but it missed to do it when reading raw data. Move decompress_kmodule() to util/dso.c and do not unlink it immediately.

---
 tools/perf/util/dso.c        | 56 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/dso.h        |  2 ++
 tools/perf/util/symbol-elf.c | 40 ++++---------------------------
 3 files changed, 63 insertions(+), 35 deletions(-)
tools/perf/util/dso.c +56 -0
index b27d127cdf68..8dc1f147547c 100644
@@ -350,6 +350,45 @@ void dso__set_module_info(struct dso *dso, struct kmod_path *m,
 	dso__set_short_name(dso, strdup(m->name), true);
 }
 
+int dso__decompress_kmodule(struct dso *dso, const char *filename,
+			    char *buf, size_t sz)
+{
+	struct kmod_path m = {};
+	int ret = -1;
+	int fd = -1;
+
+	if (!dso__needs_decompress(dso))
+		return -1;
+
+	if (kmod_path__parse_ext(&m, filename) < 0)
+		goto out;
+
+	if (!is_supported_compression(m.ext))
+		goto out;
+
+	snprintf(buf, sz, "/tmp/perf-kmod-XXXXXX");
+
+	fd = mkstemp(buf);
+	if (fd < 0) {
+		dso->load_errno = errno;
+		goto out;
+	}
+
+	pr_debug("decompressing to %s\n", buf);
+	if (!decompress_to_file(m.ext, filename, fd)) {
+		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
+		unlink(buf);
+		close(fd);
+		goto out;
+	}
+	ret = fd;
+
+out:
+	free(m.ext);
+
+	return ret;
+}
+
 /*
  * Global list of open DSOs and the counter.
  */
@@ -399,6 +438,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	int fd;
 	char *root_dir = (char *)"";
 	char *name = malloc(PATH_MAX);
+	bool needs_unlink = false;
 
 	if (!name)
 		return -ENOMEM;
@@ -415,7 +455,23 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	if (!is_regular_file(name))
 		return -EINVAL;
 
+	if (dso__needs_decompress(dso)) {
+		char buf[32];
+
+		fd = dso__decompress_kmodule(dso, name, buf, sizeof(buf));
+		if (fd < 0)
+			return -1;
+
+		close(fd);
+		strcpy(name, buf);
+		needs_unlink = true;
+	}
+
 	fd = do_open(name);
+
+	if (needs_unlink)
+		unlink(name);
+
 	free(name);
 	return fd;
 }
tools/perf/util/dso.h +2 -0
index 5fe2ab5877bd..c1e4b94d41aa 100644
@@ -244,6 +244,8 @@ bool is_supported_compression(const char *ext);
 bool is_kernel_module(const char *pathname, int cpumode);
 bool decompress_to_file(const char *ext, const char *filename, int output_fd);
 bool dso__needs_decompress(struct dso *dso);
+int dso__decompress_kmodule(struct dso *dso, const char *filename,
+			    char *buf, size_t sz);
 
 struct kmod_path {
 	char *name;
tools/perf/util/symbol-elf.c +5 -35
index 1fb2efae4f02..b335f91f0cb3 100644
@@ -637,40 +637,6 @@ static int dso__swap_init(struct dso *dso, unsigned char eidata)
 	return 0;
 }
 
-static int decompress_kmodule(struct dso *dso, const char *name,
-			      enum dso_binary_type type)
-{
-	int fd = -1;
-	char tmpbuf[] = "/tmp/perf-kmod-XXXXXX";
-	struct kmod_path m;
-
-	if (type != DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__GUEST_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__BUILD_ID_CACHE)
-		return -1;
-
-	if (kmod_path__parse_ext(&m, dso->long_name) || !m.comp)
-		return -1;
-
-	fd = mkstemp(tmpbuf);
-	if (fd < 0) {
-		dso->load_errno = errno;
-		goto out;
-	}
-
-	if (!decompress_to_file(m.ext, name, fd)) {
-		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
-		close(fd);
-		fd = -1;
-	}
-
-	unlink(tmpbuf);
-
-out:
-	free(m.ext);
-	return fd;
-}
-
 bool symsrc__possibly_runtime(struct symsrc *ss)
 {
 	return ss->dynsym || ss->opdsec;
@@ -702,9 +668,13 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 	int fd;
 
 	if (dso__needs_decompress(dso)) {
-		fd = decompress_kmodule(dso, name, type);
+		char buf[32];
+
+		fd = dso__decompress_kmodule(dso, name, buf, sizeof(buf));
 		if (fd < 0)
 			return -1;
+
+		unlink(buf);
 	} else {
 		fd = open(name, O_RDONLY);
 		if (fd < 0) {
Signature · 1 line
2.13.0
author maintainer
#1 lore raw

PATCH 2/2 perf test: Decompress kernel module before objdump

If a kernel modules is compressed, it should be decompressed before running objdump to parse binary data correctly. This fixes a failure of object code reading test for me.

---
 tools/perf/tests/code-reading.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)
tools/perf/tests/code-reading.c +28 -1
index 1f14e7612cbb..5031acdf56dd 100644
@@ -229,6 +229,7 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 	unsigned char buf2[BUFSZ];
 	size_t ret_len;
 	u64 objdump_addr;
+	const char *objdump_name;
 	int ret;
 
 	pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
@@ -289,9 +290,35 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 		state->done[state->done_cnt++] = al.map->start;
 	}
 
+	objdump_name = al.map->dso->long_name;
+	if (dso__needs_decompress(al.map->dso)) {
+		char buf[32];  /* for "/tmp/perf-kmod-XXXXXX" */
+		int fd;
+
+		fd = dso__decompress_kmodule(al.map->dso, objdump_name,
+					     buf, sizeof(buf));
+		if (fd < 0) {
+			pr_debug("decompression failed\n");
+			return -1;
+		}
+		close(fd);
+
+		objdump_name = strdup(buf);
+		if (objdump_name == NULL) {
+			pr_debug("memory allocation failed\n");
+			return -1;
+		}
+	}
+
 	/* Read the object code using objdump */
 	objdump_addr = map__rip_2objdump(al.map, al.addr);
-	ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2, len);
+	ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
+
+	if (objdump_name != al.map->dso->long_name) {
+		unlink(objdump_name);
+		free((void *)objdump_name);
+	}
+
 	if (ret > 0) {
 		/*
 		 * The kernel maps are inaccurate - assume objdump is right in
Signature · 1 line
2.13.0
#2 lore raw
In reply to Namhyung Kim

On Thu, Jun 01, 2017 at 06:06:04PM +0900, Namhyung Kim wrote:

SNIP

Quoted 9 lines
tools/perf/util/dso.c +1 -0
@@ -399,6 +438,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	int fd;
 	char *root_dir = (char *)"";
 	char *name = malloc(PATH_MAX);
+	bool needs_unlink = false;
 
 	if (!name)
 		return -ENOMEM;

tools/perf/util/dso.c static int __open_dso(struct dso *dso, struct machine *machine) · line 462

Quoted 8 lines
tools/perf/util/dso.c +5 -0
@@ -415,7 +455,23 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	if (!is_regular_file(name))
 		return -EINVAL;
 
+	if (dso__needs_decompress(dso)) {
+		char buf[32];
+
+		fd = dso__decompress_kmodule(dso, name, buf, sizeof(buf));
+		if (fd < 0)

free(name)

tools/perf/util/dso.c static int __open_dso(struct dso *dso, struct machine *machine) · line 466

Quoted 4 lines
tools/perf/util/dso.c +4 -0
+			return -1;
+
+		close(fd);
+		strcpy(name, buf);

also why not use name directly instead of buf?

jirka

author maintainer
#3 lore raw

Hi Jiri,

Quoted 16 lines · Jiri Olsa

On Thu, Jun 01, 2017 at 06:06:04PM +0900, Namhyung Kim wrote:

SNIP

Quoted 12 lines
tools/perf/util/dso.c +1 -0
@@ -399,6 +438,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
      int fd;
      char *root_dir = (char *)"";
      char *name = malloc(PATH_MAX);
+     bool needs_unlink = false;
 
      if (!name)
              return -ENOMEM;
@@ -415,7 +455,23 @@ static int __open_dso(struct dso *dso, struct machine *machine)
      if (!is_regular_file(name))
              return -EINVAL;
 
Quoted 7 lines

tools/perf/util/dso.c static int __open_dso(struct dso *dso, struct machine *machine) · line 462

Quoted 5 lines
tools/perf/util/dso.c +5 -0
+     if (dso__needs_decompress(dso)) {
+             char buf[32];
+
+             fd = dso__decompress_kmodule(dso, name, buf, sizeof(buf));
+             if (fd < 0)

free(name)

ok.

Quoted 7 lines

tools/perf/util/dso.c static int __open_dso(struct dso *dso, struct machine *machine) · line 466

Quoted 5 lines
tools/perf/util/dso.c +4 -0
+                     return -1;
+
+             close(fd);
+             strcpy(name, buf);

also why not use name directly instead of buf?

The name should have original compressed filename and buf will be set to a new decompressed name.

Thanks,
Namhyung

#4 lore raw
In reply to Namhyung Kim

On Thu, Jun 01, 2017 at 06:06:04PM +0900, Namhyung Kim wrote:

SNIP

Quoted 14 lines
tools/perf/util/symbol-elf.c +0 -9
-	}
-
-	unlink(tmpbuf);
-
-out:
-	free(m.ext);
-	return fd;
-}
-
 bool symsrc__possibly_runtime(struct symsrc *ss)
 {
 	return ss->dynsym || ss->opdsec;
@@ -702,9 +668,13 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 	int fd;

tools/perf/util/symbol-elf.c int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name, · line 677

Quoted 10 lines
tools/perf/util/symbol-elf.c +5 -1
 
 	if (dso__needs_decompress(dso)) {
-		fd = decompress_kmodule(dso, name, type);
+		char buf[32];
+
+		fd = dso__decompress_kmodule(dso, name, buf, sizeof(buf));
 		if (fd < 0)
 			return -1;
+
+		unlink(buf);

I wonder we could make this more clear and add:

dso__decompress_kmodule_fd
dso__decompress_kmodule_path

_fd version would remove the file and _path version would close the descriptor

in any case, could you please split the change so the changes to decompress_kmodule are apparent?

thanks,
jirka

author maintainer
#5 lore raw
Quoted 31 lines · Jiri Olsa

On Thu, Jun 01, 2017 at 06:06:04PM +0900, Namhyung Kim wrote:

SNIP

Quoted 14 lines
tools/perf/util/symbol-elf.c +0 -9
-     }
-
-     unlink(tmpbuf);
-
-out:
-     free(m.ext);
-     return fd;
-}
-
 bool symsrc__possibly_runtime(struct symsrc *ss)
 {
      return ss->dynsym || ss->opdsec;
@@ -702,9 +668,13 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
      int fd;

tools/perf/util/symbol-elf.c int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name, · line 677

Quoted 10 lines
tools/perf/util/symbol-elf.c +5 -1
 
      if (dso__needs_decompress(dso)) {
-             fd = decompress_kmodule(dso, name, type);
+             char buf[32];
+
+             fd = dso__decompress_kmodule(dso, name, buf, sizeof(buf));
              if (fd < 0)
                      return -1;
+
+             unlink(buf);

I wonder we could make this more clear and add:

Quoted 7 lines

dso__decompress_kmodule_fd
dso__decompress_kmodule_path

_fd version would remove the file and _path version would close the descriptor

in any case, could you please split the change so the changes to decompress_kmodule are apparent?

Will do.

Thanks,
Namhyung