perf-probe --cache and SDT support

Introduce perf_cache object and interfaces to create, add entry, commit, and delete the object. perf_cache represents a file for the cached perf-probe definitions on one binary file or vmlinux which has its own build id. The probe cache file is located under the build-id cache directory of the target binary, as below;

<perf-debug-dir>/.build-id/<BU>/<ILDID>/probe

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v10:
- Splited from "Add --cache option to cache the probe definitions"

---
 tools/perf/util/probe-file.c |  307 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/probe-file.h |   19 +++
 2 files changed, 326 insertions(+)

tools/perf/util/probe-file.c +1 -0

index 3fe6214..689d874 100644
@@ -14,6 +14,7 @@
  * GNU General Public License for more details.
  *
  */
+#include <sys/uio.h>
 #include "util.h"
 #include "event.h"
 #include "strlist.h"

tools/perf/util/probe-file.c +69 -0

+	free(node);
+}
+
+static struct probe_cache_entry *
+probe_cache_entry__new(struct perf_probe_event *pev)
+{
+	struct probe_cache_entry *ret = zalloc(sizeof(*ret));
+
+	if (ret) {
+		INIT_LIST_HEAD(&ret->list);
+		ret->tevlist = strlist__new(NULL, NULL);
+		if (!ret->tevlist)
+			zfree(&ret);
+		if (ret && pev) {
+			ret->spev = synthesize_perf_probe_command(pev);
+			if (!ret->spev ||
+			    perf_probe_event__copy(&ret->pev, pev) < 0) {
+				probe_cache_entry__delete(ret);
+				return NULL;
+			}
+		}
+	}
+
+	return ret;
+}
+
+/* For the kernel probe caches, pass target = NULL */
+static int probe_cache__open(struct probe_cache *pcache, const char *target)
+{
+	char cpath[PATH_MAX];
+	char sbuildid[SBUILD_ID_SIZE];
+	char *dir_name;
+	bool is_kallsyms = !target;
+	int ret, fd;
+
+	if (target)
+		ret = filename__sprintf_build_id(target, sbuildid);
+	else {
+		target = DSO__NAME_KALLSYMS;
+		ret = sysfs__sprintf_build_id("/", sbuildid);
+	}
+	if (ret < 0) {
+		pr_debug("Failed to get build-id from %s.\n", target);
+		return ret;
+	}
+
+	/* If we have no buildid cache, make it */
+	if (!build_id_cache__cached(sbuildid)) {
+		ret = build_id_cache__add_s(sbuildid, target,
+					    is_kallsyms, NULL);
+		if (ret < 0) {
+			pr_debug("Failed to add build-id cache: %s\n", target);
+			return ret;
+		}
+	}
+
+	dir_name = build_id_cache__cachedir(sbuildid, target, is_kallsyms,
+					    false);
+	if (!dir_name)
+		return -ENOMEM;
+
+	snprintf(cpath, PATH_MAX, "%s/probes", dir_name);
+	fd = open(cpath, O_CREAT | O_RDWR, 0644);
+	if (fd < 0)
+		pr_debug("Failed to open cache(%d): %s\n", fd, cpath);
+	free(dir_name);
+	pcache->fd = fd;
+
+	return fd;

tools/perf/util/probe-file.c +43 -0

+	while (!feof(fp)) {
+		if (!fgets(buf, MAX_CMDLEN, fp))
+			break;
+		p = strchr(buf, '\n');
+		if (p)
+			*p = '\0';
+		if (buf[0] == '#') {	/* #perf_probe_event */
+			entry = probe_cache_entry__new(NULL);
+			if (!entry) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			entry->spev = strdup(buf + 1);
+			if (entry->spev)
+				ret = parse_perf_probe_command(buf + 1,
+								&entry->pev);
+			else
+				ret = -ENOMEM;
+			if (ret < 0) {
+				probe_cache_entry__delete(entry);
+				goto out;
+			}
+			list_add_tail(&entry->list, &pcache->list);
+		} else {	/* trace_probe_event */
+			if (!entry) {
+				ret = -EINVAL;
+				goto out;
+			}
+			strlist__add(entry->tevlist, buf);
+		}
+	}
+out:
+	fclose(fp);
+	return ret;
+}
+
+static struct probe_cache *probe_cache__alloc(void)
+{
+	struct probe_cache *ret = zalloc(sizeof(*ret));
+
+	if (ret) {
+		INIT_LIST_HEAD(&ret->list);
+		ret->fd = -EINVAL;

struct probe_cache_entry {
	struct list_head	list;

	struct perf_probe_event pev;
	char			*spev;
	struct strlist		*tevlist;
};

struct probe_cache {
	int	fd;
	struct list_head list;

tools/perf/util/probe-file.c +31 -0

+
+	if (!pcache)
+		return NULL;
+
+	ret = probe_cache__open(pcache, target);
+	if (ret < 0) {
+		pr_debug("Cache open error: %d\n", ret);
+		goto out_err;
+	}
+
+	ret = probe_cache__load(pcache);
+	if (ret < 0) {
+		pr_debug("Cache read error: %d\n", ret);
+		goto out_err;
+	}
+
+	return pcache;
+
+out_err:
+	probe_cache__delete(pcache);
+	return NULL;
+}
+
+static bool streql(const char *a, const char *b)
+{
+	if (a == b)
+		return true;
+
+	if (!a || !b)
+		return false;
+

tools/perf/util/probe-file.c +25 -0

+	if (!cmd)
+		return NULL;
+
+	list_for_each_entry(entry, &pcache->list, list) {
+		/* Hit if same event name or same command-string */
+		if ((pev->event &&
+		     (streql(entry->pev.group, pev->group) &&
+		      streql(entry->pev.event, pev->event))) ||
+		    (!strcmp(entry->spev, cmd)))
+			goto found;
+	}
+	entry = NULL;
+
+found:
+	free(cmd);
+	return entry;
+}
+
+int probe_cache__add_entry(struct probe_cache *pcache,
+			   struct perf_probe_event *pev,
+			   struct probe_trace_event *tevs, int ntevs)
+{
+	struct probe_cache_entry *entry = NULL;
+	char *command;
+	int i, ret = 0;

tools/perf/util/probe-file.c +14 -0

+
+	ret = -ENOMEM;
+	entry = probe_cache_entry__new(pev);
+	if (!entry)
+		goto out_err;
+
+	for (i = 0; i < ntevs; i++) {
+		if (!tevs[i].point.symbol)
+			continue;
+
+		command = synthesize_probe_trace_command(&tevs[i]);
+		if (!command)
+			goto out_err;
+		strlist__add(entry->tevlist, command);

tools/perf/util/probe-file.c +6 -0

+	return ret;
+}
+
+static int probe_cache_entry__write(struct probe_cache_entry *entry, int fd)
+{
+	struct str_node *snode;

tools/perf/util/probe-file.c +35 -0

+
+	strlist__for_each(snode, entry->tevlist) {
+		iov[0].iov_base = (void *)snode->s;
+		iov[0].iov_len = strlen(snode->s);
+		iov[1].iov_base = (void *)"\n"; iov[1].iov_len = 1;
+		ret = writev(fd, iov, 2);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
+int probe_cache__commit(struct probe_cache *pcache)
+{
+	struct probe_cache_entry *entry;
+	int ret = 0;
+
+	/* TBD: if we do not update existing entries, skip it */
+	ret = lseek(pcache->fd, 0, SEEK_SET);
+	if (ret < 0)
+		goto out;
+
+	ret = ftruncate(pcache->fd, 0);
+	if (ret < 0)
+		goto out;
+
+	list_for_each_entry(entry, &pcache->list, list) {
+		ret = probe_cache_entry__write(entry, pcache->fd);
+		pr_debug("Cache committed: %d\n", ret);
+		if (ret < 0)
+			break;
+	}
+out:
+	return ret;
+}

tools/perf/util/probe-file.h +19 -0

index 18ac9cf..d2b8791d 100644
@@ -5,6 +5,19 @@
 #include "strfilter.h"
 #include "probe-event.h"
 
+/* Cache of probe definitions */
+struct probe_cache_entry {
+	struct list_head	list;
+	struct perf_probe_event pev;
+	char			*spev;
+	struct strlist		*tevlist;
+};
+
+struct probe_cache {
+	int	fd;
+	struct list_head list;
+};
+
 #define PF_FL_UPROBE	1
 #define PF_FL_RW	2
 

@@ -18,5 +31,11 @@ int probe_file__get_events(int fd, struct strfilter *filter,
 				  struct strlist *plist);
 int probe_file__del_strlist(int fd, struct strlist *namelist);
 
+struct probe_cache *probe_cache__new(const char *target);
+int probe_cache__add_entry(struct probe_cache *pcache,
+			   struct perf_probe_event *pev,
+			   struct probe_trace_event *tevs, int ntevs);
+int probe_cache__commit(struct probe_cache *pcache);
+void probe_cache__delete(struct probe_cache *pcache);
 
 #endif

Em Wed, Jun 08, 2016 at 06:29:59PM +0900, Masami Hiramatsu escreveu:

Quoted 31 lines

Introduce perf_cache object and interfaces to create, add entry, commit, and delete the object. perf_cache represents a file for the cached perf-probe definitions on one binary file or vmlinux which has its own build id. The probe cache file is located under the build-id cache directory of the target binary, as below;

<perf-debug-dir>/.build-id/<BU>/<ILDID>/probe

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v10:
- Splited from "Add --cache option to cache the probe definitions"

---
 tools/perf/util/probe-file.c |  307 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/probe-file.h |   19 +++
 2 files changed, 326 insertions(+)

tools/perf/util/probe-file.c +1 -0

index 3fe6214..689d874 100644
@@ -14,6 +14,7 @@
  * GNU General Public License for more details.
  *
  */
+#include <sys/uio.h>
 #include "util.h"
 #include "event.h"
 #include "strlist.h"

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 332

Quoted 8 lines

tools/perf/util/probe-file.c +5 -0

@@ -324,3 +325,309 @@ int probe_file__del_events(int fd, struct strfilter *filter)
 
 	return ret;
 }
+
+static void probe_cache_entry__delete(struct probe_cache_entry *node)
+{
+	if (!list_empty(&node->list))
+		list_del(&node->list);

Humm, shouldn't this be something like:

BUG_ON(!list_empty(&node->list)

?

I.e. whoever inserted this on a list should take care of removing it before deleting it, taking locks, whatever is needed to keep the integrity of such list.

You may not be using this stuff in a multithreaded app now, but lets not make it difficult to :-)

I noticed why you do it this way and have a suggestion to use a more usual model, see below.

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 334

Quoted 2 lines

tools/perf/util/probe-file.c +2 -0

+	if (node->tevlist)
+		strlist__delete(node->tevlist);

No checking, destructors generally follows the free() model, i.e. they eat NULL for breakfast. Lemme see if strlist does that... Yes, they do.

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 336

Quoted 3 lines

tools/perf/util/probe-file.c +2 -0

+	clear_perf_probe_event(&node->pev);
+	free(node->spev);

Here you may want to use:

	zfree(&node->spev);

To free and set node->spev to NULL, to help in debugging when this node may be still referenced even having being deleted.

Quoted 74 lines

tools/perf/util/probe-file.c +73 -0

+	free(node);
+}
+
+static struct probe_cache_entry *
+probe_cache_entry__new(struct perf_probe_event *pev)
+{
+	struct probe_cache_entry *ret = zalloc(sizeof(*ret));
+
+	if (ret) {
+		INIT_LIST_HEAD(&ret->list);
+		ret->tevlist = strlist__new(NULL, NULL);
+		if (!ret->tevlist)
+			zfree(&ret);
+		if (ret && pev) {
+			ret->spev = synthesize_perf_probe_command(pev);
+			if (!ret->spev ||
+			    perf_probe_event__copy(&ret->pev, pev) < 0) {
+				probe_cache_entry__delete(ret);
+				return NULL;
+			}
+		}
+	}
+
+	return ret;
+}
+
+/* For the kernel probe caches, pass target = NULL */
+static int probe_cache__open(struct probe_cache *pcache, const char *target)
+{
+	char cpath[PATH_MAX];
+	char sbuildid[SBUILD_ID_SIZE];
+	char *dir_name;
+	bool is_kallsyms = !target;
+	int ret, fd;
+
+	if (target)
+		ret = filename__sprintf_build_id(target, sbuildid);
+	else {
+		target = DSO__NAME_KALLSYMS;
+		ret = sysfs__sprintf_build_id("/", sbuildid);
+	}
+	if (ret < 0) {
+		pr_debug("Failed to get build-id from %s.\n", target);
+		return ret;
+	}
+
+	/* If we have no buildid cache, make it */
+	if (!build_id_cache__cached(sbuildid)) {
+		ret = build_id_cache__add_s(sbuildid, target,
+					    is_kallsyms, NULL);
+		if (ret < 0) {
+			pr_debug("Failed to add build-id cache: %s\n", target);
+			return ret;
+		}
+	}
+
+	dir_name = build_id_cache__cachedir(sbuildid, target, is_kallsyms,
+					    false);
+	if (!dir_name)
+		return -ENOMEM;
+
+	snprintf(cpath, PATH_MAX, "%s/probes", dir_name);
+	fd = open(cpath, O_CREAT | O_RDWR, 0644);
+	if (fd < 0)
+		pr_debug("Failed to open cache(%d): %s\n", fd, cpath);
+	free(dir_name);
+	pcache->fd = fd;
+
+	return fd;
+}
+
+static int probe_cache__load(struct probe_cache *pcache)
+{

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 415

Quoted 6 lines

tools/perf/util/probe-file.c +6 -0

+	struct probe_cache_entry *entry = NULL;
+	char buf[MAX_CMDLEN], *p;
+	int ret = 0;
+	FILE *fp;
+
+	fp = fdopen(dup(pcache->fd), "r");

fdopen may return NULL, a check is needed.

Quoted 44 lines

tools/perf/util/probe-file.c +43 -0

+	while (!feof(fp)) {
+		if (!fgets(buf, MAX_CMDLEN, fp))
+			break;
+		p = strchr(buf, '\n');
+		if (p)
+			*p = '\0';
+		if (buf[0] == '#') {	/* #perf_probe_event */
+			entry = probe_cache_entry__new(NULL);
+			if (!entry) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			entry->spev = strdup(buf + 1);
+			if (entry->spev)
+				ret = parse_perf_probe_command(buf + 1,
+								&entry->pev);
+			else
+				ret = -ENOMEM;
+			if (ret < 0) {
+				probe_cache_entry__delete(entry);
+				goto out;
+			}
+			list_add_tail(&entry->list, &pcache->list);
+		} else {	/* trace_probe_event */
+			if (!entry) {
+				ret = -EINVAL;
+				goto out;
+			}
+			strlist__add(entry->tevlist, buf);
+		}
+	}
+out:
+	fclose(fp);
+	return ret;
+}
+
+static struct probe_cache *probe_cache__alloc(void)
+{
+	struct probe_cache *ret = zalloc(sizeof(*ret));
+
+	if (ret) {
+		INIT_LIST_HEAD(&ret->list);
+		ret->fd = -EINVAL;

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 468

Quoted 10 lines

tools/perf/util/probe-file.c +10 -0

+	}
+	return ret;
+}
+
+void probe_cache__delete(struct probe_cache *pcache)
+{
+	struct probe_cache_entry *entry;
+
+	if (!pcache)
+		return;

see, a good destructor, accepts NULL, does nothing with it.

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 473

Quoted 5 lines

tools/perf/util/probe-file.c +5 -0

+
+	while (!list_empty(&pcache->list)) {
+		entry = list_first_entry(&pcache->list, typeof(*entry), list);
+		probe_cache_entry__delete(entry);
+	}

the above while is the definition of a "purge()" operation, that may be useful outside of a delete operation, please consider adding it, like:

void probe_cache__purge(struct probe_cache *pcache)

{
	struct probe_cache_entry *entry, *n;
	list_for_each_entry_safe(entry, n, &pcache->list, node)
		probe_cache_entry__delete(entry);
}

And please use 'list' for lists and 'node' for entries in a list, i.e. you have, at the end of this patch:

Quoted 2 lines

struct probe_cache_entry {
	struct list_head	list;

This one should be 'node', not list, this way we know that this is an entry in a list, not a list of some other structs.

Quoted 5 lines

	struct perf_probe_event pev;
	char			*spev;
	struct strlist		*tevlist;
};

Quoted 3 lines

struct probe_cache {
	int	fd;
	struct list_head list;

This one is ok, but then if you rename it from the generic name 'list' to something more informative, for instance 'cache_entries', I think it will help people reading your code to grasp what it is doing more quickly.

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 482

Quoted 12 lines

};

tools/perf/util/probe-file.c +9 -0

+	if (pcache->fd > 0)
+		close(pcache->fd);
+	free(pcache);
+}
+
+struct probe_cache *probe_cache__new(const char *target)
+{
+	struct probe_cache *pcache = probe_cache__alloc();
+	int ret;

This is odd, what you call "probe_cache__alloc() looks like what a "new()" does, if you think that the constructor for 'probe_cache' has to always open and load it, then why not just do the zalloc() here and then call probe_cache__init() on it?

Quoted 31 lines

tools/perf/util/probe-file.c +31 -0

+
+	if (!pcache)
+		return NULL;
+
+	ret = probe_cache__open(pcache, target);
+	if (ret < 0) {
+		pr_debug("Cache open error: %d\n", ret);
+		goto out_err;
+	}
+
+	ret = probe_cache__load(pcache);
+	if (ret < 0) {
+		pr_debug("Cache read error: %d\n", ret);
+		goto out_err;
+	}
+
+	return pcache;
+
+out_err:
+	probe_cache__delete(pcache);
+	return NULL;
+}
+
+static bool streql(const char *a, const char *b)
+{
+	if (a == b)
+		return true;
+
+	if (!a || !b)
+		return false;
+

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 523

Quoted 10 lines

tools/perf/util/probe-file.c +10 -0

+	return !strcmp(a, b);
+}
+
+static struct probe_cache_entry *
+probe_cache__find(struct probe_cache *pcache, struct perf_probe_event *pev)
+{
+	struct probe_cache_entry *entry = NULL;
+	char *cmd = NULL;
+
+	cmd = synthesize_perf_probe_command(pev);

Why init it to NULL only to immediately init it again to something else? Perhaps:

	char *cmd = synthesize_perf_probe_command(pev);

instead?

Quoted 26 lines

tools/perf/util/probe-file.c +25 -0

+	if (!cmd)
+		return NULL;
+
+	list_for_each_entry(entry, &pcache->list, list) {
+		/* Hit if same event name or same command-string */
+		if ((pev->event &&
+		     (streql(entry->pev.group, pev->group) &&
+		      streql(entry->pev.event, pev->event))) ||
+		    (!strcmp(entry->spev, cmd)))
+			goto found;
+	}
+	entry = NULL;
+
+found:
+	free(cmd);
+	return entry;
+}
+
+int probe_cache__add_entry(struct probe_cache *pcache,
+			   struct perf_probe_event *pev,
+			   struct probe_trace_event *tevs, int ntevs)
+{
+	struct probe_cache_entry *entry = NULL;
+	char *command;
+	int i, ret = 0;

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 558

Quoted 10 lines

tools/perf/util/probe-file.c +10 -0

+
+	if (!pcache || !pev || !tevs || ntevs <= 0) {
+		ret = -EINVAL;
+		goto out_err;
+	}
+
+	/* Remove old cache entry */
+	entry = probe_cache__find(pcache, pev);
+	if (entry)
+		probe_cache_entry__delete(entry);

Here you could be more compact with:

	probe_cache_entry__delete(probe_cache__find(pcache, pev));

Because delete() functions accept NULL?

Quoted 19 lines

tools/perf/util/probe-file.c +18 -0

+
+	ret = -ENOMEM;
+	entry = probe_cache_entry__new(pev);
+	if (!entry)
+		goto out_err;
+
+	for (i = 0; i < ntevs; i++) {
+		if (!tevs[i].point.symbol)
+			continue;
+
+		command = synthesize_probe_trace_command(&tevs[i]);
+		if (!command)
+			goto out_err;
+		strlist__add(entry->tevlist, command);
+		free(command);
+	}
+	list_add_tail(&entry->list, &pcache->list);
+	pr_debug("Added probe cache: %d\n", ntevs);

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 582

Quoted 6 lines

tools/perf/util/probe-file.c +6 -0

+	return 0;
+
+out_err:
+	pr_debug("Failed to add probe caches\n");
+	if (entry)
+		probe_cache_entry__delete(entry);

No need to check for NULL, call the destructor directly.

Quoted 12 lines

tools/perf/util/probe-file.c +11 -0

+	return ret;
+}
+
+static int probe_cache_entry__write(struct probe_cache_entry *entry, int fd)
+{
+	struct str_node *snode;
+	struct iovec iov[3];
+	int ret;
+
+	pr_debug("Writing cache: #%s\n", entry->spev);
+	iov[0].iov_base = (void *)"#"; iov[0].iov_len = 1;

tools/perf/util/probe-file.c int probe_file__del_events(int fd, struct strfilter *filter) · line 598

Quoted 5 lines

tools/perf/util/probe-file.c +5 -0

+	iov[1].iov_base = entry->spev; iov[1].iov_len = strlen(entry->spev);
+	iov[2].iov_base = (void *)"\n"; iov[2].iov_len = 1;
+	ret = writev(fd, iov, 3);
+	if (ret < 0)
+		return ret;

Shouldn't we check short writes? writev() returns the number of bytes written, isn't it possible to return less than what you asked for?

Quoted 72 lines

tools/perf/util/probe-file.c +35 -0

+
+	strlist__for_each(snode, entry->tevlist) {
+		iov[0].iov_base = (void *)snode->s;
+		iov[0].iov_len = strlen(snode->s);
+		iov[1].iov_base = (void *)"\n"; iov[1].iov_len = 1;
+		ret = writev(fd, iov, 2);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
+int probe_cache__commit(struct probe_cache *pcache)
+{
+	struct probe_cache_entry *entry;
+	int ret = 0;
+
+	/* TBD: if we do not update existing entries, skip it */
+	ret = lseek(pcache->fd, 0, SEEK_SET);
+	if (ret < 0)
+		goto out;
+
+	ret = ftruncate(pcache->fd, 0);
+	if (ret < 0)
+		goto out;
+
+	list_for_each_entry(entry, &pcache->list, list) {
+		ret = probe_cache_entry__write(entry, pcache->fd);
+		pr_debug("Cache committed: %d\n", ret);
+		if (ret < 0)
+			break;
+	}
+out:
+	return ret;
+}

tools/perf/util/probe-file.h +19 -0

index 18ac9cf..d2b8791d 100644
@@ -5,6 +5,19 @@
 #include "strfilter.h"
 #include "probe-event.h"
 
+/* Cache of probe definitions */
+struct probe_cache_entry {
+	struct list_head	list;
+	struct perf_probe_event pev;
+	char			*spev;
+	struct strlist		*tevlist;
+};
+
+struct probe_cache {
+	int	fd;
+	struct list_head list;
+};
+
 #define PF_FL_UPROBE	1
 #define PF_FL_RW	2
 

@@ -18,5 +31,11 @@ int probe_file__get_events(int fd, struct strfilter *filter,
 				  struct strlist *plist);
 int probe_file__del_strlist(int fd, struct strlist *namelist);
 
+struct probe_cache *probe_cache__new(const char *target);
+int probe_cache__add_entry(struct probe_cache *pcache,
+			   struct perf_probe_event *pev,
+			   struct probe_trace_event *tevs, int ntevs);
+int probe_cache__commit(struct probe_cache *pcache);
+void probe_cache__delete(struct probe_cache *pcache);
 
 #endif

tools/perf/util/probe-event.c +1 -0

+			probe_cache__delete(cache);

Em Wed, Jun 08, 2016 at 06:30:10PM +0900, Masami Hiramatsu escreveu:

tools/perf/util/probe-event.c static int __add_probe_trace_events(struct perf_probe_event *pev, · line 2563

Quoted 9 lines

tools/perf/util/probe-event.c +5 -0

@@ -2555,6 +2556,14 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 	}
 	if (ret == -EINVAL && pev->uprobes)
 		warn_uprobe_event_compat(tev);
+	if (ret == 0 && probe_conf.cache) {
+		cache = probe_cache__new(pev->target);
+		if (cache) {
+			probe_cache__add_entry(cache, pev, tevs, ntevs);
+			probe_cache__commit(cache);

These two functions may fail, please check its return and forward errors appropriately.

tools/perf/util/build-id.c +14 -0

+char *build_id_cache__origname(const char *sbuild_id)
+{
+	char *linkname;
+	char buf[PATH_MAX];
+	char *ret = NULL, *p;
+	size_t offs = 5;	/* == strlen("../..") */
+
+	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
+	if (!linkname)
+		return NULL;
+
+	if (readlink(linkname, buf, PATH_MAX) < 0)
+		goto out;
+	/* The link should be "../..<origpath>/<sbuild_id>" */

tools/perf/util/build-id.c +7 -0

+	}
+out:
+	free(linkname);
+	return ret;
+}
+
 static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
 {
 	return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");

@@ -390,6 +419,59 @@ void disable_buildid_cache(void)
 	no_buildid_cache = true;
 }
 
+int build_id_cache__list_all(struct strlist **result)

tools/perf/util/build-id.c +13 -0

+		goto out;
+	}
+	bidlist = strlist__new(NULL, NULL);
+	strlist__for_each(nd, toplist) {
+		if (asprintf(&linkdir, "%s/%s", topdir, nd->s) < 0) {
+			ret = -errno;
+			goto out;
+		}
+		/* Open the lower-level directory */
+		list = lsdir(linkdir, lsdir_no_dot_filter);
+		if (!list) {
+			pr_debug("Failed to open %s: %d\n", linkdir, -errno);
+			goto next;

Em Wed, Jun 08, 2016 at 06:30:30PM +0900, Masami Hiramatsu escreveu:

Quoted 18 lines

tools/perf/util/build-id.c +18 -0

+char *build_id_cache__origname(const char *sbuild_id)
+{
+	char *linkname;
+	char buf[PATH_MAX];
+	char *ret = NULL, *p;
+	size_t offs = 5;	/* == strlen("../..") */
+
+	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
+	if (!linkname)
+		return NULL;
+
+	if (readlink(linkname, buf, PATH_MAX) < 0)
+		goto out;
+	/* The link should be "../..<origpath>/<sbuild_id>" */
+	p = strrchr(buf, '/');	/* Cut off the "/<sbuild_id>" */
+	if (p && (p > buf + offs)) {
+		*p = '\0';
+		if (buf[offs + 1] == '[')

tools/perf/util/build-id.c static char *build_id_cache__linkname(const char *sbuild_id, char *bf, · line 204

Quoted 6 lines

tools/perf/util/build-id.c +6 -0

+			offs++;	/*
+				 * This is a DSO name, like [kernel.kallsyms].
+				 * Skip the first '/', since this is not the
+				 * cache of a regular file.
+				 */
+		ret = strdup(buf + offs);	/* Skip "../..[/]" */

strdup can fail.

Quoted 14 lines

tools/perf/util/build-id.c +6 -0

+	}
+out:
+	free(linkname);
+	return ret;
+}
+
 static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
 {
 	return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");

@@ -390,6 +419,59 @@ void disable_buildid_cache(void)
 	no_buildid_cache = true;
 }
 

tools/perf/util/build-id.c void disable_buildid_cache(void) · line 432

Quoted 11 lines

tools/perf/util/build-id.c +11 -0

+int build_id_cache__list_all(struct strlist **result)
+{
+	struct strlist *toplist, *list, *bidlist;
+	struct str_node *nd, *nd2;
+	char *topdir, *linkdir;
+	char sbuild_id[SBUILD_ID_SIZE];
+	int ret = 0;
+
+	/* Open the top-level directory */
+	if (asprintf(&topdir, "%s/.build-id/", buildid_dir) < 0)
+		return -errno;

Wouldn't be better to just return -1 and leave the caller to look at errno? I know that there is code that does that, but better stick to the errno convention?

From the viewpoint of debugging, maybe it is also possible

tools/perf/util/build-id.c void disable_buildid_cache(void) · line 436

Quoted 4 lines

tools/perf/util/build-id.c +4 -0

+	toplist = lsdir(topdir, lsdir_no_dot_filter);
+	if (!toplist) {
+		pr_debug("Failed to opendir %s\n", topdir);
+		ret = -errno;

ditto

Quoted 17 lines

tools/perf/util/build-id.c +17 -0

+		goto out;
+	}
+	bidlist = strlist__new(NULL, NULL);
+	strlist__for_each(nd, toplist) {
+		if (asprintf(&linkdir, "%s/%s", topdir, nd->s) < 0) {
+			ret = -errno;
+			goto out;
+		}
+		/* Open the lower-level directory */
+		list = lsdir(linkdir, lsdir_no_dot_filter);
+		if (!list) {
+			pr_debug("Failed to open %s: %d\n", linkdir, -errno);
+			goto next;
+		}
+		strlist__for_each(nd2, list) {
+			ret = snprintf(sbuild_id, SBUILD_ID_SIZE, "%s%s",
+					nd->s, nd2->s);

tools/perf/util/build-id.c void disable_buildid_cache(void) · line 459

Quoted 6 lines

tools/perf/util/build-id.c +6 -0

+			if (ret != SBUILD_ID_SIZE - 1) {
+				pr_debug("%s/%s is not buildid cache\n",
+					nd->s, nd2->s);
+				continue;
+			}
+			strlist__add(bidlist, sbuild_id);

strlist__add() can fail, please check its result.

tools/perf/util/build-id.c void disable_buildid_cache(void) · line 469

Quoted 11 lines

tools/perf/util/build-id.c +10 -0

+		}
+		strlist__delete(list);
+next:
+		free(linkdir);
+	}
+
+	*result = bidlist;
+out:
+	if (toplist)
+		strlist__delete(toplist);

No need for checking toplist.

tools/perf/util/build-id.c void disable_buildid_cache(void) · line 473

Quoted 5 lines

tools/perf/util/build-id.c +4 -0

+	free(topdir);
+
+	return ret;
+}

<SNIP>

- Arnaldo

Quoted 19 lines · On Thu, 9 Jun 2016 11:22:55 -0300 Arnaldo Carvalho de Melo <[email protected]>…

Em Wed, Jun 08, 2016 at 06:30:30PM +0900, Masami Hiramatsu escreveu:

Quoted 18 lines

tools/perf/util/build-id.c +18 -0

+char *build_id_cache__origname(const char *sbuild_id)
+{
+	char *linkname;
+	char buf[PATH_MAX];
+	char *ret = NULL, *p;
+	size_t offs = 5;	/* == strlen("../..") */
+
+	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
+	if (!linkname)
+		return NULL;
+
+	if (readlink(linkname, buf, PATH_MAX) < 0)
+		goto out;
+	/* The link should be "../..<origpath>/<sbuild_id>" */
+	p = strrchr(buf, '/');	/* Cut off the "/<sbuild_id>" */
+	if (p && (p > buf + offs)) {
+		*p = '\0';
+		if (buf[offs + 1] == '[')

Quoted 8 lines

tools/perf/util/build-id.c static char *build_id_cache__linkname(const char *sbuild_id, char *bf, · line 204

Quoted 6 lines

tools/perf/util/build-id.c +6 -0

+			offs++;	/*
+				 * This is a DSO name, like [kernel.kallsyms].
+				 * Skip the first '/', since this is not the
+				 * cache of a regular file.
+				 */
+		ret = strdup(buf + offs);	/* Skip "../..[/]" */

strdup can fail.

Ah, right.

Quoted 5 lines

tools/perf/util/build-id.c static char *build_id_cache__linkname(const char *sbuild_id, char *bf, · line 209

Quoted 5 lines

tools/perf/util/build-id.c +5 -0

+	}
+out:
+	free(linkname);
+	return ret;
+}

From: Masami Hiramatsu <[email protected]>

perf-probe --del removes caches when --cache is given. Note that the delete pattern is not same as normal events.

If you cached probes with event name, --del "eventname" works as expected. However, if you skipped it, the cached probes doesn't have actual event name. In that case
--del "probe-desc" is required (wildcard is acceptable).
For example a cache entry has the probe-desc "vfs_read $params", you can remove it with --del 'vfs_read*'.

-----

  # perf probe --cache --list
  /[kernel.kallsyms] (1466a0a250b5d0070c6d0f03c5fed30b237970a1):
  vfs_read $params
  /usr/lib64/libc-2.17.so (c31ffe7942bfd77b2fca8f9bd5709d387a86d3bc):
  getaddrinfo $params

  # perf probe --cache --del vfs_read\*
  Removed cached event: probe:vfs_read

  # perf probe --cache --list
  /[kernel.kallsyms] (1466a0a250b5d0070c6d0f03c5fed30b237970a1):
  /usr/lib64/libc-2.17.so (c31ffe7942bfd77b2fca8f9bd5709d387a86d3bc):
  getaddrinfo $params
  -----

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v9:
- Fix to show which event is deleted (Thanks Hemant!)
Changes in v4:
- move del_perf_probe_caches() into builtin-probe.c since command-line related delete procedure is there now.

---
 tools/perf/Documentation/perf-probe.txt |    1 +
 tools/perf/builtin-probe.c              |   27 +++++++++++++++++++++++++
 tools/perf/util/probe-file.c            |   34 +++++++++++++++++++++++++------
 tools/perf/util/probe-file.h            |    2 ++
 4 files changed, 57 insertions(+), 7 deletions(-)

tools/perf/Documentation/perf-probe.txt +1 -0

index 5a70d45..8d09173 100644
@@ -116,6 +116,7 @@ OPTIONS
 	(With --add) Cache the probes. Any events which successfully added
 	are also stored in the cache file.
 	(With --list) Show cached probes.
+	(With --del) Remove cached probes.
 
 --max-probes=NUM::
 	Set the maximum number of probe points for an event. Default is 128.

tools/perf/builtin-probe.c +24 -0

index 53e380c0e..8f61525 100644
@@ -363,6 +363,30 @@ out_cleanup:
 	return ret;
 }
 
+static int del_perf_probe_caches(struct strfilter *filter)
+{
+	struct probe_cache *cache;
+	struct strlist *bidlist;
+	struct str_node *nd;
+	int ret;
+
+	ret = build_id_cache__list_all(&bidlist);
+	if (ret < 0) {
+		pr_debug("Failed to get buildids: %d\n", ret);
+		return ret;
+	}
+
+	strlist__for_each(nd, bidlist) {
+		cache = probe_cache__new(nd->s);
+		if (!cache)
+			continue;
+		probe_cache__remove_entries(cache, filter);
+		probe_cache__commit(cache);
 
 
 
+		probe_cache__delete(cache);
+	}
+	return 0;
+}
+

static int perf_del_probe_events(struct strfilter *filter) {

tools/perf/builtin-probe.c +3 -0

 	int ret, ret2, ufd = -1, kfd = -1;

@@ -375,6 +399,9 @@ static int perf_del_probe_events(struct strfilter *filter)
 
 	pr_debug("Delete filter: \'%s\'\n", str);
 
+	if (probe_conf.cache)
+		return del_perf_probe_caches(filter);
+
 	/* Get current event names */
 	ret = probe_file__open_both(&kfd, &ufd, PF_FL_RW);
 	if (ret < 0)

tools/perf/util/probe-file.c +8 -0

index a6d4a67..f687607 100644
@@ -660,19 +660,39 @@ out:
 	return ret;
 }
 
+static bool probe_cache_entry__compare(struct probe_cache_entry *entry,
+				       struct strfilter *filter)
+{
+	char buf[128], *ptr = entry->spev;
+
+	if (entry->pev.event) {
+		snprintf(buf, 128, "%s:%s", entry->pev.group, entry->pev.event);
+		ptr = buf;

tools/perf/util/probe-file.c +9 -7

+		if (probe_cache_entry__compare(entry, filter)) {
+			pr_info("Removed cached event: %s\n", entry->spev);
+			probe_cache_entry__delete(entry);
+		}
+	}
+	return 0;
+}
+
 static int probe_cache__show_entries(struct probe_cache *pcache,
 				     struct strfilter *filter)
 {
 	struct probe_cache_entry *entry;
-	char buf[128], *ptr;
 
 	list_for_each_entry(entry, &pcache->list, list) {
-		if (entry->pev.event) {
-			ptr = buf;
-			snprintf(buf, 128, "%s:%s", entry->pev.group, entry->pev.event);
-		} else
-			ptr = entry->spev;
-		if (strfilter__compare(filter, ptr))
+		if (probe_cache_entry__compare(entry, filter))
 			printf("%s\n", entry->spev);
 	}
 	return 0;

tools/perf/util/probe-file.h +2 -0

index ac70446..e6fd9b9 100644
@@ -37,6 +37,8 @@ int probe_cache__add_entry(struct probe_cache *pcache,
 			   struct probe_trace_event *tevs, int ntevs);
 int probe_cache__commit(struct probe_cache *pcache);
 void probe_cache__delete(struct probe_cache *pcache);
+int probe_cache__remove_entries(struct probe_cache *pcache,
+				struct strfilter *filter);
 struct probe_cache_entry *probe_cache__find(struct probe_cache *pcache,
 					    struct perf_probe_event *pev);
 struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,

Quoted 23 lines

tools/perf/util/probe-file.c +15 -0

index a6d4a67..f687607 100644
@@ -660,19 +660,39 @@ out:
 	return ret;
 }
 
+static bool probe_cache_entry__compare(struct probe_cache_entry *entry,
+				       struct strfilter *filter)
+{
+	char buf[128], *ptr = entry->spev;
+
+	if (entry->pev.event) {
+		snprintf(buf, 128, "%s:%s", entry->pev.group, entry->pev.event);
+		ptr = buf;
+	}
+	return strfilter__compare(filter, ptr);
+}
+
+int probe_cache__remove_entries(struct probe_cache *pcache,
+				struct strfilter *filter)
+{

tools/perf/util/probe-file.c out: · line 680

Quoted 3 lines

tools/perf/util/probe-file.c +3 -0

+	struct probe_cache_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &pcache->list, list) {

so here you used the preferred idiom, i.e. using list_for_each_entry_safe(), once you stop doing the list removal at the list entry destructor, it gets the good old boring usual idiom, please do that.

Also please consider renaming perf_cache__remove_entries() to perf_cache__filter_purge(), as it doesn't simply remove entries, it purges them (that is, remove an entry and delete it), and only if the entry got filtered.