This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH v3 2/5] perf/sdt: Add SDT events into a cache
- From: Hemant Kumar <hemant at linux dot vnet dot ibm dot com>
- To: Namhyung Kim <namhyung at kernel dot org>
- Cc: linux-kernel at vger dot kernel dot org, srikar at linux dot vnet dot ibm dot com, peterz at infradead dot org, oleg at redhat dot com, hegdevasant at linux dot vnet dot ibm dot com, mingo at redhat dot com, anton at redhat dot com, systemtap at sourceware dot org, masami dot hiramatsu dot pt at hitachi dot com, aravinda at linux dot vnet dot ibm dot com, penberg at iki dot fi
- Date: Fri, 24 Oct 2014 16:58:06 +0530
- Subject: Re: [PATCH v3 2/5] perf/sdt: Add SDT events into a cache
- Authentication-results: sourceware.org; auth=none
- References: <20141010104402 dot 15506 dot 73285 dot stgit at hemant-fedora> <20141010105758 dot 15506 dot 20442 dot stgit at hemant-fedora> <8761fcg1hm dot fsf at sejong dot aot dot lge dot com>
On 10/22/2014 10:11 AM, Namhyung Kim wrote:
On Fri, 10 Oct 2014 16:28:24 +0530, Hemant Kumar wrote:
This patch adds a new sub-command to perf : sdt-cache.
sdt-cache command can be used to add SDT events.
When user invokes "perf sdt-cache add <file-name>", a hash table/list is
created named as file_hash list. A typical entry in a file_hash table looks
like:
file hash file_sdt_ent file_sdt_ent
|---------| --------------- -------------
| hlist ==|===|=>file_list =|===|=>file_list=|==..
key = 644 =>| | | sbuild_id | | sbuild_id |
|---------| | name | | name |
| | | sdt_list | | sdt_list |
key = 645 =>| hlist | | || | | || |
|---------| --------------- --------------
|| || || Connected to SDT notes
---------------
| note_list |
| name |sdt_note
| provider |
-----||--------
connected to other SDT notes
Each entry of the file_hash table is an hlist which connects to file_list
in file_sdt_ent. file_sdt_ent is allocated per file whenever a file is
mapped to file_hash list. File name serves as the key to this hash table.
If a file is added to this hash list, a file_sdt_ent is allocated and a
list of SDT events in that file is created and assigned to sdt_list of
file_sdt_ent.
Example usage :
# ./perf sdt-cache --add /home/user_app
4 events added for /home/user_app
# ./perf sdt-cache --add /lib64/libc.so.6
8 events added for /usr/lib64/libc-2.16.so
Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
[SNIP]
+static int file_hash_list__remove(struct hash_table *file_hash,
+ const char *target)
+{
+ struct file_sdt_ent *fse;
+ struct hlist_head *ent_head;
+ struct list_head *sdt_head;
+ struct hlist_node *tmp1;
+ char *res_path;
+ int key, nr_del = 0;
+
+ res_path = realpath(target, NULL);
+ if (!res_path)
+ return -ENOMEM;
+
+ key = get_hash_key(target);
+ ent_head = &file_hash->ent[key];
+
+ /* Got the file hash entry */
+ hlist_for_each_entry_safe(fse, tmp1, ent_head, file_list) {
+ sdt_head = &fse->sdt_list;
+ if (strcmp(res_path, fse->name))
+ continue;
+
+ /* Got the file list entry, now start removing */
+ nr_del = cleanup_sdt_note_list(sdt_head);
+ hlist_del(&fse->file_list);
+ list_del(&fse->sdt_list);
It seems you don't need to call list_del() here. And what about just
calling cleanup_sdt_note_list(&fse->sdt_list) instead?
Yeah, will remove this.
+ free(fse);
+ }
+ free(res_path);
+ return nr_del;
+}
[SNIP]
+static int sdt_note__read(char *data, struct list_head *sdt_list)
+{
+ struct sdt_note *sn;
+ char *tmp, *ptr, delim[2];
+ int ret = -EBADF, val;
+
+ /* Initialize the delimiter with DELIM */
+ delim[0] = DELIM;
What about delim[1] then?
It should be '\0' here, will initialize it.
+ sn = malloc(sizeof(*sn));
+ if (!sn) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ INIT_LIST_HEAD(&sn->note_list);
+
+ /* Provider name */
+ ptr = strtok_r(data, delim, &tmp);
+ if (!ptr)
+ goto out;
+ sn->provider = strdup(ptr);
+ if (!sn->provider) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ /* Marker name */
+ ptr = strtok_r(NULL, delim, &tmp);
+ if (!ptr)
+ goto out;
+ sn->name = strdup(ptr);
+ if (!sn->name) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ /* Location */
+ ptr = strtok_r(NULL, delim, &tmp);
+ if (!ptr)
+ goto out;
+ val = sscanf(ptr, "%lx", &sn->addr.a64[0]);
+ if (val == EOF)
+ goto out;
+ /* Sempahore */
+ ptr = strtok_r(NULL, delim, &tmp);
+ if (!ptr)
+ goto out;
+ val = sscanf(ptr, "%lx", &sn->addr.a64[2]);
+ if (val == EOF)
+ goto out;
+ /* Add to the SDT list */
+ list_add(&sn->note_list, sdt_list);
+ ret = 0;
+out:
+ return ret;
+}
+
+/**
+ * file_hash_list__populate: Fill up the file hash table
+ * @file_hash: empty file hash table
+ * @cache: FILE * to read from
+ *
+ * Parse @cache for file_name and its SDT events.
+ * The format of the cache is :
+ *
+ * file_name:build_id\n
+ * provider:marker:location:semaphore\n
You seem forgot to add a file delimiter before new file.
:\n
+ * file_name:build_id\n
+ * ...
Anyway IMHO it's bit uncomfortable. How about changing the format same
as it dumps? A line starts with '%' is a sdt description, otherwise
file info:
file_name1:build_id\n
%provide:marker1:location:semaphore\n
%provide:marker2:location:semaphore\n
%...
file_name2:build_id\n
%...
And we can deal with it by usual fgets/getline and strtok.
Oh, it's just a suggestion..
No problem. Will change the format to above.
+ *
+ * Parse according to the above format. Find out the file_name and build_id
+ * first and then use sdt_note__read() to parse the SDT note info.
+ * Find out the hash key from the file_name and use that to add this new
+ * entry to file hash.
+ */
+static int file_hash_list__populate(struct hash_table *file_hash, FILE *cache)
+{
+ struct file_sdt_ent *fse;
+ int key, val, ret = -EBADF;
+ char data[2 * PATH_MAX], *ptr, *tmp;
+
+ for (val = fscanf(cache, "%s", data); val != EOF;
+ val = fscanf(cache, "%s", data)) {
+ fse = malloc(sizeof(*fse));
+ if (!fse) {
+ ret = -ENOMEM;
+ break;
+ }
+ INIT_LIST_HEAD(&fse->sdt_list);
+ INIT_HLIST_NODE(&fse->file_list);
+
+ /* First line is file name and build id */
+ ptr = strtok_r(data, ":", &tmp);
+ if (!ptr)
+ break;
+ strcpy(fse->name, ptr);
+ ptr = strtok_r(NULL, ":", &tmp);
+ if (!ptr)
+ break;
+ strcpy(fse->sbuild_id, ptr);
+
+ /* Now, try to get the SDT notes for that file */
+ while ((fscanf(cache, "%s", data)) != EOF) {
+ if (!strcmp(data, ":"))
+ break;
+ ret = sdt_note__read(data, &fse->sdt_list);
+ if (ret < 0)
+ goto out;
+ }
+ key = get_hash_key(fse->name);
+ hlist_add_head(&fse->file_list, &file_hash->ent[key]);
+ ret = 0;
+ }
+out:
+ return ret;
+}
+
+/**
+ * file_hash_list__init: Initializes the file hash list
+ * @file_hash: empty file_hash
+ *
+ * Initializes the ent's of file_hash and opens the cache file.
What is the ent's?
Hash entries. :) Will change to that to entries.
+ * To look for the cache file, look into the directory in HOME env variable.
+ */
+static int file_hash_list__init(struct hash_table *file_hash)
+{
+ FILE *cache;
+ int i, ret = 0;
+ char sdt_cache_path[MAXPATHLEN], *val;
s/MAXPATHLEN/PATH_MAX/ ?
Ok, for consistency, will change it to PATH_MAX.
+
+ for (i = 0; i < SDT_HASH_SIZE; i++)
+ INIT_HLIST_HEAD(&file_hash->ent[i]);
+
+ val = getenv("HOME");
+ if (val)
+ snprintf(sdt_cache_path, MAXPATHLEN-1, "%s/%s/%s",
+ val, SDT_CACHE_DIR, SDT_FILE_CACHE);
Then you can use scnprintf(sdt_cache_path, sizeof(sdt_cache_path), ...).
Ok.
+ else {
+ pr_err("Error: Couldn't get user's home directory\n");
+ ret = -1;
+ goto out;
+ }
+
+ cache = fopen(sdt_cache_path, "r");
+ if (cache == NULL)
+ goto out;
+
+ /* Populate the hash list */
+ ret = file_hash_list__populate(file_hash, cache);
+ fclose(cache);
+out:
+ return ret;
+}
+
+/**
+ * file_hash_list__cleanup: Frees up all the space taken by file_hash list
+ * @file_hash: file_hash table
+ */
+static void file_hash_list__cleanup(struct hash_table *file_hash)
+{
+ struct file_sdt_ent *file_pos;
+ struct list_head *sdt_head;
+ struct hlist_head *ent_head;
+ struct hlist_node *tmp;
+ int i;
+
+ /* Go through all the entries */
+ for (i = 0; i < SDT_HASH_SIZE; i++) {
+ ent_head = &file_hash->ent[i];
+
+ hlist_for_each_entry_safe(file_pos, tmp, ent_head, file_list) {
+ sdt_head = &file_pos->sdt_list;
+
+ /* Cleanup the corresponding SDT notes' list */
+ cleanup_sdt_note_list(sdt_head);
+ hlist_del(&file_pos->file_list);
+ list_del(&file_pos->sdt_list);
Ditto. Don't call list_del() but call cleanup_sdt_note_list directly.
Sure.
+ free(file_pos);
+ }
+ }
+}
+
+
+/**
+ * add_to_hash_list: add an entry to file_hash_list
+ * @file_hash: file hash table
+ * @target: file name
+ *
+ * Finds out the build_id of @target, checks if @target is already present
+ * in file hash list. If not present, delete any stale entries with this
+ * file name (i.e., entries matching this file name but having older
+ * build ids). And then, adds the file entry to file hash list and also
+ * updates the SDT events in the event hash list.
+ */
+static int add_to_hash_list(struct hash_table *file_hash, const char *target)
+{
+ struct file_info *file;
+ int ret = -EBADF;
+ u8 build_id[BUILD_ID_SIZE];
+
+ LIST_HEAD(sdt_notes);
+
+ file = malloc(sizeof(*file));
+ if (!file)
+ return -ENOMEM;
+
+ file->name = realpath(target, NULL);
+ if (!file->name) {
+ pr_err("%s: Bad file name\n", target);
+ goto out;
+ }
+
+ symbol__elf_init();
No need to call symbol__elf_init() here since we already called it in
cmd_sdt_cache().
Yeah, my bad. Will remove it.
+ if (filename__read_build_id(file->name, &build_id,
+ sizeof(build_id)) < 0) {
+ pr_err("Couldn't read build-id in %s\n", file->name);
+ sdt_err(ret, file->name);
+ goto out;
+ }
+ build_id__sprintf(build_id, sizeof(build_id), file->sbuild_id);
+
+ /* File entry already exists ?*/
+ if (file_hash_list__entry_exists(file_hash, file)) {
+ pr_err("Error: SDT events for %s already exist\n",
+ file->name);
+ ret = 0;
+ goto out;
+ }
+
+ /*
+ * This should remove any stale entries (if any) from the file hash.
+ * Stale entries will have the same file name but different build ids.
+ */
+ ret = file_hash_list__remove(file_hash, file->name);
+ if (ret < 0)
+ goto out;
+ ret = get_sdt_note_list(&sdt_notes, file->name);
+ if (ret < 0)
+ sdt_err(ret, target);
+ /* Add the entry to file hash list */
+ ret = file_hash_list__add(file_hash, &sdt_notes, file);
+out:
+ free(file->name);
+ free(file);
+ return ret;
+}
[SNIP]
+static int flush_hash_list_to_cache(struct hash_table *file_hash)
+{
+ FILE *cache;
+ int ret;
+ struct stat buf;
+ char sdt_cache_path[MAXPATHLEN], sdt_dir[MAXPATHLEN], *val;
+
+ val = getenv("HOME");
+ if (val) {
+ snprintf(sdt_dir, MAXPATHLEN-1, "%s/%s", val, SDT_CACHE_DIR);
+ snprintf(sdt_cache_path, MAXPATHLEN-1, "%s/%s",
+ sdt_dir, SDT_FILE_CACHE);
Ditto. s/MAXPATHLEN/PATH_MAX/g and use scnprintf().
Thanks,
Namhyung
+ } else {
+ pr_err("Error: Couldn't get the user's home directory\n");
+ ret = -1;
+ goto out_err;
+ }
+ ret = stat(sdt_dir, &buf);
+ if (ret) {
+ ret = mkdir(sdt_dir, buf.st_mode);
+ if (ret) {
+ pr_err("Error: Couldn't create %s\n", sdt_dir);
+ goto out_err;
+ }
+ }
+
+ cache = fopen(sdt_cache_path, "w");
+ if (!cache) {
+ pr_err("Error in creating %s file\n", sdt_cache_path);
+ ret = -errno;
+ goto out_err;
+ }
+
+ file_hash_list__flush(file_hash, cache);
+ fclose(cache);
+out_err:
+ return ret;
+}
Thanks a lot for reviewing the patch.
--
Thanks,
Hemant Kumar