This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH v2 1/3] perf/sdt : Listing of SDT markers by perf
- From: Hemant Kumar <hemant at linux dot vnet dot ibm dot com>
- To: Andi Kleen <andi at firstfloor 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, namhyung at kernel 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: Tue, 22 Jul 2014 17:23:51 +0530
- Subject: Re: [PATCH v2 1/3] perf/sdt : Listing of SDT markers by perf
- Authentication-results: sourceware.org; auth=none
- References: <20140717054826 dot 19995 dot 61782 dot stgit at hemant-fedora> <20140717055341 dot 19995 dot 97042 dot stgit at hemant-fedora> <87d2d24kui dot fsf at tassilo dot jf dot intel dot com>
Hi Andi,
On 07/18/2014 11:20 PM, Andi Kleen wrote:
Hemant Kumar <hemant@linux.vnet.ibm.com> writes:
First I should say supporting these probes is very useful. Thanks for
working on this.
Thanks a lot for the appreciation.
+
+#define SDT_CACHE_DIR "/var/cache/perf/"
This requires running perf as root, right?
Yes!
It would be better to use the $HOME cache dir, like the recent JSON patches.
Hmm, seems like a good idea! We can use ~/.debug as Masami suggested.
+#define SDT_CACHE "perf-sdt.cache"
+#define SDT_CACHE_TMP "perf-sdt.cache.tmp"
+
+#define DELIM ':'
+
+struct path_list {
+ char path[PATH_MAX];
+ struct list_head list;
+} execs;
+
+/* Write operation for cache */
+static void write_cache(FILE *cache, char *buffer)
+{
+ fprintf(cache, "%s", buffer);
+}
+
The function seems redundant.
Yeah, right. Will remove it.
+/*
+ * get_sdt_note_info() is the function actually responsible for
+ * flushing the SDT notes info into the "cache" file or to the
+ * stdout if "cache" points to NULL. Also, this function finds out
+ * the build-id of an ELF to be written into "cache".
+ */
+static void get_sdt_note_info(struct list_head *start, const char *target,
+ FILE *cache)
+{
+ struct sdt_note *pos;
+ u8 build_id[BUILD_ID_SIZE];
+ char sbuild_id[BUILD_ID_SIZE * 2 + 1];
+ char buffer[2 * PATH_MAX];
+
+ if (list_empty(start))
+ return;
+
+ /* Read the build id of the file */
+ if (filename__read_build_id(target, &build_id,
+ sizeof(build_id)) < 0) {
+ pr_debug("Couldn't read build-id in %s\n", target);
pr_info ?
Ok, pr_info will be better.
+
+/*
+ * Finds out the libraries present in a system as shown by the command
+ * "ldconfig --print-cache". Uses "=>" and '/' to find out the start of a
+ * dso path.
+ */
This seems like a hack. How would that handle chroot, containers etc. ?
Right now, it doesn't handle chroot, containers and other path related
stuff. We will have to figure that out!
+static inline void append_path(char *path, struct list_head *list)
+{
+ char *res_path = NULL;
+ struct path_list *tmp = NULL;
+
+ res_path = (char *)malloc(sizeof(char) * PATH_MAX);
+
+ if (!res_path)
+ return;
+
+ memset(res_path, '\0', PATH_MAX);
+
+ if (realpath(path, res_path) && !is_present_in_list(list, res_path)) {
O^2 algorithm ?
:) Will improve that by using a hash table.
+/*
+ * Obtain the list of paths from the PATH env variable
+ */
Same as above. This probably needs to be more configurable to handle
more ways to find binaries.
We can make it more configurable, but by default, it should go for
binaries in these directories. What would you suggest?
Thanks a lot for reviewing the patch. :)
--
Thanks,
Hemant Kumar