[perf/core PATCH v5 0/4] perf-buildid-cache: Enhance --update and add --purge

[perf/core PATCH v5 0/4] perf-buildid-cache: Enhance --update and add --purge

[Masami Hiramatsu]

Hi,

Here is the 5th version of of perf buildid-cache update.
This updates the 2nd patch and add 2 patches just for
cleanup and improve usability a bit.
Here are the changes in v5.

 - [2/4] Remove NULL check before calling strlist__delete()
         (Thanks to Hemant!)
 - [3/4] Use pr_debug instead of verbose && pr_info
         (Thanks to Namhyung!)
 - [4/4] Show usage with incorrect params 

Thank you,


---

Masami Hiramatsu (4):
      perf buildid-cache: Add new buildid cache if update target is not cached
      perf buildid-cache: Add --purge FILE to remove all caches of FILE
      perf-buildid-cache: Use pr_debug instead of verbose && pr_info
      perf-buildid-cache: Show usage with incorrect params


 tools/perf/Documentation/perf-buildid-cache.txt |   24 ++++--
 tools/perf/builtin-buildid-cache.c              |   69 ++++++++++++++--
 tools/perf/util/build-id.c                      |   97 +++++++++++++++++++----
 tools/perf/util/build-id.h                      |    2 
 4 files changed, 157 insertions(+), 35 deletions(-)

--
Masami HIRAMATSU
Software Platform Research Dpt. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[perf/core PATCH v5 1/4] perf buildid-cache: Add new buildid cache if update target is not cached

[Masami Hiramatsu]

Add new buildid cache if the update target file is not cached.
This can happen when an old binary is replaced by new one
after caching the old one. In this case, user sees his operation
just failed. But it does not look straight, since user just
pass the binary "path", not "build-id".

  ----
  # ./perf buildid-cache --add ./perf
  (update ./perf to new binary)
  # ./perf buildid-cache --update ./perf
  ./perf wasn't in the cache
  #
  ----

This patch adds given new binary to cache if the new binary is
not cached. So we'll not see the above error.

  ----
  # ./perf buildid-cache --add ./perf
  (update ./perf to new binary)
  # ./perf buildid-cache --update ./perf
  #
  ----

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/Documentation/perf-buildid-cache.txt |   11 ++++++++---
 tools/perf/builtin-buildid-cache.c              |    6 ++++--
 tools/perf/util/build-id.c                      |   12 ++++++++++++
 tools/perf/util/build-id.h                      |    1 +
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/tools/perf/Documentation/perf-buildid-cache.txt b/tools/perf/Documentation/perf-buildid-cache.txt
index 0294c57..cec6b57 100644
--- a/tools/perf/Documentation/perf-buildid-cache.txt
+++ b/tools/perf/Documentation/perf-buildid-cache.txt
@@ -41,9 +41,14 @@ OPTIONS
 --missing=::
 	List missing build ids in the cache for the specified file.
 -u::
---update::
-	Update specified file of the cache. It can be used to update kallsyms
-	kernel dso to vmlinux in order to support annotation.
+--update=::
+	Update specified file of the cache. Note that this doesn't remove
+	older entires since those may be still needed for annotating old
+	(or remote) perf.data. Only if there is already a cache which has
+	exactly same build-id, that is replaced by new one. It can be used
+	to update kallsyms and kernel dso to vmlinux in order to support
+	annotation.
+
 -v::
 --verbose::
 	Be more verbose.
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index d929d95..e7568f5 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -255,7 +255,7 @@ static int build_id_cache__update_file(const char *filename)
 	u8 build_id[BUILD_ID_SIZE];
 	char sbuild_id[BUILD_ID_SIZE * 2 + 1];
 
-	int err;
+	int err = 0;
 
 	if (filename__read_build_id(filename, &build_id, sizeof(build_id)) < 0) {
 		pr_debug("Couldn't read a build-id in %s\n", filename);
@@ -263,7 +263,9 @@ static int build_id_cache__update_file(const char *filename)
 	}
 
 	build_id__sprintf(build_id, sizeof(build_id), sbuild_id);
-	err = build_id_cache__remove_s(sbuild_id);
+	if (build_id_cache__cached(sbuild_id))
+		err = build_id_cache__remove_s(sbuild_id);
+
 	if (!err)
 		err = build_id_cache__add_s(sbuild_id, filename, false, false);
 
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index adbc360..0bc33be 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -352,6 +352,18 @@ static int build_id_cache__add_b(const u8 *build_id, size_t build_id_size,
 	return build_id_cache__add_s(sbuild_id, name, is_kallsyms, is_vdso);
 }
 
+bool build_id_cache__cached(const char *sbuild_id)
+{
+	bool ret = false;
+	char *filename = build_id__filename(sbuild_id, NULL, 0);
+
+	if (filename && !access(filename, F_OK))
+		ret = true;
+	free(filename);
+
+	return ret;
+}
+
 int build_id_cache__remove_s(const char *sbuild_id)
 {
 	const size_t size = PATH_MAX;
diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index 31b3c63..2a09498 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -22,6 +22,7 @@ bool perf_session__read_build_ids(struct perf_session *session, bool with_hits);
 int perf_session__write_buildid_table(struct perf_session *session, int fd);
 int perf_session__cache_build_ids(struct perf_session *session);
 
+bool build_id_cache__cached(const char *sbuild_id);
 int build_id_cache__add_s(const char *sbuild_id,
 			  const char *name, bool is_kallsyms, bool is_vdso);
 int build_id_cache__remove_s(const char *sbuild_id);

[perf/core PATCH v5 2/4] perf buildid-cache: Add --purge FILE to remove all caches of FILE

[Masami Hiramatsu]

Add --purge FILE to remove all caches of FILE.
Since the current --remove FILE removes a cache which has
same build-id of given FILE. Since the command takes a
FILE path, it can confuse user who tries to remove cache
about FILE path.

  -----
  # ./perf buildid-cache -v --add ./perf
  Adding 133b7b5486d987a5ab5c3ebf4ea14941f45d4d4f ./perf: Ok
  # (update the ./perf binary)
  # ./perf buildid-cache -v --remove ./perf
  Removing 305bbd1be68f66eca7e2d78db294653031edfa79 ./perf: FAIL
  ./perf wasn't in the cache
  -----
Actually, the --remove's FAIL is not shown, it just silently fails.

So, this patch adds --purge FILE action for such usecase.
perf buildid-cache --purge FILE removes all caches which
has same FILE path.
In other words, it removes all caches including old binaries.

  -----
  # ./perf buildid-cache -v --add ./perf
  Adding 133b7b5486d987a5ab5c3ebf4ea14941f45d4d4f ./perf: Ok
  # (update the ./perf binary)
  # ./perf buildid-cache -v --purge ./perf
  Removing 133b7b5486d987a5ab5c3ebf4ea14941f45d4d4f ./perf: Ok
  -----

BTW, if you want to purge all the caches, remove ~/.debug/* .

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/Documentation/perf-buildid-cache.txt |   13 ++--
 tools/perf/builtin-buildid-cache.c              |   44 ++++++++++++
 tools/perf/util/build-id.c                      |   85 ++++++++++++++++++-----
 tools/perf/util/build-id.h                      |    1 
 4 files changed, 122 insertions(+), 21 deletions(-)

diff --git a/tools/perf/Documentation/perf-buildid-cache.txt b/tools/perf/Documentation/perf-buildid-cache.txt
index cec6b57..dd07b55 100644
--- a/tools/perf/Documentation/perf-buildid-cache.txt
+++ b/tools/perf/Documentation/perf-buildid-cache.txt
@@ -12,9 +12,9 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-This command manages the build-id cache. It can add and remove files to/from
-the cache. In the future it should as well purge older entries, set upper
-limits for the space used by the cache, etc.
+This command manages the build-id cache. It can add, remove, update and purge
+files to/from the cache. In the future it should as well set upper limits for
+the space used by the cache, etc.
 
 OPTIONS
 -------
@@ -36,7 +36,12 @@ OPTIONS
         actually made.
 -r::
 --remove=::
-        Remove specified file from the cache.
+        Remove a cached binary which has same build-id of specified file
+        from the cache.
+-p::
+--purge=::
+        Purge all cached binaries including older caches which have specified
+	path from the cache.
 -M::
 --missing=::
 	List missing build ids in the cache for the specified file.
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index e7568f5..37182bb 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -223,6 +223,29 @@ static int build_id_cache__remove_file(const char *filename)
 	return err;
 }
 
+static int build_id_cache__purge_path(const char *pathname)
+{
+	struct strlist *list;
+	struct str_node *pos;
+	int err;
+
+	list = build_id_cache__list_build_ids(pathname);
+	if (!list)
+		return 0;
+
+	strlist__for_each(pos, list) {
+		err = build_id_cache__remove_s(pos->s);
+		if (verbose)
+			pr_info("Removing %s %s: %s\n", pos->s, pathname,
+				err ? "FAIL" : "Ok");
+		if (err)
+			break;
+	}
+	strlist__delete(list);
+
+	return err;
+}
+
 static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
 {
 	char filename[PATH_MAX];
@@ -285,6 +308,7 @@ int cmd_buildid_cache(int argc, const char **argv,
 	bool force = false;
 	char const *add_name_list_str = NULL,
 		   *remove_name_list_str = NULL,
+		   *purge_name_list_str = NULL,
 		   *missing_filename = NULL,
 		   *update_name_list_str = NULL,
 		   *kcore_filename = NULL;
@@ -302,6 +326,8 @@ int cmd_buildid_cache(int argc, const char **argv,
 		   "file", "kcore file to add"),
 	OPT_STRING('r', "remove", &remove_name_list_str, "file list",
 		    "file(s) to remove"),
+	OPT_STRING('p', "purge", &purge_name_list_str, "path list",
+		    "path(s) to remove (remove old caches too)"),
 	OPT_STRING('M', "missing", &missing_filename, "file",
 		   "to find missing build ids in the cache"),
 	OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
@@ -368,6 +394,24 @@ int cmd_buildid_cache(int argc, const char **argv,
 		}
 	}
 
+	if (purge_name_list_str) {
+		list = strlist__new(true, purge_name_list_str);
+		if (list) {
+			strlist__for_each(pos, list)
+				if (build_id_cache__purge_path(pos->s)) {
+					if (errno == ENOENT) {
+						pr_debug("%s wasn't in the cache\n",
+							 pos->s);
+						continue;
+					}
+					pr_warning("Couldn't remove %s: %s\n",
+						   pos->s, strerror_r(errno, sbuf, sizeof(sbuf)));
+				}
+
+			strlist__delete(list);
+		}
+	}
+
 	if (missing_filename)
 		ret = build_id_cache__fprintf_missing(session, stdout);
 
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 0bc33be..dcaa964 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -281,35 +281,85 @@ void disable_buildid_cache(void)
 	no_buildid_cache = true;
 }
 
+static char *build_id_cache__dirname_from_path(const char *name,
+					       bool is_kallsyms, bool is_vdso)
+{
+	char *realname = (char *)name, *filename;
+	bool slash = is_kallsyms || is_vdso;
+
+	if (!slash) {
+		realname = realpath(name, NULL);
+		if (!realname)
+			return NULL;
+	}
+
+	if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "",
+		     is_vdso ? DSO__NAME_VDSO : realname) < 0)
+		filename = NULL;
+
+	if (!slash)
+		free(realname);
+
+	return filename;
+}
+
+struct strlist *build_id_cache__list_build_ids(const char *pathname)
+{
+	struct strlist *list;
+	char *dirname;
+	DIR *dir;
+	struct dirent *d;
+
+	list = strlist__new(true, NULL);
+	dirname = build_id_cache__dirname_from_path(pathname, false, false);
+	if (!list || !dirname)
+		goto error_free;
+
+	/* List up all dirents */
+	dir = opendir(dirname);
+	if (!dir)
+		goto error_free;
+	while ((d = readdir(dir)) != NULL) {
+		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+			continue;
+		strlist__add(list, d->d_name);
+	}
+	closedir(dir);
+
+	free(dirname);
+	return list;
+
+error_free:
+	free(dirname);
+	strlist__delete(list);
+	return NULL;
+}
+
 int build_id_cache__add_s(const char *sbuild_id, const char *name,
 			  bool is_kallsyms, bool is_vdso)
 {
 	const size_t size = PATH_MAX;
-	char *realname, *filename = zalloc(size),
+	char *realname = NULL, *filename = NULL, *dirname = NULL,
 	     *linkname = zalloc(size), *targetname, *tmp;
-	int len, err = -1;
-	bool slash = is_kallsyms || is_vdso;
+	int err = -1;
 
-	if (is_kallsyms) {
-		if (symbol_conf.kptr_restrict) {
-			pr_debug("Not caching a kptr_restrict'ed /proc/kallsyms\n");
-			err = 0;
-			goto out_free;
-		}
-		realname = (char *) name;
-	} else
+	if (!is_kallsyms) {
 		realname = realpath(name, NULL);
+		if (!realname)
+			goto out_free;
+	}
 
-	if (realname == NULL || filename == NULL || linkname == NULL)
+	dirname = build_id_cache__dirname_from_path(name, is_kallsyms, is_vdso);
+	if (!dirname)
 		goto out_free;
 
-	len = scnprintf(filename, size, "%s%s%s",
-		       buildid_dir, slash ? "/" : "",
-		       is_vdso ? DSO__NAME_VDSO : realname);
-	if (mkdir_p(filename, 0755))
+	if (mkdir_p(dirname, 0755))
 		goto out_free;
 
-	snprintf(filename + len, size - len, "/%s", sbuild_id);
+	if (asprintf(&filename, "%s/%s", dirname, sbuild_id) < 0) {
+		filename = NULL;
+		goto out_free;
+	}
 
 	if (access(filename, F_OK)) {
 		if (is_kallsyms) {
@@ -337,6 +387,7 @@ out_free:
 	if (!is_kallsyms)
 		free(realname);
 	free(filename);
+	free(dirname);
 	free(linkname);
 	return err;
 }
diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index 2a09498..cbcadea 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -22,6 +22,7 @@ bool perf_session__read_build_ids(struct perf_session *session, bool with_hits);
 int perf_session__write_buildid_table(struct perf_session *session, int fd);
 int perf_session__cache_build_ids(struct perf_session *session);
 
+struct strlist *build_id_cache__list_build_ids(const char *pathname);
 bool build_id_cache__cached(const char *sbuild_id);
 int build_id_cache__add_s(const char *sbuild_id,
 			  const char *name, bool is_kallsyms, bool is_vdso);

[perf/core PATCH v5 3/4] perf-buildid-cache: Use pr_debug instead of verbose && pr_info

[Masami Hiramatsu]

Use pr_debug instead of the combination of verbose and pr_info.

"if (verbose) pr_info(...)" is same as "pr_debug(...)", replace it.

Suggested-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/builtin-buildid-cache.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index 37182bb..20be743 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -196,9 +196,8 @@ static int build_id_cache__add_file(const char *filename)
 	build_id__sprintf(build_id, sizeof(build_id), sbuild_id);
 	err = build_id_cache__add_s(sbuild_id, filename,
 				    false, false);
-	if (verbose)
-		pr_info("Adding %s %s: %s\n", sbuild_id, filename,
-			err ? "FAIL" : "Ok");
+	pr_debug("Adding %s %s: %s\n", sbuild_id, filename,
+		 err ? "FAIL" : "Ok");
 	return err;
 }
 
@@ -216,9 +215,8 @@ static int build_id_cache__remove_file(const char *filename)
 
 	build_id__sprintf(build_id, sizeof(build_id), sbuild_id);
 	err = build_id_cache__remove_s(sbuild_id);
-	if (verbose)
-		pr_info("Removing %s %s: %s\n", sbuild_id, filename,
-			err ? "FAIL" : "Ok");
+	pr_debug("Removing %s %s: %s\n", sbuild_id, filename,
+		 err ? "FAIL" : "Ok");
 
 	return err;
 }
@@ -235,9 +233,8 @@ static int build_id_cache__purge_path(const char *pathname)
 
 	strlist__for_each(pos, list) {
 		err = build_id_cache__remove_s(pos->s);
-		if (verbose)
-			pr_info("Removing %s %s: %s\n", pos->s, pathname,
-				err ? "FAIL" : "Ok");
+		pr_debug("Removing %s %s: %s\n", pos->s, pathname,
+			 err ? "FAIL" : "Ok");
 		if (err)
 			break;
 	}
@@ -292,9 +289,8 @@ static int build_id_cache__update_file(const char *filename)
 	if (!err)
 		err = build_id_cache__add_s(sbuild_id, filename, false, false);
 
-	if (verbose)
-		pr_info("Updating %s %s: %s\n", sbuild_id, filename,
-			err ? "FAIL" : "Ok");
+	pr_debug("Updating %s %s: %s\n", sbuild_id, filename,
+		 err ? "FAIL" : "Ok");
 
 	return err;
 }

[perf/core PATCH v5 4/4] perf-buildid-cache: Show usage with incorrect params

[Masami Hiramatsu]

Show usage if no action is specified or unexpected parameter
is given. In other words, be more user friendly.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/builtin-buildid-cache.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index 20be743..5eaa9bf 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -340,6 +340,11 @@ int cmd_buildid_cache(int argc, const char **argv,
 	argc = parse_options(argc, argv, buildid_cache_options,
 			     buildid_cache_usage, 0);
 
+	if (argc || (!add_name_list_str && !kcore_filename &&
+		     !remove_name_list_str && !purge_name_list_str &&
+		     !missing_filename && !update_name_list_str))
+		usage_with_options(buildid_cache_usage, buildid_cache_options);
+
 	if (missing_filename) {
 		file.path = missing_filename;
 		file.force = force;

Re: [perf/core PATCH v5 2/4] perf buildid-cache: Add --purge FILE to remove all caches of FILE

[Arnaldo Carvalho de Melo]

Em Thu, Feb 26, 2015 at 03:54:47PM +0900, Masami Hiramatsu escreveu:
> @@ -285,6 +308,7 @@ int cmd_buildid_cache(int argc, const char **argv,
>  	bool force = false;
>  	char const *add_name_list_str = NULL,
>  		   *remove_name_list_str = NULL,
> +		   *purge_name_list_str = NULL,
>  		   *missing_filename = NULL,
>  		   *update_name_list_str = NULL,
>  		   *kcore_filename = NULL;
> @@ -302,6 +326,8 @@ int cmd_buildid_cache(int argc, const char **argv,
>  		   "file", "kcore file to add"),
>  	OPT_STRING('r', "remove", &remove_name_list_str, "file list",
>  		    "file(s) to remove"),
> +	OPT_STRING('p', "purge", &purge_name_list_str, "path list",
> +		    "path(s) to remove (remove old caches too)"),
>  	OPT_STRING('M', "missing", &missing_filename, "file",
>  		   "to find missing build ids in the cache"),
>  	OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
> @@ -368,6 +394,24 @@ int cmd_buildid_cache(int argc, const char **argv,
>  		}
>  	}
>  
> +	if (purge_name_list_str) {
> +		list = strlist__new(true, purge_name_list_str);
> +		if (list) {
> +			strlist__for_each(pos, list)
> +				if (build_id_cache__purge_path(pos->s)) {
> +					if (errno == ENOENT) {
> +						pr_debug("%s wasn't in the cache\n",
> +							 pos->s);
> +						continue;
> +					}
> +					pr_warning("Couldn't remove %s: %s\n",
> +						   pos->s, strerror_r(errno, sbuf, sizeof(sbuf)));
> +				}
> +
> +			strlist__delete(list);
> +		}

Here we need to at least warn the user that what he/she asked couldn't
be done, i.e. strlist__new() failed.

Seeing --remove, --purge and --update makes me think eventually we need
a OPT_STRLIST() :-) 

I'm going thru the other patches, the first got in already.

Thanks for implementing --purge!

- Arnaldo

> +	}
> +
>  	if (missing_filename)
>  		ret = build_id_cache__fprintf_missing(session, stdout);
>  
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 0bc33be..dcaa964 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -281,35 +281,85 @@ void disable_buildid_cache(void)
>  	no_buildid_cache = true;
>  }
>  
> +static char *build_id_cache__dirname_from_path(const char *name,
> +					       bool is_kallsyms, bool is_vdso)
> +{
> +	char *realname = (char *)name, *filename;
> +	bool slash = is_kallsyms || is_vdso;
> +
> +	if (!slash) {
> +		realname = realpath(name, NULL);
> +		if (!realname)
> +			return NULL;
> +	}
> +
> +	if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "",
> +		     is_vdso ? DSO__NAME_VDSO : realname) < 0)
> +		filename = NULL;
> +
> +	if (!slash)
> +		free(realname);
> +
> +	return filename;
> +}
> +
> +struct strlist *build_id_cache__list_build_ids(const char *pathname)
> +{
> +	struct strlist *list;
> +	char *dirname;
> +	DIR *dir;
> +	struct dirent *d;
> +
> +	list = strlist__new(true, NULL);
> +	dirname = build_id_cache__dirname_from_path(pathname, false, false);
> +	if (!list || !dirname)
> +		goto error_free;
> +
> +	/* List up all dirents */
> +	dir = opendir(dirname);
> +	if (!dir)
> +		goto error_free;
> +	while ((d = readdir(dir)) != NULL) {
> +		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> +			continue;
> +		strlist__add(list, d->d_name);
> +	}
> +	closedir(dir);
> +
> +	free(dirname);
> +	return list;
> +
> +error_free:
> +	free(dirname);
> +	strlist__delete(list);
> +	return NULL;
> +}
> +
>  int build_id_cache__add_s(const char *sbuild_id, const char *name,
>  			  bool is_kallsyms, bool is_vdso)
>  {
>  	const size_t size = PATH_MAX;
> -	char *realname, *filename = zalloc(size),
> +	char *realname = NULL, *filename = NULL, *dirname = NULL,
>  	     *linkname = zalloc(size), *targetname, *tmp;
> -	int len, err = -1;
> -	bool slash = is_kallsyms || is_vdso;
> +	int err = -1;
>  
> -	if (is_kallsyms) {
> -		if (symbol_conf.kptr_restrict) {
> -			pr_debug("Not caching a kptr_restrict'ed /proc/kallsyms\n");
> -			err = 0;
> -			goto out_free;
> -		}
> -		realname = (char *) name;
> -	} else
> +	if (!is_kallsyms) {
>  		realname = realpath(name, NULL);
> +		if (!realname)
> +			goto out_free;
> +	}
>  
> -	if (realname == NULL || filename == NULL || linkname == NULL)
> +	dirname = build_id_cache__dirname_from_path(name, is_kallsyms, is_vdso);
> +	if (!dirname)
>  		goto out_free;
>  
> -	len = scnprintf(filename, size, "%s%s%s",
> -		       buildid_dir, slash ? "/" : "",
> -		       is_vdso ? DSO__NAME_VDSO : realname);
> -	if (mkdir_p(filename, 0755))
> +	if (mkdir_p(dirname, 0755))
>  		goto out_free;
>  
> -	snprintf(filename + len, size - len, "/%s", sbuild_id);
> +	if (asprintf(&filename, "%s/%s", dirname, sbuild_id) < 0) {
> +		filename = NULL;
> +		goto out_free;
> +	}
>  
>  	if (access(filename, F_OK)) {
>  		if (is_kallsyms) {
> @@ -337,6 +387,7 @@ out_free:
>  	if (!is_kallsyms)
>  		free(realname);
>  	free(filename);
> +	free(dirname);
>  	free(linkname);
>  	return err;
>  }
> diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
> index 2a09498..cbcadea 100644
> --- a/tools/perf/util/build-id.h
> +++ b/tools/perf/util/build-id.h
> @@ -22,6 +22,7 @@ bool perf_session__read_build_ids(struct perf_session *session, bool with_hits);
>  int perf_session__write_buildid_table(struct perf_session *session, int fd);
>  int perf_session__cache_build_ids(struct perf_session *session);
>  
> +struct strlist *build_id_cache__list_build_ids(const char *pathname);
>  bool build_id_cache__cached(const char *sbuild_id);
>  int build_id_cache__add_s(const char *sbuild_id,
>  			  const char *name, bool is_kallsyms, bool is_vdso);
> 

Re: [perf/core PATCH v5 3/4] perf-buildid-cache: Use pr_debug instead of verbose && pr_info

[Arnaldo Carvalho de Melo]

Em Thu, Feb 26, 2015 at 03:54:54PM +0900, Masami Hiramatsu escreveu:
> Use pr_debug instead of the combination of verbose and pr_info.
> 
> "if (verbose) pr_info(...)" is same as "pr_debug(...)", replace it.

This one doesn't apply because 2/4 got delayed.

- Arnaldo

Re: [perf/core PATCH v5 4/4] perf-buildid-cache: Show usage with incorrect params

[Arnaldo Carvalho de Melo]

Em Thu, Feb 26, 2015 at 03:55:01PM +0900, Masami Hiramatsu escreveu:
> Show usage if no action is specified or unexpected parameter
> is given. In other words, be more user friendly.

Thanks for doing that!

Doesn't apply due to previous patches being delayer at the moment.

- Arnaldo
 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  tools/perf/builtin-buildid-cache.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> index 20be743..5eaa9bf 100644
> --- a/tools/perf/builtin-buildid-cache.c
> +++ b/tools/perf/builtin-buildid-cache.c
> @@ -340,6 +340,11 @@ int cmd_buildid_cache(int argc, const char **argv,
>  	argc = parse_options(argc, argv, buildid_cache_options,
>  			     buildid_cache_usage, 0);
>  
> +	if (argc || (!add_name_list_str && !kcore_filename &&
> +		     !remove_name_list_str && !purge_name_list_str &&
> +		     !missing_filename && !update_name_list_str))
> +		usage_with_options(buildid_cache_usage, buildid_cache_options);
> +
>  	if (missing_filename) {
>  		file.path = missing_filename;
>  		file.force = force;
> 

Re: Re: [perf/core PATCH v5 2/4] perf buildid-cache: Add --purge FILE to remove all caches of FILE

[Masami Hiramatsu]

(2015/02/27 0:05), Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 26, 2015 at 03:54:47PM +0900, Masami Hiramatsu escreveu:
>> @@ -285,6 +308,7 @@ int cmd_buildid_cache(int argc, const char **argv,
>>  	bool force = false;
>>  	char const *add_name_list_str = NULL,
>>  		   *remove_name_list_str = NULL,
>> +		   *purge_name_list_str = NULL,
>>  		   *missing_filename = NULL,
>>  		   *update_name_list_str = NULL,
>>  		   *kcore_filename = NULL;
>> @@ -302,6 +326,8 @@ int cmd_buildid_cache(int argc, const char **argv,
>>  		   "file", "kcore file to add"),
>>  	OPT_STRING('r', "remove", &remove_name_list_str, "file list",
>>  		    "file(s) to remove"),
>> +	OPT_STRING('p', "purge", &purge_name_list_str, "path list",
>> +		    "path(s) to remove (remove old caches too)"),
>>  	OPT_STRING('M', "missing", &missing_filename, "file",
>>  		   "to find missing build ids in the cache"),
>>  	OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
>> @@ -368,6 +394,24 @@ int cmd_buildid_cache(int argc, const char **argv,
>>  		}
>>  	}
>>  
>> +	if (purge_name_list_str) {
>> +		list = strlist__new(true, purge_name_list_str);
>> +		if (list) {
>> +			strlist__for_each(pos, list)
>> +				if (build_id_cache__purge_path(pos->s)) {
>> +					if (errno == ENOENT) {
>> +						pr_debug("%s wasn't in the cache\n",
>> +							 pos->s);
>> +						continue;
>> +					}
>> +					pr_warning("Couldn't remove %s: %s\n",
>> +						   pos->s, strerror_r(errno, sbuf, sizeof(sbuf)));
>> +				}
>> +
>> +			strlist__delete(list);
>> +		}
> 
> Here we need to at least warn the user that what he/she asked couldn't
> be done, i.e. strlist__new() failed.

Ah, OK I'll update it.

> 
> Seeing --remove, --purge and --update makes me think eventually we need
> a OPT_STRLIST() :-) 

Agreed :)

> 
> I'm going thru the other patches, the first got in already.

Thanks!

> 
> Thanks for implementing --purge!
> 
> - Arnaldo
> 
>> +	}
>> +
>>  	if (missing_filename)
>>  		ret = build_id_cache__fprintf_missing(session, stdout);
>>  
>> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
>> index 0bc33be..dcaa964 100644
>> --- a/tools/perf/util/build-id.c
>> +++ b/tools/perf/util/build-id.c
>> @@ -281,35 +281,85 @@ void disable_buildid_cache(void)
>>  	no_buildid_cache = true;
>>  }
>>  
>> +static char *build_id_cache__dirname_from_path(const char *name,
>> +					       bool is_kallsyms, bool is_vdso)
>> +{
>> +	char *realname = (char *)name, *filename;
>> +	bool slash = is_kallsyms || is_vdso;
>> +
>> +	if (!slash) {
>> +		realname = realpath(name, NULL);
>> +		if (!realname)
>> +			return NULL;
>> +	}
>> +
>> +	if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "",
>> +		     is_vdso ? DSO__NAME_VDSO : realname) < 0)
>> +		filename = NULL;
>> +
>> +	if (!slash)
>> +		free(realname);
>> +
>> +	return filename;
>> +}
>> +
>> +struct strlist *build_id_cache__list_build_ids(const char *pathname)
>> +{
>> +	struct strlist *list;
>> +	char *dirname;
>> +	DIR *dir;
>> +	struct dirent *d;
>> +
>> +	list = strlist__new(true, NULL);
>> +	dirname = build_id_cache__dirname_from_path(pathname, false, false);
>> +	if (!list || !dirname)
>> +		goto error_free;
>> +
>> +	/* List up all dirents */
>> +	dir = opendir(dirname);
>> +	if (!dir)
>> +		goto error_free;
>> +	while ((d = readdir(dir)) != NULL) {
>> +		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
>> +			continue;
>> +		strlist__add(list, d->d_name);
>> +	}
>> +	closedir(dir);
>> +
>> +	free(dirname);
>> +	return list;
>> +
>> +error_free:
>> +	free(dirname);
>> +	strlist__delete(list);
>> +	return NULL;
>> +}
>> +
>>  int build_id_cache__add_s(const char *sbuild_id, const char *name,
>>  			  bool is_kallsyms, bool is_vdso)
>>  {
>>  	const size_t size = PATH_MAX;
>> -	char *realname, *filename = zalloc(size),
>> +	char *realname = NULL, *filename = NULL, *dirname = NULL,
>>  	     *linkname = zalloc(size), *targetname, *tmp;
>> -	int len, err = -1;
>> -	bool slash = is_kallsyms || is_vdso;
>> +	int err = -1;
>>  
>> -	if (is_kallsyms) {
>> -		if (symbol_conf.kptr_restrict) {
>> -			pr_debug("Not caching a kptr_restrict'ed /proc/kallsyms\n");
>> -			err = 0;
>> -			goto out_free;
>> -		}
>> -		realname = (char *) name;
>> -	} else
>> +	if (!is_kallsyms) {
>>  		realname = realpath(name, NULL);
>> +		if (!realname)
>> +			goto out_free;
>> +	}
>>  
>> -	if (realname == NULL || filename == NULL || linkname == NULL)
>> +	dirname = build_id_cache__dirname_from_path(name, is_kallsyms, is_vdso);
>> +	if (!dirname)
>>  		goto out_free;
>>  
>> -	len = scnprintf(filename, size, "%s%s%s",
>> -		       buildid_dir, slash ? "/" : "",
>> -		       is_vdso ? DSO__NAME_VDSO : realname);
>> -	if (mkdir_p(filename, 0755))
>> +	if (mkdir_p(dirname, 0755))
>>  		goto out_free;
>>  
>> -	snprintf(filename + len, size - len, "/%s", sbuild_id);
>> +	if (asprintf(&filename, "%s/%s", dirname, sbuild_id) < 0) {
>> +		filename = NULL;
>> +		goto out_free;
>> +	}
>>  
>>  	if (access(filename, F_OK)) {
>>  		if (is_kallsyms) {
>> @@ -337,6 +387,7 @@ out_free:
>>  	if (!is_kallsyms)
>>  		free(realname);
>>  	free(filename);
>> +	free(dirname);
>>  	free(linkname);
>>  	return err;
>>  }
>> diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
>> index 2a09498..cbcadea 100644
>> --- a/tools/perf/util/build-id.h
>> +++ b/tools/perf/util/build-id.h
>> @@ -22,6 +22,7 @@ bool perf_session__read_build_ids(struct perf_session *session, bool with_hits);
>>  int perf_session__write_buildid_table(struct perf_session *session, int fd);
>>  int perf_session__cache_build_ids(struct perf_session *session);
>>  
>> +struct strlist *build_id_cache__list_build_ids(const char *pathname);
>>  bool build_id_cache__cached(const char *sbuild_id);
>>  int build_id_cache__add_s(const char *sbuild_id,
>>  			  const char *name, bool is_kallsyms, bool is_vdso);
>>
> --
> 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/
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[tip:perf/core] perf buildid-cache: Add new buildid cache if update target is not cached

[tip-bot for Masami Hiramatsu]

Commit-ID:  a50d11a10c2db86d7383c281d4e249d5393661e9
Gitweb:     http://git.kernel.org/tip/a50d11a10c2db86d7383c281d4e249d5393661e9
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Thu, 26 Feb 2015 15:54:40 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 27 Feb 2015 10:08:37 -0300

perf buildid-cache: Add new buildid cache if update target is not cached

Add new buildid cache if the update target file is not cached.

This can happen when an old binary is replaced by new one after caching
the old one. In this case, user sees his operation just failed.

But it does not look straight, since user just pass the binary "path",
not "build-id".

  ----
  # ./perf buildid-cache --add ./perf
  (update ./perf to new binary)
  # ./perf buildid-cache --update ./perf
  ./perf wasn't in the cache
  #
  ----

This patch adds given new binary to cache if the new binary is
not cached. So we'll not see the above error.

  ----
  # ./perf buildid-cache --add ./perf
  (update ./perf to new binary)
  # ./perf buildid-cache --update ./perf
  #
  ----

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Hemant Kumar <hemant@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20150226065440.23912.1494.stgit@localhost.localdomain
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-buildid-cache.txt | 11 ++++++++---
 tools/perf/builtin-buildid-cache.c              |  6 ++++--
 tools/perf/util/build-id.c                      | 12 ++++++++++++
 tools/perf/util/build-id.h                      |  1 +
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/tools/perf/Documentation/perf-buildid-cache.txt b/tools/perf/Documentation/perf-buildid-cache.txt
index 0294c57..cec6b57 100644
--- a/tools/perf/Documentation/perf-buildid-cache.txt
+++ b/tools/perf/Documentation/perf-buildid-cache.txt
@@ -41,9 +41,14 @@ OPTIONS
 --missing=::
 	List missing build ids in the cache for the specified file.
 -u::
---update::
-	Update specified file of the cache. It can be used to update kallsyms
-	kernel dso to vmlinux in order to support annotation.
+--update=::
+	Update specified file of the cache. Note that this doesn't remove
+	older entires since those may be still needed for annotating old
+	(or remote) perf.data. Only if there is already a cache which has
+	exactly same build-id, that is replaced by new one. It can be used
+	to update kallsyms and kernel dso to vmlinux in order to support
+	annotation.
+
 -v::
 --verbose::
 	Be more verbose.
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index d929d95..e7568f5 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -255,7 +255,7 @@ static int build_id_cache__update_file(const char *filename)
 	u8 build_id[BUILD_ID_SIZE];
 	char sbuild_id[BUILD_ID_SIZE * 2 + 1];
 
-	int err;
+	int err = 0;
 
 	if (filename__read_build_id(filename, &build_id, sizeof(build_id)) < 0) {
 		pr_debug("Couldn't read a build-id in %s\n", filename);
@@ -263,7 +263,9 @@ static int build_id_cache__update_file(const char *filename)
 	}
 
 	build_id__sprintf(build_id, sizeof(build_id), sbuild_id);
-	err = build_id_cache__remove_s(sbuild_id);
+	if (build_id_cache__cached(sbuild_id))
+		err = build_id_cache__remove_s(sbuild_id);
+
 	if (!err)
 		err = build_id_cache__add_s(sbuild_id, filename, false, false);
 
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index adbc360..0bc33be 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -352,6 +352,18 @@ static int build_id_cache__add_b(const u8 *build_id, size_t build_id_size,
 	return build_id_cache__add_s(sbuild_id, name, is_kallsyms, is_vdso);
 }
 
+bool build_id_cache__cached(const char *sbuild_id)
+{
+	bool ret = false;
+	char *filename = build_id__filename(sbuild_id, NULL, 0);
+
+	if (filename && !access(filename, F_OK))
+		ret = true;
+	free(filename);
+
+	return ret;
+}
+
 int build_id_cache__remove_s(const char *sbuild_id)
 {
 	const size_t size = PATH_MAX;
diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index 31b3c63..2a09498 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -22,6 +22,7 @@ bool perf_session__read_build_ids(struct perf_session *session, bool with_hits);
 int perf_session__write_buildid_table(struct perf_session *session, int fd);
 int perf_session__cache_build_ids(struct perf_session *session);
 
+bool build_id_cache__cached(const char *sbuild_id);
 int build_id_cache__add_s(const char *sbuild_id,
 			  const char *name, bool is_kallsyms, bool is_vdso);
 int build_id_cache__remove_s(const char *sbuild_id);