[3/7] kernel-shark-qt: Add API for loading trace.dat files

Commit Message

Yordan Karadzhov June 25, 2018, 3:01 p.m. UTC

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
an example, demonstrating the usage of this part of the API.

Signed-off-by: Yordan Karadzhov (VMware) <[e-mail]>
---
 kernel-shark-qt/src/CMakeLists.txt |   9 +
 kernel-shark-qt/src/libkshark.c    | 438 +++++++++++++++++++++++++++++
 kernel-shark-qt/src/libkshark.h    | 157 +++++++++++
 3 files changed, 604 insertions(+)
 create mode 100644 kernel-shark-qt/src/libkshark.c
 create mode 100644 kernel-shark-qt/src/libkshark.h

Comments

Steven Rostedt June 25, 2018, 6:30 p.m. UTC | #1

On Mon, 25 Jun 2018 18:01:17 +0300
"Yordan Karadzhov (VMware)" <[e-mail]> 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) <[e-mail]>


> +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 <[e-mail]>
> + */
> +
> + /**
> + *  @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

Yordan Karadzhov June 26, 2018, 2:47 p.m. UTC | #2

On 25.06.2018 21:30, Steven Rostedt wrote:
>> +/** 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.
>   */
> 

If we want to be able to produce Doxygen documentation for the code we 
have to stick to the Doxygen syntax for comments.

>> +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?

I have no experience with "kernel doc", so I have no idea if it will be 
easy or hard. I can try to investigate, but this will take some time.
I this the way to go?

Thanks!
Yordan

Steven Rostedt June 26, 2018, 3:16 p.m. UTC | #3

On Tue, 26 Jun 2018 17:47:13 +0300
"Yordan Karadzhov (VMware)" <[e-mail]> wrote:

> > How hard would it be to switch over?  
> 
> I have no experience with "kernel doc", so I have no idea if it will be 
> easy or hard. I can try to investigate, but this will take some time.
> I this the way to go?

Doing a search, I found this link:

https://groups.google.com/forum/#!topic/jailhouse-dev/mkfNiU3epaY

It's from Jan Kiszka (you may have met him in Prague. He gave one of
the keynotes, and is the author of Jailhouse). Seems he finds
kernel-doc limiting, and prefers doxygen. I respect Jan's opinion in
this (oh, and he was on the Alps with me too). Let's follow the
jailhouse use of doxygen. Would that do?

-- Steve

Yordan Karadzhov June 26, 2018, 3:26 p.m. UTC | #4

On 26.06.2018 18:16, Steven Rostedt wrote:
> On Tue, 26 Jun 2018 17:47:13 +0300
> "Yordan Karadzhov (VMware)" <[e-mail]> wrote:
> 
>>> How hard would it be to switch over?
>>
>> I have no experience with "kernel doc", so I have no idea if it will be
>> easy or hard. I can try to investigate, but this will take some time.
>> I this the way to go?
> 
> Doing a search, I found this link:
> 
> https://groups.google.com/forum/#!topic/jailhouse-dev/mkfNiU3epaY
> 
> It's from Jan Kiszka (you may have met him in Prague. He gave one of
> the keynotes, and is the author of Jailhouse). Seems he finds
> kernel-doc limiting, and prefers doxygen. I respect Jan's opinion in
> this (oh, and he was on the Alps with me too). Let's follow the
> jailhouse use of doxygen. Would that do?
> 

OK, great!
I have been using Doxygen documentation as a user (to learn new 
libraries) so I feel a bit attached to it. It is really useful. That why 
I wanted to use Doxygen to document our code.
Thanks!
Yordan


> -- Steve
>

Patch

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..f112a50
--- /dev/null
+++ b/kernel-shark-qt/src/libkshark.c
@@ -0,0 +1,438 @@ 
+// SPDX-License-Identifier: LGPL-2.1
+
+/*
+ * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <[e-mail]>
+ */
+
+// C
+#define _GNU_SOURCE 1
+#include <stdlib.h>
+#include <stdio.h>
+#include <assert.h>
+
+// 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) {
+		free(*context);
+		*context = NULL;
+	}
+
+	if (kshark_context_handler) {
+		free(kshark_context_handler);
+		kshark_context_handler = NULL;
+	}
+
+	kshark_context_handler = kshark_ctx;
+	*context = kshark_ctx;
+
+	return true;
+}
+
+bool kshark_instance(struct kshark_context **context)
+{
+	if (!seq.buffer)
+		trace_seq_init(&seq);
+
+	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)
+			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;
+		free(task);
+	}
+
+	task = kshark_ctx->tasks = NULL;
+}
+
+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);
+
+	kshark_ctx->handle = handle;
+	kshark_ctx->pevt = 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->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;
+
+	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;
+	}
+
+	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));
+	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->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;
+
+	if (*data_rows)
+		free(*data_rows);
+
+	struct kshark_entry *entry, **next;
+	struct kshark_entry **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) );
+			assert(entry != NULL);
+
+			kshark_set_entry_values(kshark_ctx, rec, entry);
+			kshark_add_task(kshark_ctx, entry->pid);
+
+			entry->next = NULL;
+			next = &(entry->next);
+			free_record(rec);
+
+			++count;
+			rec = tracecmd_read_data(kshark_ctx->handle, cpu);
+		}
+
+		total += count;
+	}
+
+	struct kshark_entry **rows;
+	rows = calloc(total, sizeof(struct kshark_entry *));
+	if(!rows) {
+		fprintf(stderr, "Failed to allocate memory during data loading.\n");
+		return 0;
+	}
+
+	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]->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;
+
+	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);
+
+			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);
+
+			++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;
+	}
+
+	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);
+		}
+
+		++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);
+
+	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)
+		*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);
+
+	trace_seq_reset(&seq);
+
+	char *tmp_str, *entry_str;
+	int size_tmp, size = 0;
+
+	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]";
+	const char *task = pevent_data_comm_from_pid(kshark_ctx->pevt, entry->pid);
+	const char *lat = kshark_get_latency(kshark_ctx->pevt, data);
+
+	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);
+	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 <[e-mail]>
+ */
+
+ /**
+ *  @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
+ * 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.
+ */
+struct kshark_entry {
+	/**
+	 * A bit mask controlling the visibility of the entry. A value of OxFF would mean
+	 * that the entry is visible everywhere.
+	 */
+	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. */
+	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. */
+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.
+
+ */
+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