On Mon, 25 Jun 2018 18:01:17 +0300
"Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> 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 introduces
"will introduce"
> an example, demonstrating the usage of this part of the API.
>
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@xxxxxxxxx>
> +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;
> +
Because in the Linux Kernel development, it is looked down on to add a
check for a ptr having non-null before free, I'll push it here too. As
free(ptr) is basically a nop if ptr == NULL, we like to eliminate if
statements checking for NULL unless they are really necessary.
To get you use to this mindset, I'll suggest here instead of:
> + /* Free the existing context (if any). */
> + if (*context && *context != kshark_context_handler) {
> + free(*context);
> + *context = NULL;
> + }
> +
> + if (kshark_context_handler) {
> + free(kshark_context_handler);
> + kshark_context_handler = NULL;
> + }
> +
> + kshark_context_handler = kshark_ctx;
> + *context = kshark_ctx;
Do:
if (*context != kshark_context_handler) {
free(*context);
*context = NULL;
}
free(kshark_context_handler);
kshark_context_handle = kshark_ctx;
> +
> + return true;
> +}
> +
> +bool kshark_instance(struct kshark_context **context)
> +{
> + if (!seq.buffer)
> + trace_seq_init(&seq);
May need to add here:
if (!seq.buffer)
return false;
}
As the trace_seq_init() can fail. Hmm, I wonder if I should change that
to have a return status instead of just returning void.
> +
> + if (*context == NULL && kshark_context_handler == NULL) {
> + // No kshark_context exists. Create a default one.
BTW, for C files, please use the /* */ comment styles.
Only the SPDX gets to use '//'.
> + 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)
No need for the if statement.
> + 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;
> + }
> +
> + return true;
> +}
> +
> +static void kshark_free_task_list(struct kshark_context *kshark_ctx)
> +{
> + struct kshark_task_list *task;
> +
> + while (kshark_ctx->tasks) {
> + task = kshark_ctx->tasks;
> + kshark_ctx->tasks = kshark_ctx->tasks->next;
You don't really need to change this, but I'm lazy, and like to type
less. So I would have written the above as:
kshark_ctx->tasks = task->next;
> + free(task);
> + }
> +
> + task = kshark_ctx->tasks = NULL;
Why the assignment of "task" here? It's no longer used.
> +}
> +
> +bool kshark_open(struct kshark_context *kshark_ctx, const char *file)
> +{
> + kshark_free_task_list(kshark_ctx);
> + struct tracecmd_input *handle = tracecmd_open(file);
> + if (!handle)
> + return false;
> +
> + pthread_mutex_init(&kshark_ctx->input_mutex, NULL);
pthread_mutex_init() may fail. Need to handle that case.
> +
> + kshark_ctx->handle = handle;
> + kshark_ctx->pevt = tracecmd_get_pevent(handle);
Let's be consistent, and use "pevent" as the field name here, instead
of "pevt". In the near future, we will be converting "pevent" to "tep",
and it would be good to do a global search and replace for that.
> +
> + /*
> + * 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->pevt = 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;
> + }
> +
> + kshark_free_task_list(kshark_ctx);
> +
> + if (seq.buffer)
> + trace_seq_destroy(&seq);
> +
> + free(kshark_ctx);
> +
> + if (kshark_ctx == kshark_context_handler)
> + kshark_context_handler = NULL;
Move the free(kshark_ctx) here. I understand that we are only comparing
the value of the pointer, but it's considered cleaner if you don't use
a pointer that you freed, even if you are not dereferencing it. It's
also best to keep a free(x); x = NULL; together. On separate lines, of
course.
> +
> + kshark_ctx = NULL;
> +}
> +
> +static struct kshark_task_list *
> +kshark_find_task(struct kshark_context *kshark_ctx, int pid)
> +{
> + struct kshark_task_list *list = kshark_ctx->tasks;
> + while (list) {
> + if (list->pid == pid)
> + return list;
> +
> + list = list->next;
> + }
Could you convert this to a for loop?
for (list = kshark_ctx->tasks; list; list = list->next)
if (list->pid == pid)
return list;
> +
> + 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));
Need to check if list == NULL 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
I know it's rather a pain, but please use /* */ comment style here.
> + 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->pevt, 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->pevt, 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;
> + size_t count, total = 0;
> + struct pevent_record *rec;
Please get into the habit of "upside-down-xmas-tree" style of arranging
of declarations. Also, let's keep to the old C style (as it is required
by the Linux kernel too and not do declarations after code.
int n_cpus = tracecmd_cpus(kshark_ctx->handle);
struct kshark_entry *entry, **next;
struct kshark_entry **cpu_list;
struct pevent_record *rec;
size_t count, total = 0;
int cpu;
> +
> + if (*data_rows)
> + free(*data_rows);
> +
> + struct kshark_entry *entry, **next;
Move this declaration above (as noted).
> + struct kshark_entry **cpu_list = calloc(n_cpus, sizeof(struct kshark_entry *));
Just use assignment, without the declaration.
Need to test if the calloc() passed.
> +
> + 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) {
Probably could make this a for loop:
for (rec = tracecmd_read_cpu_first(kshark_ctx->handle, cpu),
rec,
rec = tracecmd_read_data(kshark_ctx->handle, cpu)) {
Or maybe not ;-)
> + *next = entry = malloc( sizeof(struct kshark_entry) );
> + assert(entry != NULL);
> +
> + kshark_set_entry_values(kshark_ctx, rec, entry);
> + kshark_add_task(kshark_ctx, entry->pid);
> +
> + entry->next = NULL;
> + next = &(entry->next);
Parenthesis are not needed: next = &entry->next;
> + free_record(rec);
> +
> + ++count;
> + rec = tracecmd_read_data(kshark_ctx->handle, cpu);
> + }
> +
> + total += count;
> + }
> +
> + struct kshark_entry **rows;
Move the declaration to the top of the function. I really wish that C
and C++ never allowed this "feature".
> + rows = calloc(total, sizeof(struct kshark_entry *));
> + if(!rows) {
Add a space between "if" and "("
> + fprintf(stderr, "Failed to allocate memory during data loading.\n");
We fail safely here on allocation, but not above with the assert?
Should be be consistent. Also, I think we need a function for errors
that can be overwritten. "warning()" I believe already does that, and
allows for nice pop-up errors.
> + return 0;
> + }
> +
> + count = 0;
> + int next_cpu;
> + uint64_t ts;
Again, please keep all declarations at the top of the function.
> + 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;
> +}
> +
> +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 cpu;
> + size_t count, total = 0;
> + struct pevent_record *data;
Again, "upside-down-xmas-tree" style and move all declarations to up
here.
> +
> + 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));
> + assert(temp_rec != NULL);
Again, should we just assert, or make it fail nicely?
BTW, "goto" is your friend in these cases ;-)
> +
> + kshark_add_task(kshark_ctx,
> + pevent_data_pid(kshark_ctx->pevt, data));
> + temp_rec->rec = data;
> + temp_rec->next = NULL;
> + temp_next = &(temp_rec->next);
Again, no need for the parenthesis. The &temp_rec->next is common
nomenclature for this type of construct.
> +
> + ++count;
> + data = tracecmd_read_data(kshark_ctx->handle, cpu);
> + }
> +
> + total += count;
> + }
> +
> + struct pevent_record **rows;
> + rows = calloc(total, sizeof(struct pevent_record *));
> + if(!rows) {
> + fprintf(stderr, "Failed to allocate memory during data loading.\n");
> + return 0;
> + }
> +
The below is awfully similar to the end of kshark_load_data_entries(),
I wonder if we could use the same code.
> + count = 0;
> + int next_cpu;
> + uint64_t ts;
> + 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);
We could move the freeing of cpu_list content out of this code (do it
separately), and then we can make a helper function to do the row
assignments that both functions use.
> + }
> +
> + ++count;
> + }
> +
> + free(cpu_list);
> + *data_rows = rows;
> + return total;
> +}
> +
> +static struct pevent_record *kshark_read_at(struct kshark_context *kshark_ctx,
> + uint64_t offset)
> +{
> + pthread_mutex_lock(&kshark_ctx->input_mutex);
I'm curious, what is this lock protecting?
> +
> + 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)
> +{
> + if (!record || !event)
> + return NULL;
> +
> + trace_seq_reset(&seq);
> + pevent_event_info(&seq, event, record);
> +
> + // Remove the trailing newline from the Info string.
> + char *pos;
> + if ((pos = strchr(seq.buffer, '\n')) != NULL)
This does not actually do what the comment says. It removes the first
'\n' from the Info string, not the last one.
> + *pos = '\0';
> +
> + return seq.buffer;
> +}
> +
> +char* kshark_dump_entry(struct kshark_entry *entry)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> + kshark_instance(&kshark_ctx);
> +
> + if (!seq.buffer)
> + trace_seq_init(&seq);
Need to check the result of trace_seq_init().
> +
> + trace_seq_reset(&seq);
Why reset here? You also reset it within kshark_get_latency().
> +
> + char *tmp_str, *entry_str;
> + int size_tmp, size = 0;
Please move these to the top of the function. Also, there's no reason
to make two "size" variables. You can use size for the size_tmp and it
won't affect the code.
> +
> + struct pevent_record *data = kshark_read_at(kshark_ctx, entry->offset);
> +
> + int event_id = pevent_data_type(kshark_ctx->pevt, data);
> + struct event_format *event =
> + pevent_data_event_from_type(kshark_ctx->pevt, event_id);
> +
> + const char *event_name = (event)? event->name : "[UNKNOWN EVENT]";
No need for the parenthesis around "event".
> + const char *task = pevent_data_comm_from_pid(kshark_ctx->pevt, entry->pid);
> + const char *lat = kshark_get_latency(kshark_ctx->pevt, data);
And separate these as declarations and assignments.
> +
> + size_tmp = asprintf(&tmp_str, "%li %s-%i; CPU %i; %s;",
> + entry->ts,
> + task,
> + entry->pid,
> + entry->cpu,
> + lat);
> +
> + const char *info = kshark_get_info(kshark_ctx->pevt, data, event);
Where is "info" used?
> + if (size_tmp) {
> + 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..d6e41bb
> --- /dev/null
> +++ b/kernel-shark-qt/src/libkshark.h
> @@ -0,0 +1,157 @@
> +/* SPDX-License-Identifier: LGPL-2.1 */
> +
> +/*
> + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@xxxxxxxxx>
> + */
> +
> + /**
> + * @file libkshark.h
> + * @brief API for processing of FTRACE (trace-cmd) data.
> + */
> +
> +#ifndef _LIB_KSHARK_H
> +#define _LIB_KSHARK_H
> +
> +// C
> +#include <stdint.h>
> +#include <pthread.h>
> +
> +#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
^
Remove the comma.
> + * 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 treace file.
> + */
Also, keep descriptions like the above <= 80 characters wide.
> +struct kshark_entry {
> + /**
You don't need the double star within here. If you do this, (at least
for the recommended way in the Linux kernel), then you should add all
the comments about the variables above the struct.
> + * A bit mask controlling the visibility of the entry. A value of OxFF would mean
> + * that the entry is visible everywhere.
And here too (80 chars)
I'm not too strict on 80 char max width, but the Linux kernel folks are
somewhat. These comments definitely should fit within an 80 character
screen.
As for the bitmask, what other values are acceptable?
> + */
> + 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 treace file, used to find the record. */
Typo "treace".
> + 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.
> + */
> + uint64_t ts;
> +
> + /** Pointer to the next (in time) kshark_entry on the same CPU core. */
> + struct kshark_entry *next;
> +};
> +
> +/** Linked list of tasks. */
Note, the Linux kernel doc way of doing these types of comments is like
this:
/**
* kshark_task_list - Linked list of tasks
* @next: Pointer to the text task's PID
* @pid: PID of a task.
*/
> +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 *pevt;
> +
> + /** 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.
Looks like you have a different set of commands for the doxygen
comments than the kernel uses. Of course, the kernel may be using
"kernel doc" format. I'm not an expert on this. But I would prefer to
keep it more like what the kernel does.
How hard would it be to switch over?
-- Steve
> +
> + */
> +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.
> + */
> +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.
> + */
> +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.
> + */
> +void kshark_free(struct kshark_context *kshark_ctx);
> +
> +/**
> + * @brief Dump into a sting the content of one entry. The function allocates a null
> + * terminated string and return a pointer to this string. The user has 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
|