Em Wed, Jun 08, 2016 at 06:29:59PM +0900, Masami Hiramatsu escreveu:
> 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 <mhira...@kernel.org>
> ---
> 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(+)
>
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 3fe6214..689d874 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -14,6 +14,7 @@
> * GNU General Public License for more details.
> *
> */
> +#include <sys/uio.h>
> #include "util.h"
> #include "event.h"
> #include "strlist.h"
> @@ -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.
> + 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.
> + 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.
> + 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)
> +{
> + 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.
> + 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;
> + }
> + 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.
> +
> + 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:
> 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.
> struct perf_probe_event pev;
> char *spev;
> struct strlist *tevlist;
> };
> 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.
> };
> + 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?
> +
> + 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;
> +
> + 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?
> + 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;
> +
> + 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?
> +
> + 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);
> + 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.
> + 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;
> + 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?
> +
> + 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;
> +}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index 18ac9cf..d2b8791d 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -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
