Message-ID: <5430FE19.3050803@linux.vnet.ibm.com>
Date:	Sun, 05 Oct 2014 13:45:21 +0530
From:	Hemant Kumar <hemant@...ux.vnet.ibm.com>
To:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
CC:	linux-kernel@...r.kernel.org, srikar@...ux.vnet.ibm.com,
	peterz@...radead.org, oleg@...hat.com,
	hegdevasant@...ux.vnet.ibm.com, mingo@...hat.com, anton@...hat.com,
	systemtap@...rceware.org, namhyung@...nel.org,
	aravinda@...ux.vnet.ibm.com, penberg@....fi
Subject: Re: [PATCH v1 2/5] perf/sdt: Add SDT events into a cache


On 10/03/2014 06:17 PM, Masami Hiramatsu wrote:
> (2014/10/01 0:32), Hemant Kumar wrote:
>> This patch adds a new sub-command to perf : sdt-cache.
> When I ran perf without option, sdt-cache did not appear. You should update
> command-list.txt and add Documentation/perf-sdt-cache.txt, so that sdt-cache is
> shown (Note that perf build automatically extracts NAME line and embeds it as a
> help message). Maybe commit ef12a141 is good to refer :)

Will add that.

>> sdt-cache command can be used to add, remove and dump SDT events.
>> This patch adds support to only "add" the SDT events. The rest of the
>> patches add support to rest of them.
> You don't need to describe dump and remove in this patch. How about below?
>
>    perf-sdt-cache command manages SDT events in applications. This adds
>    "perf sdt-cache --add <file>" to add SDT events in given file to local
>    cache.
>

Ok.

>> When user invokes "perf sdt-cache add <file-name>", two hash tables are
>> created: file_hash list and event_hash list. A typical entry in a file_hash
>> table looks like:
>>              file hash      file_sdt_ent     file_sdt_ent
>>              |---------|   ---------------   -------------
>>              | list  ==|===|=>file_list =|===|=>file_list=|==..
>> key = 644 =>|         |   | sbuild_id   |   | sbuild_id  |
>>              |---------|   | name        |   | name       |
>>              |	      |   | sdt_list    |   | sdt_list   |
>> key = 645 =>| list    |   |   ||        |   |    ||      |
>>              |---------|   ---------------   --------------
>> 	        ||            ||                 || Connected to SDT notes
>> 	                  ---------------
>> 			  |  note_list  |
>> 		  sdt_note|  name       |
>> 			  |  provider	|
>>           		  -----||--------
>> 		  connected to other SDT notes
> Hmm, why don't you use hlist for file_hash.list?

Will use hlist.

>>
>> Each entry of the file_hash table is a list 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!
> IMO, when succeeding to add events, it may not need the last "!", since
> it is eye-catching too much (as like as we get an error).

Ok. Will remove '!'.

>
> [...]
>> diff --git a/tools/perf/util/sdt.c b/tools/perf/util/sdt.c
>> new file mode 100644
>> index 0000000..0b56210
>> --- /dev/null
>> +++ b/tools/perf/util/sdt.c
>> @@ -0,0 +1,665 @@
>> +/*
>> + * util/sdt.c
>> + * This contains the relevant functions needed to find the SDT events
>> + * in a binary and adding them to a cache.
>> + *
>> + * TODOS:
>> + * - Listing SDT events in most of the binaries present in the system.
>> + * - Looking into directories provided by the user for binaries with SDTs,
>> + * etc.
>> + * - Support SDT event arguments.
>> + * - Support SDT event semaphores.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <errno.h>
>> +#include <string.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <unistd.h>
>> +#include <limits.h>
>> +#include <sys/mman.h>
>> +#include <fcntl.h>
>> +
>> +#include "parse-events.h"
>> +#include "probe-event.h"
>> +#include "linux/list.h"
>> +#include "symbol.h"
>> +#include "build-id.h"
>> +
>> +struct file_info {
>> +	char *name;                             /* File name */
>> +	char sbuild_id[BUILD_ID_SIZE * 2 + 1];  /* Build id of the file */
>> +};
>> +
>> +/**
>> + * get_hash_key: calculates the key to hash tables
>> + * @str: input string
>> + *
>> + * adds the ascii values for all the chars in @str, multiplies with a prime
>> + * and finds the modulo with HASH_TABLE_SIZE.
>> + *
>> + * Return : An integer key
>> + */
>> +static unsigned get_hash_key(const char *str)
>> +{
>> +	unsigned value = 0, key;
>> +	unsigned c;
>> +
>> +	for (c = 0; str[c] != '\0'; c++)
>> +		value += str[c];
>> +
>> +	key = (value * 37) % HASH_TABLE_SIZE;
> 37 seems to a magic number :) how about defining it as HASH_PRIME_BASE?

Yeah. will make it a #define.

>> +
>> +	return key;
>> +}
>> +
>> +/**
>> + * sdt_err: print SDT related error
>> + * @val: error code
>> + * @target: input file
>> + *
>> + * Display error corresponding to @val for file @target
>> + */
>> +static int sdt_err(int val, const char *target)
>> +{
>> +	switch (-val) {
>> +	case 0:
>> +		break;
>> +	case ENOENT:
>> +		/* Absence of SDT markers */
>> +		pr_err("%s: No SDT events found\n", target);
>> +		break;
>> +	case EBADF:
>> +		pr_err("%s: Bad file name\n", target);
>> +		break;
>> +	default:
>> +		pr_err("%s\n", strerror(val));
>> +	}
>> +
>> +	return val;
>> +}
>> +
>> +/**
>> + * cleanup_sdt_note_list : free the sdt notes' list
>> + * @sdt_notes: sdt notes' list
>> + *
>> + * Free up the SDT notes in @sdt_notes.
>> + */
>> +static void cleanup_sdt_note_list(struct list_head *sdt_notes)
>> +{
>> +	struct sdt_note *tmp, *pos;
>> +
>> +	if (list_empty(sdt_notes))
>> +		return;
>> +
>> +	list_for_each_entry_safe(pos, tmp, sdt_notes, note_list) {
>> +		list_del(&pos->note_list);
>> +		free(pos->name);
>> +		free(pos->provider);
>> +		free(pos);
>> +	}
>> +}
> Hmm, since the list is created in tools/perf/util/symbol-elf.c, it is better
> to be defined in the same file, and same patch(1/1).

Ok. Moving into patch1 and inside util/symbol-elf.c does make sense.

>> +
>> +/**
>> + * file_list_entry__init: Initialize a file_list_entry
>> + * @fse: file_list_entry
>> + * @file: file information
>> + *
>> + * Initializes @fse with the build id of a @file.
>> + */
>> +static void file_list_entry__init(struct file_sdt_ent *fse,
>> +				 struct file_info *file)
>> +{
>> +	strcpy(fse->sbuild_id, file->sbuild_id);
>> +	INIT_LIST_HEAD(&fse->file_list);
>> +	INIT_LIST_HEAD(&fse->sdt_list);
>> +}
>> +
>> +/**
>> + * sdt_events__get_count: Counts the number of sdt events
>> + * @start: list_head to sdt_notes list
>> + *
>> + * Returns the number of SDT notes in a list
>> + */
>> +static int sdt_events__get_count(struct list_head *start)
>> +{
>> +	struct sdt_note *sdt_ptr;
>> +	int count = 0;
>> +
>> +	list_for_each_entry(sdt_ptr, start, note_list) {
>> +		count++;
>> +	}
>> +	return count;
>> +}
> This should be also in symbol-elf.c or symbol.h. Moreover, since there is no sdt_events
> data structure, the function name looks a bit odd.

Will rename it to sdt_note_list__get_count() and will move it to symbol.h.

>
>> +
>> +/**
>> + * file_hash_list__add: add an entry to file_hash_list
>> + * @sdt_notes: list of sdt_notes
>> + * @file: file name and build id
>> + * @file_hash: hash table for file and sdt notes
>> + *
>> + * Get the key corresponding to @file->name. Fetch the entry
>> + * for that key. Build a file_list_entry fse, assign the SDT notes
>> + * to it and then assign fse to the fetched entry into the hash.
>> + */
>> +static int file_hash_list__add(struct list_head *sdt_notes,
>> +				struct file_info *file,
>> +				struct hash_list *file_hash)
>> +{
>> +	struct file_sdt_ent *fse;
>> +	struct list_head *ent_head;
>> +	int key, nr_evs;
>> +
>> +	key = get_hash_key(file->name);
>> +	ent_head = &file_hash->ent[key].list;
>> +
>> +	/* Initialize the file entry */
>> +	fse = (struct file_sdt_ent *)malloc(sizeof(struct file_sdt_ent));
>> +	if (!fse)
>> +		return -ENOMEM;
>> +	file_list_entry__init(fse, file);
>> +	nr_evs = sdt_events__get_count(sdt_notes);
>> +	list_splice(sdt_notes, &fse->sdt_list);
>> +
>> +	printf("%d events added for %s\n", nr_evs, file->name);
>> +	strcpy(fse->name, file->name);
>> +
>> +	/* Add the file to the file hash entry */
>> +	list_add(&fse->file_list, ent_head);
>> +
>> +	return MOD;
>> +}
>> +
>> +/**
>> + * file_hash_list__remove: Remove a file entry from the file_hash table
>> + * @file_hash: file_hash_table
>> + * @target: file name
>> + *
>> + * Removes the entries from file_hash table corresponding to @target.
>> + * Gets the key from @target. Also frees up the SDT events for that
>> + * file.
>> + */
>> +static int file_hash_list__remove(struct hash_list *file_hash,
>> +				   const char *target)
>> +{
>> +	struct file_sdt_ent *fse, *tmp1;
>> +	struct list_head *sdt_head, *ent_head;
>> +	struct sdt_note *sdt_ptr, *tmp2;
>> +
>> +	char *res_path;
>> +	int key, nr_del = 0;
>> +
>> +	key = get_hash_key(target);
>> +	ent_head = &file_hash->ent[key].list;
>> +
>> +	res_path = realpath(target, NULL);
>> +	if (!res_path)
>> +		return -ENOMEM;
>> +
>> +	if (list_empty(ent_head))
>> +		return 0;
>> +
>> +	/* Got the file hash entry */
>> +	list_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 */
>> +		list_for_each_entry_safe(sdt_ptr, tmp2, sdt_head, note_list) {
>> +			list_del(&sdt_ptr->note_list);
>> +			free(sdt_ptr->name);
>> +			free(sdt_ptr->provider);
>> +			free(sdt_ptr);
>> +			nr_del++;
>> +		}
> You can use cleanup_sdt_note_list() here. If you need to count nr_del, modify
> cleanup_sdt_note_list() to return the number.

Oops, yeah I can use cleanup_sdt_note_list() here.

>> +		list_del(&fse->file_list);
>> +		list_del(&fse->sdt_list);
>> +		free(fse);
>> +	}
>> +
>> +	return nr_del;
>> +}
>> +/**
>> + * file_hash_list__entry_exists: Checks if a file is already present
>> + * @file_hash: file_hash table
>> + * @file: file name and build id to check
>> + *
>> + * Obtains the key from @file->name, fetches the correct entry,
>> + * and goes through the chain to find out the correct file list entry.
>> + * Compares the build id, if they match, return true, else, false.
>> + */
>> +static bool file_hash_list__entry_exists(struct hash_list *file_hash,
>> +					  struct file_info *file)
>> +{
>> +	struct file_sdt_ent *fse;
>> +	struct list_head *ent_head;
>> +	int key;
>> +
>> +	key = get_hash_key(file->name);
>> +	ent_head = &file_hash->ent[key].list;
>> +	if (list_empty(ent_head))
>> +		return false;
>> +
>> +	list_for_each_entry(fse, ent_head, file_list) {
>> +		if (!strcmp(file->name, fse->name)) {
>> +			if (!strcmp(file->sbuild_id, fse->sbuild_id))
>> +				return true;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + * copy_delim: copy till a character
>> + * @src: source string
>> + * @target: dest string
>> + * @delim: till this character
>> + * @len: length of @target
>> + * @end: end of @src
>> + *
>> + * Returns the number of chars copied.
>> + * NOTE: We need @end as @src may or may not be terminated by NULL
>> + */
>> +static int copy_delim(char *src, char *target, char delim, size_t len,
>> +		      char *end)
>> +{
>> +	int i;
>> +
>> +	memset(target, '\0', len);
> I'm not sure why this is needed.
>
>> +	for (i = 0; (src[i] != delim) && !(src + i > end) ; i++)
>> +		target[i] = src[i];
>> +
>> +	return i + 1;
>> +}
> Anyway, this may be replaced with strchr(3) or strtok_r(3) and strncpy(3).

Will discard this whole func and use strchr() and strtok_r().

>> +
>> +/**
>> + * sdt_note__read: Parse SDT note info
>> + * @ptr: raw data
>> + * @sdt_list: empty list
>> + * @end: end of the raw data
>> + *
>> + * Parse @ptr to find out SDT note name, provider, location and semaphore.
>> + * All these data are separated by DELIM.
>> + */
>> +static void sdt_note__read(char **ptr, struct list_head *sdt_list, char *end)
>> +{
>> +	struct sdt_note *sn;
>> +	char addr[MAX_ADDR];
>> +	int len = 0;
>> +
>> +	while (1) {
> Hmm, shouldn't we check the end of the sdt note?

We are checking this inside the loop with copy_delim() and if (*ptr == 
DELIM), but
I guess its better to put the check in while itself.

>
>> +		sn = (struct sdt_note *)malloc(sizeof(struct sdt_note));
>> +		if (!sn)
>> +			return;
>> +		INIT_LIST_HEAD(&sn->note_list);
>> +		sn->name = (char *)malloc(PATH_MAX);
>> +		if (!sn->name)
>> +			return;
>> +		sn->provider = (char *)malloc(PATH_MAX);
>> +		if (!sn->provider)
>> +			return;
>> +		/* Extract the provider name */
>> +		len = copy_delim(*ptr, sn->provider, DELIM, PATH_MAX, end);
>> +		*ptr += len;
>> +
>> +		/* Extract the note name */
>> +		len = copy_delim(*ptr, sn->name, DELIM, PATH_MAX, end);
>> +		*ptr += len;
>> +
>> +		/* Extract the note's location */
>> +		len = copy_delim(*ptr, addr , DELIM, MAX_ADDR, end);
>> +		*ptr += len;
>> +		sscanf(addr, "%lx", &sn->addr.a64[0]);
>> +
>> +		/* Extract the sem location */
>> +		len = copy_delim(*ptr, addr , DELIM, MAX_ADDR, end);
>> +		*ptr += len;
>> +		sscanf(addr, "%lx", &sn->addr.a64[2]);
>> +		list_add(&sn->note_list, sdt_list);
>> +
>> +		/* If the entries for this file are finished */
>> +		if (**ptr == DELIM)
>> +			break;
>> +	}
>> +}
>> +
>> +/**
>> + * file_hash_list__populate: Fill up the file hash table
>> + * @file_hash: empty file hash table
>> + * @data: raw cache data
>> + * @size: size of data
>> + *
>> + * Find out the end of data with help of @size. Start parsing @data.
>> + * In the outer loop, it reads the 'key' delimited by "::-". In the inner
>> + * loop, it reads the file name, build id and calls sdt_note__read() to
>> + * read the SDT notes. Multiple entries for the same key are delimited
>> + * by "::". Then, it assigns the file name, build id and SDT notes to the
>> + * list entry corresponding to 'key'.
>> + */
>> +static void file_hash_list__populate(struct hash_list *file_hash, char *data,
>> +				     off_t size)
>> +{
>> +	struct list_head *ent_head;
>> +	struct file_sdt_ent *fse;
>> +	char *end, tmp[PATH_MAX], *ptr;
>> +	int key, len = 0;
>> +
>> +	end = data + size - 1;
>> +	ptr = data;
>> +	if (ptr == end)
>> +		return;
>> +	do {
> Why don't you use while(ptr < end) {} here ?

Will use that.

>> +		/* Obtain the key */
>> +		len = copy_delim(ptr, tmp, DELIM, PATH_MAX, end);
>> +		ptr += len;
>> +		key = atoi(tmp);
>> +		/* Obtain the file_hash list entry */
>> +		ent_head = &file_hash->ent[key].list;
>> +		while (1) {
>> +			fse = (struct file_sdt_ent *)
>> +				malloc(sizeof(struct file_sdt_ent));
>> +			if (!fse)
>> +				return;
>> +			INIT_LIST_HEAD(&fse->file_list);
>> +			INIT_LIST_HEAD(&fse->sdt_list);
>> +			/* Obtain the file name */
>> +			len = copy_delim(ptr, fse->name, DELIM,
>> +					 sizeof(fse->name), end);
>> +			ptr += len;
>> +			/* Obtain the build id */
>> +			len = copy_delim(ptr, fse->sbuild_id, DELIM,
>> +					 sizeof(fse->sbuild_id), end);
>> +			ptr += len;
>> +			sdt_note__read(&ptr, &fse->sdt_list, end);
>> +
>> +			list_add(&fse->file_list, ent_head);
>> +
>> +			/* Check for another file entry */
>> +			if ((*ptr++ == DELIM) && (*ptr == '-'))
>> +				break;
>> +		}
>> +		if (ptr == end)
>> +			break;
>> +		else
>> +			ptr++;
>> +
>> +	} while (1);
>> +}
>> +
>> +/**
>> + * file_hash_list__init: Initializes the file hash list
>> + * @file_hash: empty file_hash
>> + *
>> + * Opens the cache file, reads the data into 'data' and calls
>> + * file_hash_list__populate() to populate the above hash list.
>> + */
>> +static void file_hash_list__init(struct hash_list *file_hash)
> This should return an error code, since there are lots of error paths.

Yeah, will change this.

>> +{
>> +	struct stat sb;
>> +	char *data;
>> +	int fd, i, ret;
>> +
>> +	for (i = 0; i < HASH_TABLE_SIZE; i++)
>> +		INIT_LIST_HEAD(&file_hash->ent[i].list);
>> +
>> +	fd = open(SDT_CACHE_DIR SDT_FILE_CACHE, O_RDONLY);
>> +	if (fd == -1)
>> +		return;
>> +
>> +	ret = fstat(fd, &sb);
>> +	if (ret == -1) {
>> +		pr_err("fstat : error\n");
>> +		return;
>> +	}
>> +
>> +	if (!S_ISREG(sb.st_mode)) {
>> +		pr_err("%s is not a regular file\n", SDT_CACHE_DIR
>> +		       SDT_FILE_CACHE);
>> +		return;
>> +	}
>> +	/* Is size of the cache zero ?*/
>> +	if (!sb.st_size)
>> +		return;
>> +	/* Read the data */
>> +	data = mmap(0, sb.st_size, PROT_READ, MAP_SHARED, fd, 0);
>> +	if (data == MAP_FAILED) {
>> +		pr_err("Error in mmap\n");
>> +		return;
>> +	}
>> +
>> +	ret = madvise(data, sb.st_size, MADV_SEQUENTIAL);
>> +	if (ret == -1)
>> +		pr_debug("Error in madvise\n");
> At first step, you don't need to take care of the speed.

Will move this to where I parse the data.

>> +	ret = close(fd);
>> +	if (ret == -1) {
>> +		pr_err("Error in close\n");
>> +		return;
>> +	}
>> +
>> +	/* Populate the hash list */
>> +	file_hash_list__populate(file_hash, data, sb.st_size);
>> +	ret = munmap(data, sb.st_size);
>> +	if (ret == -1)
>> +		pr_err("Error in munmap\n");
>> +}
>> +
>> +/**
>> + * file_hash_list__cleanup: Free up all the space for file_hash list
>> + * @file_hash: file_hash table
>> + */
>> +static void file_hash_list__cleanup(struct hash_list *file_hash)
>> +{
>> +	struct file_sdt_ent *file_pos, *tmp;
>> +	struct list_head *ent_head, *sdt_head;
>> +	int i;
>> +
>> +	/* Go through all the entries */
>> +	for (i = 0; i < HASH_TABLE_SIZE; i++) {
>> +		ent_head = &file_hash->ent[i].list;
>> +		if (list_empty(ent_head))
>> +			continue;
>> +
>> +		list_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);
>> +			list_del(&file_pos->file_list);
>> +			list_del(&file_pos->sdt_list);
>> +			free(file_pos);
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * add_to_hash_list: add an entry to file_hash_list
>> + * @file_hash: file hash table
>> + * @target: file name
>> + *
>> + * Does a sanity check for the @target, finds out its build id,
>> + * 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_list *file_hash, const char *target)
>> +{
>> +	struct file_info *file;
>> +	int ret = 0;
>> +	u8 build_id[BUILD_ID_SIZE];
>> +
>> +	LIST_HEAD(sdt_notes);
>> +
>> +	file = (struct file_info *)malloc(sizeof(struct file_info));
>> +	if (!file)
>> +		return -ENOMEM;
>> +
>> +	file->name = realpath(target, NULL);
>> +	if (!file->name) {
>> +		ret = -EBADF;
>> +		pr_err("%s: Bad file name!\n", target);
>> +		goto out;
>> +	}
>> +
>> +	symbol__elf_init();
>> +	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);
>> +		ret = -EBADF;
>> +		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!",
>> +		       file->name);
>> +		ret = NO_MOD;
> NO_MOD is too generic. I'd like to suggest that this returns the number of
> modified(added) entries.

Ok. Will modify this to return the no. of entries added.

>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * This will also remove any stale entries (if any) from the event hash.
>> +	 * Stale entries will have the same file name but older build ids.
>> +	 */
>> +	file_hash_list__remove(file_hash, file->name);
>> +	ret = get_sdt_note_list(&sdt_notes, file->name);
>> +	if (!ret) {
>> +		/* Add the entry to file hash list */
>> +		ret = file_hash_list__add(&sdt_notes, file, file_hash);
>> +	} else {
>> +		sdt_err(ret, target);
>> +	}
>> +
>> +out:
>> +	free(file->name);
>> +	free(file);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * file_hash_list__flush: Flush the file_hash list to cache
>> + * @file_hash: file_hash list
>> + * @fcache: opened SDT events cache
>> + *
>> + * Iterate through all the entries of @file_hash and flush them
>> + * onto fcache.
>> + * The complete file hash list is flushed into the cache. First
>> + * write the key for non-empty entry and then file entries for that
>> + * key, and then the SDT notes. The delimiter used is ':'.
>> + */
>> +static void file_hash_list__flush(struct hash_list *file_hash,
>> +				  FILE *fcache)
>> +{
>> +	struct file_sdt_ent *file_pos;
>> +	struct list_head *ent_head, *sdt_head;
>> +	struct sdt_note *sdt_ptr;
>> +	int i;
>> +
>> +	/* Go through all entries */
>> +	for (i = 0; i < HASH_TABLE_SIZE; i++) {
>> +		/* Obtain the list head */
>> +		ent_head = &file_hash->ent[i].list;
>> +		/* Empty ? */
>> +		if (list_empty(ent_head))
>> +			continue;
>> +		/* ':' are used here as delimiters */
>> +		fprintf(fcache, "%d:", i);
>> +		list_for_each_entry(file_pos, ent_head, file_list) {
>> +			fprintf(fcache, "%s:", file_pos->name);
>> +			fprintf(fcache, "%s:", file_pos->sbuild_id);
>> +			sdt_head = &file_pos->sdt_list;
>> +			list_for_each_entry(sdt_ptr, sdt_head, note_list) {
>> +				fprintf(fcache, "%s:%s:%lx:%lx:",
>> +					sdt_ptr->provider, sdt_ptr->name,
>> +					sdt_ptr->addr.a64[0],
>> +					sdt_ptr->addr.a64[2]);
>> +			}
>> +			fprintf(fcache, ":");
> Hmm, this format is horrible. Please use more sscanf-readable format, so that
> we can simplify the reader-side code. e.g.
>
> <number>:\n
> <file>:<buildid>\n
> <provider>:<name>:<addr>:<sem>\n
> :\n
> <number>:\n
> ...
> Then, you can use fscanf(), or a combination of fgets() and sscanf().
> We don't need to care about the loading speed at this point.
>

Hmm, will try this.

>> +		}
>> +		/* '-' marks the end of entries for that key */
>> +		fprintf(fcache, "-");
>> +	}
>> +
>> +}
>> +
>> +/**
>> + * flush_hash_list_to_cache: Flush everything from file_hash to disk
>> + * @file_hash: file_hash list
>> + *
>> + * Opens a cache and calls file_hash_list__flush() to dump everything
>> + * on to the cache.
>> + */
>> +static void flush_hash_list_to_cache(struct hash_list *file_hash)
>> +{
>> +	FILE *cache;
>> +	int ret;
>> +	struct stat buf;
>> +
>> +	ret = stat(SDT_CACHE_DIR, &buf);
>> +	if (ret) {
>> +		ret = mkdir(SDT_CACHE_DIR, buf.st_mode);
>> +		if (ret) {
>> +			pr_err("%s : %s\n", SDT_CACHE_DIR, strerror(errno));
>> +			return;
>> +		}
>> +	}
>> +
>> +	cache = fopen(SDT_CACHE_DIR SDT_FILE_CACHE, "w");
>> +	if (!cache) {
>> +		pr_err("Error in creating %s file!\n",
>> +		       SDT_CACHE_DIR SDT_FILE_CACHE);
>> +		return;
>> +	}
>> +
>> +	file_hash_list__flush(file_hash, cache);
>> +	fclose(cache);
>> +}
>> +
>> +/**
>> + * add_sdt_events: Add SDT events
>> + * @arg: filename
>> + *
>> + * Initializes a hash table 'file_hash', calls add_to_hash_list() to add SDT
>> + * events of @arg to the cache and then cleans them up.
>> + * 'file_hash' list is a hash table which maintains all the information
>> + * related to the files with the SDT events in them. The file name serves as the
>> + * key to this hash list. Each entry of the file_hash table is a list head
>> + * which contains a chain of 'file_list' entries. Each 'file_list' entry contains
>> + * the list of SDT events found in that file. This hash serves as a mapping
>> + * from file name to the SDT events. 'file_hash' is used to add/del SDT events
>> + * to the SDT cache.
>> + */
>> +int add_sdt_events(const char *arg)
>> +{
>> +	struct hash_list file_hash;
>> +	int ret;
>> +
>> +	if (!arg) {
>> +		pr_err("Error : File Name must be specified with \"--add\""
>> +		       "option!\n"
>> +	       "Usage :\n  perf sdt-cache --add <file-name>\n");
>> +		return -1;
>> +	}
>> +	/* Initialize the file hash_list */
>> +	file_hash_list__init(&file_hash);
>> +
>> +	/* Try to add the events to the file hash_list */
>> +	ret = add_to_hash_list(&file_hash, arg);
>> +	switch (ret) {
>> +	case MOD:
>> +		/* file hash table is dirty, flush it */
>> +		flush_hash_list_to_cache(&file_hash);
>> +	case NO_MOD:
>> +		/* file hash isn't dirty, do not flush */
>> +		file_hash_list__cleanup(&file_hash);
>> +		ret = 0;
>> +		break;
> Again, MOD and NO_MOD is too general.
>
>> +	default:
>> +		file_hash_list__cleanup(&file_hash);
>> +	}
>> +	return ret;
>> +}
>> diff --git a/tools/perf/util/sdt.h b/tools/perf/util/sdt.h
>> new file mode 100644
>> index 0000000..4ed27d7
>> --- /dev/null
>> +++ b/tools/perf/util/sdt.h
>> @@ -0,0 +1,30 @@
>> +#include "build-id.h"
>> +
>> +#define HASH_TABLE_SIZE 2063
>> +#define SDT_CACHE_DIR "/var/cache/perf/"
>> +#define SDT_FILE_CACHE "perf-sdt-file.cache"
>> +#define SDT_EVENT_CACHE "perf-sdt-event.cache"
> Hmm, this should be treated as same as buildid_dir. Please see util/config.c.

Will look into this. :)

> Thank you,
>
>> +
>> +#define DELIM ':'
>> +#define MAX_ADDR 17
>> +
>> +#define MOD 1
>> +#define NO_MOD 2
>> +
>> +struct file_sdt_ent {
>> +	char name[PATH_MAX];                    /* file name */
>> +	char sbuild_id[BUILD_ID_SIZE * 2 + 1];  /* Build id of the file */
>> +	struct list_head file_list;
>> +	struct list_head sdt_list;              /* SDT notes in this file */
>> +
>> +};
>> +
>> +/* hash table entry */
>> +struct hash_ent {
>> +	struct list_head list;
>> +};
>> +
>> +/* Hash table struct */
>> +struct hash_list {
>> +	struct hash_ent ent[HASH_TABLE_SIZE];
>> +};
>>
>>
>

Thanks a lot for reviewing the patch.

-- 
Thanks,
Hemant Kumar

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists