Re: [PATCH v2 1/3] SDT markers listing by perf:
From: Hemant
Date: Tue Oct 08 2013 - 09:04:50 EST
On 10/08/2013 02:27 PM, Namhyung Kim wrote:
Hi Hemant,
Hi and thanks for reviewing the patches...
On Mon, 07 Oct 2013 12:17:57 +0530, Hemant Kumar wrote:
[...]
+static void cleanup_sdt_note_list(struct list_head *sdt_notes)
+{
+ struct sdt_note *tmp;
+ struct list_head *pos, *s;
+
+ list_for_each_safe(pos, s, sdt_notes) {
+ tmp = list_entry(pos, struct sdt_note, note_list);
You might use list_for_each_entry_safe() instead.
Ok, will use that.
+ list_del(pos);
+ free(tmp->name);
+ free(tmp->provider);
+ free(tmp);
+ }
+}
+
static int convert_to_probe_trace_events(struct perf_probe_event *pev,
struct probe_trace_event **tevs,
int max_tevs, const char *target)
@@ -2372,3 +2386,28 @@ out:
free(name);
return ret;
}
+
+static void display_sdt_note_info(struct list_head *start)
+{
+ struct list_head *pos;
+ struct sdt_note *tmp;
+
+ list_for_each(pos, start) {
+ tmp = list_entry(pos, struct sdt_note, note_list);
Ditto. list_for_each_entry().
Ok...
+ printf("%%%s:%s\n", tmp->provider, tmp->name);
+ }
+}
+
+int show_sdt_notes(const char *target)
+{
+ struct list_head sdt_notes;
+ int ret = -1;
+
+ INIT_LIST_HEAD(&sdt_notes);
You can use LIST_HEAD(sdt_notes) here.
Yes. Will use LIST_HEAD() instead.
+ ret = get_sdt_note_list(&sdt_notes, target);
+ if (!ret) {
+ display_sdt_note_info(&sdt_notes);
+ cleanup_sdt_note_list(&sdt_notes);
+ }
+ return ret;
+}
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index f9f3de8..b8a9347 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -133,7 +133,7 @@ extern int show_available_vars(struct perf_probe_event *pevs, int npevs,
struct strfilter *filter, bool externs);
extern int show_available_funcs(const char *module, struct strfilter *filter,
bool user);
-
+int show_sdt_notes(const char *target);
/* Maximum index number of event-name postfix */
#define MAX_EVENT_INDEX 1024
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 4b12bf8..6b17260 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -846,6 +846,182 @@ out_elf_end:
return err;
}
+/*
+ * Populate the name, type, offset in the SDT note structure and
+ * ignore the argument fields (for now)
+ */
+static struct sdt_note *populate_sdt_note(Elf **elf, const char *data,
+ size_t len, int type)
+{
+ const char *provider, *name;
+ struct sdt_note *note;
+
+ /*
+ * Three addresses need to be obtained :
+ * Marker location, address of base section and semaphore location
+ */
+ union {
+ Elf64_Addr a64[3];
+ Elf32_Addr a32[3];
+ } buf;
+
+ /*
+ * dst and src are required for translation from file to memory
+ * representation
+ */
+ Elf_Data dst = {
+ .d_buf = &buf, .d_type = ELF_T_ADDR, .d_version = EV_CURRENT,
+ .d_size = gelf_fsize((*elf), ELF_T_ADDR, 3, EV_CURRENT),
+ .d_off = 0, .d_align = 0
+ };
+
+ Elf_Data src = {
+ .d_buf = (void *) data, .d_type = ELF_T_ADDR,
+ .d_version = EV_CURRENT, .d_size = dst.d_size, .d_off = 0,
+ .d_align = 0
+ };
+
+ /* Check the type of each of the notes */
+ if (type != SDT_NOTE_TYPE)
+ goto out_ret;
+
+ note = (struct sdt_note *)zalloc(sizeof(struct sdt_note));
+ if (note == NULL) {
+ pr_err("Memory allocation error\n");
+ goto out_ret;
+ }
+ INIT_LIST_HEAD(¬e->note_list);
+
+ if (len < dst.d_size + 3)
+ goto out_free;
+
+ /* Translation from file representation to memory representation */
+ if (gelf_xlatetom(*elf, &dst, &src,
+ elf_getident(*elf, NULL)[EI_DATA]) == NULL)
+ pr_debug("gelf_xlatetom : %s", elf_errmsg(-1));
I doubt this translation is really needed as we only deal with SDTs on a
local system only, right?
In case of SDTs on a local system, we don't need gelf_xlatetom().
+
+ /* Populate the fields of sdt_note */
+ provider = data + dst.d_size;
+
+ name = (const char *)memchr(provider, '\0', data + len - provider);
+ if (name++ == NULL)
+ goto out_free;
+ note->provider = strdup(provider);
+ note->name = strdup(name);
You need to check the return value of strdup's and it should also be
freed when an error occurres after this.
Missed that. Thanks for pointing that out.
[...]
+ /* Get the SDT notes */
+ for (offset = 0; (next = gelf_getnote(data, offset, &nhdr, &name_off,
+ &desc_off)) > 0; offset = next) {
+ if (nhdr.n_namesz == sizeof(SDT_NOTE_NAME) &&
+ !memcmp(data->d_buf + name_off, SDT_NOTE_NAME,
+ sizeof(SDT_NOTE_NAME))) {
+ tmp = populate_sdt_note(&elf, (const char *)
+ ((long)(data->d_buf) +
+ (long)desc_off),
It seems that data->d_buf is type of void *, so these casts can go away,
I guess.
Yeah correct, we don't need these casts.
+ nhdr.n_descsz, nhdr.n_type);
+ if (!note && tmp)
+ note = tmp;
+ else if (tmp)
+ list_add_tail(&tmp->note_list,
+ &(note->note_list));
+ }
+ }
+ if (tmp)
+ return &tmp->note_list;
This list handling code looks very strange to me. Why not just passing
a list head and connect notes to it?
Yes it will be better to use list_head...
+out_err:
+ return NULL;
+}
+
+int get_sdt_note_list(struct list_head *head, const char *target)
+{
+ Elf *elf;
+ int fd, ret = -1;
+ struct list_head *tmp = NULL;
+
+ fd = open(target, O_RDONLY);
+ if (fd < 0) {
+ pr_err("Failed to open %s\n", target);
+ goto out_ret;
+ }
+
+ symbol__elf_init();
+ elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
+ if (!elf) {
+ pr_err("%s: cannot read %s ELF file.\n", __func__, target);
+ goto out_close;
+ }
+ tmp = construct_sdt_notes_list(elf);
+ if (tmp) {
+ list_add(head, tmp);
Look like the params are exchanged?
/**
* list_add - add a new entry
* @new: new entry to be added
* @head: list head to add it after
*
* Insert a new entry after the specified head.
* This is good for implementing stacks.
*/
static inline void list_add(struct list_head *new, struct list_head *head)
{
__list_add(new, head, head->next);
}
I guess they won't be exchanged... tmp will be added to head, right?
Anyway, this won't be needed if we go with list_head in struct
probe_point instead of sdt_note. Will make the required changes.
+ ret = 0;
+ }
+ elf_end(elf);
+
+out_close:
+ close(fd);
+out_ret:
+ return ret;
+}
+
void symbol__elf_init(void)
{
elf_version(EV_CURRENT);
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 5f720dc..037185c 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -197,6 +197,17 @@ struct symsrc {
#endif
};
+struct sdt_note {
+ char *name;
+ char *provider;
+ bool bit32; /* 32 or 64 bit flag */
+ union {
+ Elf64_Addr a64[3];
+ Elf32_Addr a32[3];
+ } addr;
+ struct list_head note_list;
+};
+
void symsrc__destroy(struct symsrc *ss);
int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
enum dso_binary_type type);
@@ -247,4 +258,11 @@ void symbols__fixup_duplicate(struct rb_root *symbols);
void symbols__fixup_end(struct rb_root *symbols);
void __map_groups__fixup_end(struct map_groups *mg, enum map_type type);
+/* Specific to SDT notes */
+int get_sdt_note_list(struct list_head *head, const char *target);
+
+#define SDT_NOTE_TYPE 3
+#define NOTE_SCN ".note.stapsdt"
What about SDT_NOTE_SCN for consistency?
Seems better. Will change to that.
--
Thanks
Hemant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/