From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.99]:60456 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966732AbeF1Vza (ORCPT ); Thu, 28 Jun 2018 17:55:30 -0400 Date: Thu, 28 Jun 2018 17:55:27 -0400 From: Steven Rostedt To: "Yordan Karadzhov (VMware)" Cc: linux-trace-devel@vger.kernel.org Subject: Re: [PATCH v2 3/9] kernel-shark-qt: Add API for loading trace.dat files Message-ID: <20180628175527.77beb480@gandalf.local.home> In-Reply-To: <20180628163012.21477-4-y.karadz@gmail.com> References: <20180628163012.21477-1-y.karadz@gmail.com> <20180628163012.21477-4-y.karadz@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8BIT Sender: linux-trace-devel-owner@vger.kernel.org List-ID: On Thu, 28 Jun 2018 19:30:06 +0300 "Yordan Karadzhov (VMware)" wrote: > This patch introduces the first component of the C API used > by the new Qt-based version of KernelShark. The patch includes > only the part of the API responsible for loading data files > generated by trace-cmd. The following patch will introduce > an example, demonstrating the usage of this part of the API. > > Signed-off-by: Yordan Karadzhov (VMware) > --- > kernel-shark-qt/src/CMakeLists.txt | 9 + > kernel-shark-qt/src/libkshark.c | 475 +++++++++++++++++++++++++++++ > kernel-shark-qt/src/libkshark.h | 161 ++++++++++ > 3 files changed, 645 insertions(+) > create mode 100644 kernel-shark-qt/src/libkshark.c > create mode 100644 kernel-shark-qt/src/libkshark.h > > diff --git a/kernel-shark-qt/src/CMakeLists.txt b/kernel-shark-qt/src/CMakeLists.txt > index 8c66424..ed3c60e 100644 > --- a/kernel-shark-qt/src/CMakeLists.txt > +++ b/kernel-shark-qt/src/CMakeLists.txt > @@ -1,4 +1,13 @@ > message("\n src ...") > > +message(STATUS "libkshark") > +add_library(kshark SHARED libkshark.c) > + > +target_link_libraries(kshark ${CMAKE_DL_LIBS} > + ${TRACEEVENT_LIBRARY} > + ${TRACECMD_LIBRARY}) > + > +set_target_properties(kshark PROPERTIES SUFFIX ".so.${KS_VERSION_STRING}") > + > configure_file( ${KS_DIR}/build/deff.h.cmake > ${KS_DIR}/src/KsDeff.h) > diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c > new file mode 100644 > index 0000000..8f4059a > --- /dev/null > +++ b/kernel-shark-qt/src/libkshark.c > @@ -0,0 +1,475 @@ > +// SPDX-License-Identifier: LGPL-2.1 > + > +/* > + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov > + */ > + > + /** > + * @file libkshark.c > + * @brief API for processing of FTRACE (trace-cmd) data. > + */ > + > +/** Use GNU C Library. */ > +#define _GNU_SOURCE 1 > + > +// C > +#include > +#include > +#include > + > +// trace-cmd > +#include "trace-filter-hash.h" > + > +// KernelShark > +#include "libkshark.h" > + > +static __thread struct trace_seq seq; > + > +static struct kshark_context *kshark_context_handler = NULL; > + > +static bool kshark_default_context(struct kshark_context **context) > +{ > + struct kshark_context *kshark_ctx; > + > + kshark_ctx = calloc(1, sizeof(*kshark_ctx)); > + if (!kshark_ctx) > + return false; > + > + /* Free the existing context (if any). */ > + if (*context && *context != kshark_context_handler) { > + kshark_free(*context); > + *context = NULL; > + } > + > + if (kshark_context_handler) { > + kshark_free(kshark_context_handler); > + kshark_context_handler = NULL; If kshark_context_handler is passed into kshark_free(), then kshark_free() already sets kshark_context_handler to NULL, so this isn't needed. > + } > + > + kshark_context_handler = kshark_ctx; > + *context = kshark_ctx; In fact, I think we can simplify this a bit by doing: { struct kshark_context *kshark_ctx; kshark_ctx = calloc(1, sizeof(*kshark_ctx); if (!kshark_ctx) return false; /* Will free kshark_context_handler */ kshark_free(NULL); /* Will do nothing if *context is NULL */ kshark_free(*context); *context = kshark_context_handler = kshark_ctx; > + > + return true; > +} > + > +bool kshark_instance(struct kshark_context **context) > +{ > + if (*context == NULL && kshark_context_handler == NULL) { > + /* No kshark_context exists. Create a default one. */ > + bool status = kshark_default_context(context); > + if (status) > + return status; > + } else if (*context != NULL) { > + /* Use the context provided by the user. */ > + if (kshark_context_handler) > + kshark_free(kshark_context_handler); > + > + kshark_context_handler = *context; > + } else { > + /* > + * No context is provided by the user, but the context handler > + * is already set. Use the context handler. > + */ > + *context = kshark_context_handler; > + } > + > + if (!seq.buffer) > + trace_seq_init(&seq); > + > + if (!seq.buffer) > + return false; > + > + return true; I think the above can be simplified as: bool status = true; if (*context != NULL) { /* Will free kshark_context_handler */ kshark_free(NULL); kshark_context_handler = *context; } else { if (kshark_context_handler) *context = kshark_context_handler; else status = kshark_default_context(context); } if (!seq.buffer) trace_seq_init(&seq); if (!seq.buffer) return false; return status; > +} > + > +static void kshark_free_task_list(struct kshark_context *kshark_ctx) > +{ > + struct kshark_task_list *task; > + As a fail-safe, might want to add: if (!kshark_ctx) return; To allow this to be called with NULL. > + while (kshark_ctx->tasks) { > + task = kshark_ctx->tasks; > + kshark_ctx->tasks = task->next; > + free(task); > + } > + > + kshark_ctx->tasks = NULL; > +} > + > +bool kshark_open(struct kshark_context *kshark_ctx, const char *file) > +{ Please move the declaration of handle to here: struct tracecmd_input *handle; > + kshark_free_task_list(kshark_ctx); > + struct tracecmd_input *handle = tracecmd_open(file); handle = tracecmd_open(file); > + if (!handle) > + return false; > + > + if (pthread_mutex_init(&kshark_ctx->input_mutex, NULL) != 0) { > + tracecmd_close(handle); > + return false; > + } > + > + kshark_ctx->handle = handle; > + kshark_ctx->pevent = tracecmd_get_pevent(handle); > + > + /* > + * Turn off function trace indent and turn on show parent > + * if possible. > + */ > + trace_util_add_option("ftrace:parent", "1"); > + trace_util_add_option("ftrace:indent", "0"); > + > + return true; > +} > + > +void kshark_close(struct kshark_context *kshark_ctx) > +{ > + if (!kshark_ctx || !kshark_ctx->handle) > + return; > + > + tracecmd_close(kshark_ctx->handle); > + kshark_ctx->handle = NULL; > + kshark_ctx->pevent = NULL; > + > + pthread_mutex_destroy(&kshark_ctx->input_mutex); > +} > + > +void kshark_free(struct kshark_context *kshark_ctx) > +{ > + if (kshark_ctx == NULL && kshark_context_handler == NULL) > + return; > + > + if (kshark_ctx == NULL) { > + kshark_ctx = kshark_context_handler; > + kshark_context_handler = NULL; > + } To understand the above better, consolidate it to: if (kshark_ctx == NULL) { if (kshark_context_handler == NULL) return; kshark_ctx = kshark_context_handler; /* kshark_ctx_handler will be set to NULL below */ } > + > + kshark_free_task_list(kshark_ctx); > + > + if (seq.buffer) > + trace_seq_destroy(&seq); > + > + if (kshark_ctx == kshark_context_handler) > + kshark_context_handler = NULL; > + > + free(kshark_ctx); > +} > + > +static struct kshark_task_list * > +kshark_find_task(struct kshark_context *kshark_ctx, int pid) > +{ > + struct kshark_task_list *list; > + for (list = kshark_ctx->tasks; list; list = list->next) > + if (list->pid == pid) > + return list; I wonder if we should make the above into a sorted array, and use sort/bsearch to find it. After hitting my own issue with a linear search algorithm, I'm a bit weary of this type of look up. > + > + return NULL; > +} > + > +static struct kshark_task_list * > +kshark_add_task(struct kshark_context *kshark_ctx, int pid) > +{ > + struct kshark_task_list *list = kshark_find_task(kshark_ctx, pid); > + if (list) > + return list; > + > + list = malloc(sizeof(*list)); > + if (!list) > + return NULL; Yeah, let's make the kshark_ctx->tasks into an array and just use realloc() to increase it here. > + > + list->pid = pid; > + list->next = kshark_ctx->tasks; > + kshark_ctx->tasks = list; > + > + return list; > +} > + > +static void kshark_set_entry_values(struct kshark_context *kshark_ctx, > + struct pevent_record *record, > + struct kshark_entry *entry) > +{ > + /* Offset of the record */ > + entry->offset = record->offset; > + > + /* CPU Id of the record */ > + entry->cpu = record->cpu; > + > + /* Time stamp of the record */ > + entry->ts = record->ts; > + > + /* Event Id of the record */ > + entry->event_id = pevent_data_type(kshark_ctx->pevent, record); > + > + /* > + * Is visible mask. This default value means that the entry > + * is visible everywhere. > + */ > + entry->visible = 0xFF; > + > + /* Process Id of the record */ > + entry->pid = pevent_data_pid(kshark_ctx->pevent, record); > +} > + > +size_t kshark_load_data_entries(struct kshark_context *kshark_ctx, > + struct kshark_entry ***data_rows) > +{ > + int n_cpus = tracecmd_cpus(kshark_ctx->handle); > + int cpu, next_cpu; > + size_t count, total = 0; > + uint64_t ts; > + > + struct pevent_record *rec; > + struct kshark_entry *entry, **next; > + struct kshark_entry **cpu_list, **rows; > + struct kshark_task_list *task; > + > + if (*data_rows) > + free(*data_rows); > + > + cpu_list = calloc(n_cpus, sizeof(struct kshark_entry *)); > + > + for (cpu = 0; cpu < n_cpus; ++cpu) { > + count = 0; > + cpu_list[cpu] = NULL; > + next = &cpu_list[cpu]; > + > + rec = tracecmd_read_cpu_first(kshark_ctx->handle, cpu); > + while (rec) { > + *next = entry = malloc(sizeof(struct kshark_entry)); > + if (!entry) > + goto fail; > + > + kshark_set_entry_values(kshark_ctx, rec, entry); > + task = kshark_add_task(kshark_ctx, entry->pid); > + if (!task) > + goto fail; > + > + entry->next = NULL; > + next = &entry->next; > + free_record(rec); > + > + ++count; > + rec = tracecmd_read_data(kshark_ctx->handle, cpu); > + } > + > + total += count; > + } > + > + rows = calloc(total, sizeof(struct kshark_entry *)); > + if (!rows) > + goto fail; > + > + count = 0; > + while (count < total) { > + ts = 0; > + next_cpu = -1; > + for (cpu = 0; cpu < n_cpus; ++cpu) { > + if (!cpu_list[cpu]) > + continue; > + > + if (!ts || cpu_list[cpu]->ts < ts) { > + ts = cpu_list[cpu]->ts; > + next_cpu = cpu; > + } > + } > + > + if (next_cpu >= 0) { > + rows[count] = cpu_list[next_cpu]; > + cpu_list[next_cpu] = cpu_list[next_cpu]->next; > + } > + ++count; > + } > + > + free(cpu_list); > + *data_rows = rows; > + return total; > + > +fail: > + fprintf(stderr, "Failed to allocate memory during data loading.\n"); > + return 0; > +} > + > +size_t kshark_load_data_records(struct kshark_context *kshark_ctx, > + struct pevent_record ***data_rows) > +{ > + int n_cpus = tracecmd_cpus(kshark_ctx->handle); > + int pid, cpu, next_cpu; > + size_t count, total = 0; > + uint64_t ts; > + > + struct pevent_record *data; > + struct pevent_record **rows; > + struct kshark_task_list *task; > + > + struct temp { > + struct pevent_record *rec; > + struct temp *next; > + } **cpu_list, **temp_next, *temp_rec; > + > + cpu_list = calloc(n_cpus, sizeof(struct temp *)); > + > + for (cpu = 0; cpu < n_cpus; ++cpu) { > + count = 0; > + cpu_list[cpu] = NULL; > + temp_next = &cpu_list[cpu]; > + > + data = tracecmd_read_cpu_first(kshark_ctx->handle, cpu); > + while (data) { > + *temp_next = temp_rec = malloc(sizeof(*temp_rec)); > + if (!temp_rec) > + goto fail; > + > + pid = pevent_data_pid(kshark_ctx->pevent, data); > + task = kshark_add_task(kshark_ctx, pid); > + if (!task) > + goto fail; > + > + temp_rec->rec = data; > + temp_rec->next = NULL; > + temp_next = &(temp_rec->next); > + > + ++count; > + data = tracecmd_read_data(kshark_ctx->handle, cpu); > + } > + > + total += count; > + } > + > + rows = calloc(total, sizeof(struct pevent_record *)); > + if (!rows) > + goto fail; > + > + count = 0; > + while (count < total) { > + ts = 0; > + next_cpu = -1; > + for (cpu = 0; cpu < n_cpus; ++cpu) { > + if (!cpu_list[cpu]) > + continue; > + > + if (!ts || cpu_list[cpu]->rec->ts < ts) { > + ts = cpu_list[cpu]->rec->ts; > + next_cpu = cpu; > + } > + } > + > + if (next_cpu >= 0) { > + rows[count] = cpu_list[next_cpu]->rec; > + temp_rec = cpu_list[next_cpu]; > + cpu_list[next_cpu] = cpu_list[next_cpu]->next; > + free (temp_rec); > + } > + > + ++count; > + } > + > + free(cpu_list); > + *data_rows = rows; > + return total; > + > +fail: > + fprintf(stderr, "Failed to allocate memory during data loading.\n"); > + return 0; > +} > + > +static struct pevent_record *kshark_read_at(struct kshark_context *kshark_ctx, > + uint64_t offset) > +{ > + /* > + * It turns that tracecmd_read_at() is not thread-safe. > + * TODO: Understand why and see if this can be fixed. > + * For the time being use a mutex to protect the access. > + */ Doing a quick look, it's because it does things in states. That is, it calls one function that will set up state in the handle (peek_event), which saves a record, and then tracecmd_read_data() which then uses that record stored in the handle to retrieve the data. All access to the handle, should be protected by the input mutex. > + pthread_mutex_lock(&kshark_ctx->input_mutex); > + > + struct pevent_record *data = tracecmd_read_at(kshark_ctx->handle, > + offset, NULL); > + > + pthread_mutex_unlock(&kshark_ctx->input_mutex); > + > + return data; > +} > + > +static const char *kshark_get_latency(struct pevent *pe, > + struct pevent_record *record) > +{ > + if (!record) > + return NULL; > + > + trace_seq_reset(&seq); > + pevent_data_lat_fmt(pe, &seq, record); > + return seq.buffer; > +} > + > +static const char *kshark_get_info(struct pevent *pe, > + struct pevent_record *record, > + struct event_format *event) > +{ > + char *pos; > + > + if (!record || !event) > + return NULL; > + > + trace_seq_reset(&seq); > + pevent_event_info(&seq, event, record); > + > + /* > + * The event info string contains a trailing newline. > + * Remove this new line. > + */ > + if ((pos = strchr(seq.buffer, '\n')) != NULL) > + *pos = '\0'; > + > + return seq.buffer; > +} > + > +char* kshark_dump_entry(struct kshark_entry *entry) > +{ > + struct pevent_record *data; > + struct event_format *event; > + > + const char *event_name, *task, *lat, *info; > + char *tmp_str, *entry_str; > + int event_id, size = 0; > + > + struct kshark_context *kshark_ctx = NULL; > + kshark_instance(&kshark_ctx); > + > + if (!seq.buffer) > + trace_seq_init(&seq); > + > + if (!seq.buffer) > + return NULL; Since the above double test is done a few times, probably best to make helper function: bool init_thread_seq(void) { if (!seq.buffer) trace_seq_init(&seq); return seq.buffer != NULL; } Then you can just do: if (!init_thread_seq()) return NULL; > + > + data = kshark_read_at(kshark_ctx, entry->offset); Does this access need to be protected by the mutex? > + > + event_id = pevent_data_type(kshark_ctx->pevent, data); > + event = pevent_data_event_from_type(kshark_ctx->pevent, event_id); > + > + event_name = event? event->name : "[UNKNOWN EVENT]"; > + task = pevent_data_comm_from_pid(kshark_ctx->pevent, entry->pid); > + lat = kshark_get_latency(kshark_ctx->pevent, data); > + > + size = asprintf(&tmp_str, "%li %s-%i; CPU %i; %s;", > + entry->ts, > + task, > + entry->pid, > + entry->cpu, > + lat); > + > + info = kshark_get_info(kshark_ctx->pevent, data, event); > + if (size > 0) { > + size = asprintf(&entry_str, "%s %s; %s; 0x%x", > + tmp_str, > + event_name, > + info, > + entry->visible); > + > + free(tmp_str); > + } > + > + free_record(data); > + > + if (size > 0) > + return entry_str; > + > + return NULL; > +} > diff --git a/kernel-shark-qt/src/libkshark.h > b/kernel-shark-qt/src/libkshark.h new file mode 100644 > index 0000000..da5359a > --- /dev/null > +++ b/kernel-shark-qt/src/libkshark.h > @@ -0,0 +1,161 @@ > +/* SPDX-License-Identifier: LGPL-2.1 */ > + > +/* > + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov > > + */ > + > + /** > + * @file libkshark.h > + * @brief API for processing of FTRACE (trace-cmd) data. > + */ > + > +#ifndef _LIB_KSHARK_H > +#define _LIB_KSHARK_H > + > +// C > +#include > +#include > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +// trace-cmd > +#include "trace-cmd.h" > +#include "event-parse.h" > + > +/** > + * Kernel Shark entry contains all information from one trace record needed > + * in order to visualize the time-series of trace records. The part of the > + * data which is not directly required for the visualization (latency, record > + * info etc.) is available on-demand via the offset into the trace file. > + */ > +struct kshark_entry { > + /** > + * A bit mask controlling the visibility of the entry. A value of OxFF > + * would mean that the entry is visible everywhere. Is there a comment somewhere describing what other values mean? > + */ > + uint8_t visible; > + > + /** The CPU core of the record. */ > + uint8_t cpu; > + > + /** The PID of the task the record was generated. */ > + int16_t pid; > + > + /** Unique Id ot the trace event type. */ > + int event_id; > + > + /** The offset into the trace file, used to find the record. */ > + uint64_t offset; > + > + /** > + * The time of the record in nano seconds. The value is taken from > + * the timestamps within the trace data file, which are architecture > + * dependent. The time usually is the timestamp from when the system > + * started. You don't need to change the comment, but FYI, the default of a trace-cmd record is usually the time from when the system started, but it will be a different default when we get things like the virtserver working. > + */ > + uint64_t ts; > + > + /** Pointer to the next (in time) kshark_entry on the same CPU core. */ > + struct kshark_entry *next; > +}; > + > +/** Linked list of tasks. */ > +struct kshark_task_list { > + /** Pointer to the next task's PID. */ > + struct kshark_task_list *next; > + > + /** PID of a task. */ > + int pid; > +}; > + > +/** Structure representing a kshark session. */ > +struct kshark_context { > + /** Input handle for the trace data file. */ > + struct tracecmd_input *handle; > + > + /** Page event used to parse the page. */ > + struct pevent *pevent; > + > + /** List of task's PIDs. */ > + struct kshark_task_list *tasks; > + > + /** A mutex, used to protect the access to the input file. */ > + pthread_mutex_t input_mutex; > +}; > + > +/** > + * @brief Initialize a kshark session. This function must be called before > + * calling any other kshark function. If the session has been initialized, > + * this function can be used to obtain the session's context. > + * @param kshark_ctx: Optional input/output location for context pointer. > + * Only valid on return true. > + * @returns true on success, or false on failure. BTW, to make the above easier to read, can we add indentation? Like this: * @brief Initialize a kshark session. This function must be called before * calling any other kshark function. If the session has been initialized, * this function can be used to obtain the session's context. * @param kshark_ctx: Optional input/output location for context pointer. * Only valid on return true. * @returns true on success, or false on failure. ? Also, probably want to specify exactly how the session is obtained. That is: * @param kshark_ctx: Optional input/output location for context pointer. * If it points to a context, that context will become * the new session. If it points to NULL, it will obtain * the current (or new) session. The result is only * valid on return of true. Finally, another reason to keep the comments with the code. They are less likely to become stale. I'm finding that I'm jumping back and forth from the C file to here to see if the comments match the code. >>From my experience, the farther the comments are from the code they describe, the more likely they never get updated when the code does. > + > + */ > +bool kshark_instance(struct kshark_context **kshark_ctx); > + > +/** > + * @brief Open and prepare for reading a trace data file specified by "file". > + * If the specified file does not exist, or contains no trace data, the > + * function returns false. > + * @param kshark_ctx: Input location for context pointer. > + * @param file: The file to load. > + * @returns true on success, or false on failure. > + */ > +bool kshark_open(struct kshark_context *kshark_ctx, const char *file); > + > +/** > + * @brief Load the content of the trace data file into an array of > + * kshark_entries. This function provides fast loading, however the "latency" > + * and the "info" fields can be accessed only via the offset into the file. > + * This makes the access to these two fields much slower. > + * @param kshark_ctx: Input location for context pointer. > + * @param data_rows: Output location for the trace data. The user is > + * responsible for freeing the elements of the outputted array. > + * @returns The size of the outputted data. I wonder if we should have it return negative on error, as zero may be a valid return value, if the kshark_ctx does not have any rows. > + */ > +size_t kshark_load_data_entries(struct kshark_context *kshark_ctx, > + struct kshark_entry ***data_rows); > + > +/** > + * @brief Load the content of the trace data file into an array of > + * pevent_records. Use this function only if you need fast access to all > + * fields of the record. > + * @param kshark_ctx: Input location for the session context pointer. > + * @param data_rows: Output location for the trace data. Use free_record() > + * to free the elements of the outputted array. > + * @returns The size of the outputted data. Same here. > + */ > +size_t kshark_load_data_records(struct kshark_context *kshark_ctx, > + struct pevent_record ***data_rows); > + > +/** > + * @brief Close the trace data file and free the trace data handle. > + * @param kshark_ctx: Input location for the session context pointer. > + */ > +void kshark_close(struct kshark_context *kshark_ctx); > + > +/** > + * @brief Deinitialize kshark session. Should be called after closing all > + * open trace data files and before your application terminates. > + * @param kshark_ctx: Optional input location for session context pointer. > + */ This does not describe what it does if kshark_ctx is NULL. > +void kshark_free(struct kshark_context *kshark_ctx); > + > +/** > + * @brief Dump into a sting the content of one entry. The function allocates Does the shark dump into a sting ray? (I think you meant string ;-) > + * a null terminated string and return a pointer to this string. The user has "and returns a pointer" -- Steve > + * to free the returned string. > + * @param entry: A Kernel Shark entry to be printed. > + * @returns The returned string contains a semicolon-separated list of data > + * fields. > + */ > +char* kshark_dump_entry(struct kshark_entry *entry); > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif // _LIB_KSHARK_H