[PATCH v4 0/7] Add infrastructure for plugins.

[PATCH v4 0/7] Add infrastructure for plugins.

[Yordan Karadzhov (VMware)]

The infrastructure for plugins used by the Qt-based version of KernelShark
is introduced in this series of patches. This is the last major component
of the C API. The last patch adds a plugin for dealing with "sched" events.

This is version 4 of this series of patches. The major changes from v3 are
in [PATCH v4 1/7]. The procedure for registering and initializing
(unregistering and deinitializing respectively) of the plugins has been
modified. This modification is inspired by the comments and suggestions
made by Steven Rostedt during our phone conversations.

Yordan Karadzhov (VMware) (7):
  kernel-shark-qt: Add plugin infrastructure to be used by the Qt-baset
    KS.
  kernel-shark-qt: Add Plugin event handlers to session.
  kernel-shark-qt: Add C++/C conversion for args of a plugin draw
    function.
  kernel-shark-qt: Make kshark_read_at() non-static.
  kernel-shark-qt: Add src/plugins dir. to hold the source code of the
    plugins
  kernel-shark-qt: Tell Doxygen to enter ../src/plugins/
  kernel-shark-qt: Add a plugin for sched events.

 kernel-shark-qt/doc/dox_config              |   2 +-
 kernel-shark-qt/src/CMakeLists.txt          |   3 +
 kernel-shark-qt/src/KsPlugins.hpp           |  51 ++++
 kernel-shark-qt/src/libkshark-plugin.c      | 292 +++++++++++++++++++
 kernel-shark-qt/src/libkshark-plugin.h      | 179 ++++++++++++
 kernel-shark-qt/src/libkshark.c             |  33 ++-
 kernel-shark-qt/src/libkshark.h             |  18 ++
 kernel-shark-qt/src/plugins/CMakeLists.txt  |  27 ++
 kernel-shark-qt/src/plugins/SchedEvents.cpp | 263 +++++++++++++++++
 kernel-shark-qt/src/plugins/sched_events.c  | 294 ++++++++++++++++++++
 kernel-shark-qt/src/plugins/sched_events.h  |  76 +++++
 11 files changed, 1235 insertions(+), 3 deletions(-)
 create mode 100644 kernel-shark-qt/src/KsPlugins.hpp
 create mode 100644 kernel-shark-qt/src/libkshark-plugin.c
 create mode 100644 kernel-shark-qt/src/libkshark-plugin.h
 create mode 100644 kernel-shark-qt/src/plugins/CMakeLists.txt
 create mode 100644 kernel-shark-qt/src/plugins/SchedEvents.cpp
 create mode 100644 kernel-shark-qt/src/plugins/sched_events.c
 create mode 100644 kernel-shark-qt/src/plugins/sched_events.h

-- 
2.17.1

[PATCH v4 1/7] kernel-shark-qt: Add plugin infrastructure to be used by the Qt-baset KS.

[Yordan Karadzhov (VMware)]

This patch adds infrastructure for loading/unloading of plugins. Each
plugin is coupled to a specific type of trace event and is allowed to
perform two types of actions.

The first action is executed once for each kshark_entry only at the time
when the data is loaded. This action can modify (even completely rewrite)
the content of the kshark_entrys generated from the trace events having
the type of the plugin.

The second action can add graphical elements on top of the existing graphs
generated by the Visualization model. This will become clear in the
following patches. This action is executed once for each graph and this is
happening every time when the Visualization model changes its state. It is
very powerful and can be used not only for drawing, but also for performing
arbitrary modifications on the data. The pointer to the model descriptor
object can access the entire array of kshark_entries, which, in turn, could
be used to modify those entries.

The plugin infrastructure of the Qt-baset KernelShark reuses the system
of macros for loading/unloading, implemented for the original GTK version.

This version of the patch contains number of improvements suggested by
Steven Rostedt. Thanks Steven!

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark-qt/src/CMakeLists.txt     |   1 +
 kernel-shark-qt/src/libkshark-plugin.c | 292 +++++++++++++++++++++++++
 kernel-shark-qt/src/libkshark-plugin.h | 179 +++++++++++++++
 kernel-shark-qt/src/libkshark.h        |   3 +
 4 files changed, 475 insertions(+)
 create mode 100644 kernel-shark-qt/src/libkshark-plugin.c
 create mode 100644 kernel-shark-qt/src/libkshark-plugin.h

diff --git a/kernel-shark-qt/src/CMakeLists.txt b/kernel-shark-qt/src/CMakeLists.txt
index ac2847a..cdc28c4 100644
--- a/kernel-shark-qt/src/CMakeLists.txt
+++ b/kernel-shark-qt/src/CMakeLists.txt
@@ -3,6 +3,7 @@ message("\n src ...")
 message(STATUS "libkshark")
 add_library(kshark SHARED libkshark.c
                           libkshark-model.c
+                          libkshark-plugin.c
                           libkshark-configio.c
                           libkshark-collection.c)
 
diff --git a/kernel-shark-qt/src/libkshark-plugin.c b/kernel-shark-qt/src/libkshark-plugin.c
new file mode 100644
index 0000000..8feb00a
--- /dev/null
+++ b/kernel-shark-qt/src/libkshark-plugin.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: LGPL-2.1
+
+/*
+ * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
+ */
+
+ /**
+  *  @file    libkshark-plugin.c
+  *  @brief   KernelShark plugins.
+  */
+
+// C
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <dlfcn.h>
+#include <errno.h>
+
+// KernelShark
+#include "libkshark-plugin.h"
+#include "libkshark.h"
+
+static struct kshark_event_handler *
+gui_event_handler_alloc(int event_id,
+			kshark_plugin_event_handler_func evt_func,
+			kshark_plugin_draw_handler_func dw_func)
+{
+	struct kshark_event_handler *handler = malloc(sizeof(*handler));
+
+	if (!handler) {
+		fprintf(stderr,
+			"failed to allocate memory for gui eventhandler");
+		return NULL;
+	}
+
+	handler->next = NULL;
+	handler->id = event_id;
+	handler->event_func = evt_func;
+	handler->draw_func = dw_func;
+
+	return handler;
+}
+
+/**
+ * @brief Search the list of event handlers for a handle associated with a
+ *	  given event type.
+ *
+ * @param handlers: Input location for the Event handler list.
+ * @param event_id: Event Id to search for.
+ */
+struct kshark_event_handler *
+find_event_handler(struct kshark_event_handler *handlers, int event_id)
+{
+	for (; handlers; handlers = handlers->next)
+		if (handlers->id == event_id)
+			return handlers;
+
+	return NULL;
+}
+
+/**
+ * @brief Add new event handler to an existing list of handlers.
+ *
+ * @param handlers: Input location for the Event handler list.
+ * @param event_id: Event Id.
+ * @param evt_func: Input location for an Event action provided by the plugin.
+ * @param dw_func: Input location for a Draw action provided by the plugin.
+ *
+ * @returns Zero on success, or a negative error code on failure.
+ */
+int kshark_register_event_handler(struct kshark_event_handler **handlers,
+				  int event_id,
+				  kshark_plugin_event_handler_func evt_func,
+				  kshark_plugin_draw_handler_func dw_func)
+{
+	struct kshark_event_handler *handler =
+		gui_event_handler_alloc(event_id, evt_func, dw_func);
+
+	if(!handler)
+		return -ENOMEM;
+
+	handler->next = *handlers;
+	*handlers = handler;
+	return 0;
+}
+
+/**
+ * @brief Search the list for a specific plugin handle. If such a plugin handle
+ *	  exists, unregister (remove and free) this handle from the list.
+ *
+ * @param handlers: Input location for the Event handler list.
+ * @param event_id: Event Id of the plugin handler to be unregistered.
+ * @param evt_func: Event action function of the handler to be unregistered.
+ * @param dw_func: Draw action function of the handler to be unregistered.
+ */
+void kshark_unregister_event_handler(struct kshark_event_handler **handlers,
+				     int event_id,
+				     kshark_plugin_event_handler_func evt_func,
+				     kshark_plugin_draw_handler_func dw_func)
+{
+	struct kshark_event_handler **last;
+
+	for (last = handlers; *last; last = &(*last)->next) {
+		if ((*last)->id == event_id &&
+		    (*last)->event_func == evt_func &&
+		    (*last)->draw_func == dw_func) {
+			struct kshark_event_handler *this_handler;
+			this_handler = *last;
+			*last = this_handler->next;
+			free(this_handler);
+
+			return;
+		}
+	}
+}
+
+/**
+ * @brief Free all Event handlers in a given list.
+ *
+ * @param handlers: Input location for the Event handler list.
+ */
+void kshark_free_event_handler_list(struct kshark_event_handler *handlers)
+{
+	struct kshark_event_handler *last;
+
+	while (handlers) {
+		last = handlers;
+		handlers = handlers->next;
+		free(last);
+	}
+}
+
+/**
+ * @brief Allocate memory for a new plugin. Add this plugin to the list of
+ *	  plugins used by the session.
+ *
+ * @param kshark_ctx: Input location for the session context pointer.
+ * @param file: The plugin object file to load.
+ *
+ * @returns Zero on success, or a negative error code on failure.
+ */
+int kshark_register_plugin(struct kshark_context *kshark_ctx,
+			   const char *file)
+{
+	struct kshark_plugin_list *plugin = kshark_ctx->plugins;
+	struct stat st;
+	int ret;
+
+	while (plugin) {
+		if (strcmp(plugin->file, file) == 0)
+			return -EEXIST;
+
+		plugin = plugin->next;
+	}
+
+	ret = stat(file, &st);
+	if (ret < 0) {
+		fprintf(stderr, "plugin %s not found\n", file);
+		return -ENODEV;
+	}
+
+	plugin = calloc(sizeof(struct kshark_plugin_list), 1);
+	if (!plugin) {
+		fprintf(stderr, "failed to allocate memory for plugin\n");
+		return -ENOMEM;
+	}
+
+	if (asprintf(&plugin->file, "%s", file) <= 0) {
+		fprintf(stderr,
+			"failed to allocate memory for plugin file name");
+		return -ENOMEM;
+	}
+
+	plugin->handle = dlopen(plugin->file, RTLD_NOW | RTLD_GLOBAL);
+	if (!plugin->handle)
+		goto fail;
+
+	plugin->init = dlsym(plugin->handle,
+			     KSHARK_PLUGIN_INITIALIZER_NAME);
+
+	plugin->close = dlsym(plugin->handle,
+			      KSHARK_PLUGIN_DEINITIALIZER_NAME);
+
+	if (!plugin->init || !plugin->close)
+		goto fail;
+
+	plugin->next = kshark_ctx->plugins;
+	kshark_ctx->plugins = plugin;
+
+	return 0;
+
+ fail:
+	fprintf(stderr, "cannot load plugin '%s'\n%s\n",
+		plugin->file, dlerror());
+
+	if (plugin->handle) {
+		dlclose(plugin->handle);
+		plugin->handle = NULL;
+	}
+
+	free(plugin);
+
+	return EFAULT;
+}
+
+/**
+ * @brief Unrgister a plugin.
+ *
+ * @param kshark_ctx: Input location for context pointer.
+ * @param file: The plugin object file to unregister.
+ */
+void kshark_unregister_plugin(struct kshark_context *kshark_ctx,
+			      const char *file)
+{
+	struct kshark_plugin_list **last;
+
+	for (last = &kshark_ctx->plugins; *last; last = &(*last)->next) {
+		if (strcmp((*last)->file, file) == 0) {
+			struct kshark_plugin_list *this_plugin;
+			this_plugin = *last;
+			*last = this_plugin->next;
+
+			dlclose(this_plugin->handle);
+			free(this_plugin);
+
+			return;
+		}
+	}
+}
+
+/**
+ * @brief Free all plugins in a given list.
+ *
+ * @param plugins: Input location for the plugins list.
+ */
+void kshark_free_plugin_list(struct kshark_plugin_list *plugins)
+{
+	struct kshark_plugin_list *last;
+
+	while (plugins) {
+		last = plugins;
+		plugins = plugins->next;
+
+		free(last->file);
+		dlclose(last->handle);
+
+		free(last);
+	}
+}
+
+/**
+ * @brief Use this function to initialize/update/deinitialize all registered
+ *	  plugins.
+ *
+ * @param kshark_ctx: Input location for context pointer.
+ * @param task_id: Action identifier specifying the action to be executed.
+ *
+ * @returns The number of successful added/removed plugin handlers on success,
+ *	    or a negative error code on failure.
+ */
+int kshark_handle_plugins(struct kshark_context *kshark_ctx,
+			  int task_id)
+{
+	struct kshark_plugin_list *plugin;
+	int handler_count = 0;
+
+	if (task_id != KSHARK_PLUGIN_INIT &&
+	    task_id != KSHARK_PLUGIN_UPDATE &&
+	    task_id != KSHARK_PLUGIN_CLOSE)
+		return -EINVAL;
+
+	for (plugin = kshark_ctx->plugins; plugin; plugin = plugin->next) {
+		switch (task_id) {
+		case KSHARK_PLUGIN_INIT:
+			handler_count += plugin->init(kshark_ctx);
+			break;
+
+		case KSHARK_PLUGIN_UPDATE:
+			plugin->close(kshark_ctx);
+			handler_count += plugin->init(kshark_ctx);
+			break;
+
+		case KSHARK_PLUGIN_CLOSE:
+			handler_count += plugin->close(kshark_ctx);
+			break;
+		}
+	}
+
+	return handler_count;
+}
diff --git a/kernel-shark-qt/src/libkshark-plugin.h b/kernel-shark-qt/src/libkshark-plugin.h
new file mode 100644
index 0000000..e770e08
--- /dev/null
+++ b/kernel-shark-qt/src/libkshark-plugin.h
@@ -0,0 +1,179 @@
+/* SPDX-License-Identifier: LGPL-2.1 */
+
+/*
+ * Copyright (C) 2016 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
+ */
+
+ /**
+  *  @file    libkshark-plugin.h
+  *  @brief   KernelShark plugins.
+  */
+
+#ifndef _KSHARK_PLUGIN_H
+#define _KSHARK_PLUGIN_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif // __cplusplus
+
+// trace-cmd
+#include "event-parse.h"
+
+/* Quiet warnings over documenting simple structures */
+//! @cond Doxygen_Suppress
+
+#define KSHARK_PLUGIN_INITIALIZER kshark_plugin_initializer
+
+#define KSHARK_PLUGIN_DEINITIALIZER kshark_plugin_deinitializer
+
+#define _MAKE_STR(x)	#x
+#define MAKE_STR(x)	_MAKE_STR(x)
+
+#define KSHARK_PLUGIN_INITIALIZER_NAME MAKE_STR(KSHARK_PLUGIN_INITIALIZER)
+
+#define KSHARK_PLUGIN_DEINITIALIZER_NAME MAKE_STR(KSHARK_PLUGIN_DEINITIALIZER)
+
+struct kshark_context;
+
+struct kshark_entry;
+
+//! @endcond
+
+/**
+ * A function type to be used when defining load/reload/unload plugin
+ * functions.
+ */
+typedef int (*kshark_plugin_load_func)(struct kshark_context *);
+
+struct kshark_trace_histo;
+
+/**
+ * Structure representing the C arguments of the drawing function of
+ * a plugin.
+ */
+struct kshark_cpp_argv {
+	/** Pointer to the model descriptor object. */
+	struct kshark_trace_histo	*histo;
+};
+
+/** A function type to be used when defining plugin functions for drawing. */
+typedef void
+(*kshark_plugin_draw_handler_func)(struct kshark_cpp_argv *argv,
+				   int val, int draw_action);
+
+/**
+ * A function type to be used when defining plugin functions for data
+ * manipulation.
+ */
+typedef void
+(*kshark_plugin_event_handler_func)(struct kshark_context *kshark_ctx,
+				    struct tep_record *rec,
+				    struct kshark_entry *e);
+
+/** Plugin action identifier. */
+enum kshark_plugin_actions {
+	/**
+	 * Load plugins action. This action identifier is used when handling
+	 * plugins.
+	 */
+	KSHARK_PLUGIN_INIT,
+
+	/**
+	 * Reload plugins action. This action identifier is used when handling
+	 * plugins.
+	 */
+	KSHARK_PLUGIN_UPDATE,
+
+	/**
+	 * Unload plugins action. This action identifier is used when handling
+	 * plugins.
+	 */
+	KSHARK_PLUGIN_CLOSE,
+
+	/**
+	 * Task draw action. This action identifier is used by the plugin draw
+	 * function.
+	 */
+	KSHARK_PLUGIN_TASK_DRAW,
+
+	/**
+	 * CPU draw action. This action identifier is used by the plugin draw
+	 * function.
+	 */
+	KSHARK_PLUGIN_CPU_DRAW,
+};
+
+/**
+ * Plugin Event handler structure, defining the properties of the required
+ * kshark_entry.
+ */
+struct kshark_event_handler {
+	/** Pointer to the next Plugin Event handler. */
+	struct kshark_event_handler		*next;
+
+	/** Unique Id ot the trace event type. */
+	int					id;
+
+	/**
+	 * Event action function. This action can be used to modify the content
+	 * of all kshark_entries having Event Ids equal to "id".
+	 */
+	kshark_plugin_event_handler_func	event_func;
+
+	/**
+	 * Draw action function. This action can be used to draw additional
+	 * graphical elements (shapes) for all kshark_entries having Event Ids
+	 * equal to "id".
+	 */
+	kshark_plugin_draw_handler_func		draw_func;
+};
+
+struct kshark_event_handler *
+find_event_handler(struct kshark_event_handler *handlers,
+						int event_id);
+
+int kshark_register_event_handler(struct kshark_event_handler **handlers,
+				  int event_id,
+				  kshark_plugin_event_handler_func evt_func,
+				  kshark_plugin_draw_handler_func dw_func);
+
+void kshark_unregister_event_handler(struct kshark_event_handler **handlers,
+				     int event_id,
+				     kshark_plugin_event_handler_func evt_func,
+				     kshark_plugin_draw_handler_func dw_func);
+
+void kshark_free_event_handler_list(struct kshark_event_handler *handlers);
+
+/** Linked list of plugins. */
+struct kshark_plugin_list {
+	/** Pointer to the next Plugin. */
+	struct kshark_plugin_list	*next;
+
+	/** The plugin object file to load. */
+	char				*file;
+
+	/** Plugin Event handler. */
+	void				*handle;
+
+	/** Callback function for initialization of the plugin. */
+	kshark_plugin_load_func		init;
+
+	/** Callback function for deinitialization of the plugin. */
+	kshark_plugin_load_func		close;
+};
+
+int kshark_register_plugin(struct kshark_context *kshark_ctx,
+			   const char *file);
+
+void kshark_unregister_plugin(struct kshark_context *kshark_ctx,
+			      const char *file);
+
+void kshark_free_plugin_list(struct kshark_plugin_list *plugins);
+
+int kshark_handle_plugins(struct kshark_context *kshark_ctx, int task_id);
+
+#ifdef __cplusplus
+}
+#endif // __cplusplus
+
+#endif // _KSHARK_PLUGIN_H
diff --git a/kernel-shark-qt/src/libkshark.h b/kernel-shark-qt/src/libkshark.h
index 2580449..fda133c 100644
--- a/kernel-shark-qt/src/libkshark.h
+++ b/kernel-shark-qt/src/libkshark.h
@@ -121,6 +121,9 @@ struct kshark_context {
 
 	/** List of Data collections. */
 	struct kshark_entry_collection *collections;
+
+	/** List of Plugins. */
+	struct kshark_plugin_list	*plugins;
 };
 
 bool kshark_instance(struct kshark_context **kshark_ctx);
-- 
2.17.1

[PATCH v4 2/7] kernel-shark-qt: Add Plugin event handlers to session.

[Yordan Karadzhov (VMware)]

Plugin event handlers are added to the Session context descriptor.
The handlers are used to execute plugin-specific action (callback
function) during the processing of the trace data.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark-qt/src/libkshark.c | 21 +++++++++++++++++++++
 kernel-shark-qt/src/libkshark.h | 12 ++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
index b4a76ae..6fb08b2 100644
--- a/kernel-shark-qt/src/libkshark.c
+++ b/kernel-shark-qt/src/libkshark.c
@@ -33,6 +33,9 @@ static bool kshark_default_context(struct kshark_context **context)
 	if (!kshark_ctx)
 		return false;
 
+	kshark_ctx->event_handlers = NULL;
+	kshark_ctx->plugins = NULL;
+
 	kshark_ctx->show_task_filter = tracecmd_filter_id_hash_alloc();
 	kshark_ctx->hide_task_filter = tracecmd_filter_id_hash_alloc();
 
@@ -217,6 +220,12 @@ void kshark_free(struct kshark_context *kshark_ctx)
 	tracecmd_filter_id_hash_free(kshark_ctx->show_event_filter);
 	tracecmd_filter_id_hash_free(kshark_ctx->hide_event_filter);
 
+	if(kshark_ctx->plugins) {
+		kshark_handle_plugins(kshark_ctx, KSHARK_PLUGIN_CLOSE);
+		kshark_free_plugin_list(kshark_ctx->plugins);
+		kshark_free_event_handler_list(kshark_ctx->event_handlers);
+	}
+
 	kshark_free_task_list(kshark_ctx);
 
 	if (seq.buffer)
@@ -564,6 +573,7 @@ static void free_rec_list(struct rec_list **rec_list, int n_cpus,
 static size_t get_records(struct kshark_context *kshark_ctx,
 			  struct rec_list ***rec_list, enum rec_type type)
 {
+	struct kshark_event_handler *evt_handler;
 	struct event_filter *adv_filter;
 	struct kshark_task_list *task;
 	struct tep_record *rec;
@@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx,
 
 				entry = &temp_rec->entry;
 				kshark_set_entry_values(kshark_ctx, rec, entry);
+
+				/* Execute all plugin-provided actions (if any). */
+				evt_handler = kshark_ctx->event_handlers;
+				while ((evt_handler = find_event_handler(evt_handler,
+									 entry->event_id))) {
+					evt_handler->event_func(kshark_ctx, rec, entry);
+
+					if ((evt_handler = evt_handler->next))
+						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
+				}
+
 				pid = entry->pid;
 				/* Apply event filtering. */
 				ret = FILTER_MATCH;
diff --git a/kernel-shark-qt/src/libkshark.h b/kernel-shark-qt/src/libkshark.h
index fda133c..203c812 100644
--- a/kernel-shark-qt/src/libkshark.h
+++ b/kernel-shark-qt/src/libkshark.h
@@ -29,6 +29,9 @@ extern "C" {
 #include "event-parse.h"
 #include "trace-filter-hash.h"
 
+// KernelShark
+#include "libkshark-plugin.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
@@ -124,6 +127,9 @@ struct kshark_context {
 
 	/** List of Plugins. */
 	struct kshark_plugin_list	*plugins;
+
+	/** List of Plugin Event handlers. */
+	struct kshark_event_handler	*event_handlers;
 };
 
 bool kshark_instance(struct kshark_context **kshark_ctx);
@@ -160,6 +166,12 @@ enum kshark_filter_masks {
 
 	/** Special mask used whene filtering events. */
 	KS_EVENT_VIEW_FILTER_MASK	= 1 << 2,
+
+	/**
+	 * Use this mask to check if the content of the entry has been accessed
+	 * by a plugin-defined function.
+	 */
+	KS_PLUGIN_UNTOUCHED_MASK	= 1 << 7
 };
 
 /** Filter type identifier. */
-- 
2.17.1

[PATCH v4 3/7] kernel-shark-qt: Add C++/C conversion for args of a plugin draw function.

[Yordan Karadzhov (VMware)]

The draw function of a plugin has a type declared in C. However, the
function needs to access some high level objects, defined in the C++
libraries. This patch adds instruments for converting the arguments
of the plugin's draw function from C++ to C and from C to C++.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark-qt/src/KsPlugins.hpp | 51 +++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 kernel-shark-qt/src/KsPlugins.hpp

diff --git a/kernel-shark-qt/src/KsPlugins.hpp b/kernel-shark-qt/src/KsPlugins.hpp
new file mode 100644
index 0000000..3955cdf
--- /dev/null
+++ b/kernel-shark-qt/src/KsPlugins.hpp
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: LGPL-2.1 */
+
+/*
+ * Copyright (C) 2018 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
+ */
+
+/**
+  *  @file    KsPlugins.hpp
+  *  @brief   KernelShark C++ plugin declarations.
+  */
+
+#ifndef _KS_PLUGINS_H
+#define _KS_PLUGINS_H
+
+// KernelShark
+#include "libkshark-model.h"
+#include "KsPlotTools.hpp"
+
+/**
+ * Structure representing the vector of C++ arguments of the drawing function
+ * of a plugin.
+ */
+struct KsCppArgV {
+	/** Pointer to the model descriptor object. */
+	kshark_trace_histo	*_histo;
+
+	/** Pointer to the graph object. */
+	KsPlot::Graph		*_graph;
+
+	/**
+	 * Pointer to the list of shapes. All shapes created by the plugin
+	 * will be added to this list.
+	 */
+	KsPlot::PlotObjList	*_shapes;
+
+	/**
+	 * Convert the "this" pointer of the C++ argument vector into a
+	 * C pointer.
+	 */
+	kshark_cpp_argv *toC()
+	{
+		return reinterpret_cast<kshark_cpp_argv *>(this);
+	}
+};
+
+/**
+ * Macro used to convert a C pointer into a pointer to KsCppArgV (C++ struct).
+ */
+#define KS_ARGV_TO_CPP(a) (reinterpret_cast<KsCppArgV *>(a))
+
+#endif
-- 
2.17.1

[PATCH v4 4/7] kernel-shark-qt: Make kshark_read_at() non-static.

[Yordan Karadzhov (VMware)]

kshark_read_at() function provides a thread-safe read of a record from
a specific offset inside the trace.dat file. So far this function was
used only in libkshark.c and was defined static. This patch makes the
function non-static in order to make possible to use it from a plugin.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark-qt/src/libkshark.c | 12 ++++++++++--
 kernel-shark-qt/src/libkshark.h |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
index 6fb08b2..fcf61bf 100644
--- a/kernel-shark-qt/src/libkshark.c
+++ b/kernel-shark-qt/src/libkshark.c
@@ -830,8 +830,16 @@ ssize_t kshark_load_data_records(struct kshark_context *kshark_ctx,
 	return -ENOMEM;
 }
 
-static struct tep_record *kshark_read_at(struct kshark_context *kshark_ctx,
-					 uint64_t offset)
+/**
+ * @brief A thread-safe read of a record from a specific offset.
+ *
+ * @param kshark_ctx: Input location for the session context pointer.
+ * @param offset: the offset into the file to find the record.
+ *
+ * @returns The returned pevent_record must be freed.
+ */
+struct tep_record *kshark_read_at(struct kshark_context *kshark_ctx,
+				  uint64_t offset)
 {
 	/*
 	 * It turns that tracecmd_read_at() is not thread-safe.
diff --git a/kernel-shark-qt/src/libkshark.h b/kernel-shark-qt/src/libkshark.h
index 203c812..ba5ea6c 100644
--- a/kernel-shark-qt/src/libkshark.h
+++ b/kernel-shark-qt/src/libkshark.h
@@ -150,6 +150,9 @@ void kshark_free(struct kshark_context *kshark_ctx);
 
 char* kshark_dump_entry(const struct kshark_entry *entry);
 
+struct tep_record *kshark_read_at(struct kshark_context *kshark_ctx,
+				  uint64_t offset);
+
 /** Bit masks used to control the visibility of the entry after filtering. */
 enum kshark_filter_masks {
 	/**
-- 
2.17.1

[PATCH v4 5/7] kernel-shark-qt: Add src/plugins dir. to hold the source code of the plugins

[Yordan Karadzhov (VMware)]

Tell Cmake to enter src/plugins. Add a Cmake function for building plugin
libraries.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark-qt/src/CMakeLists.txt         |  2 ++
 kernel-shark-qt/src/plugins/CMakeLists.txt | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 kernel-shark-qt/src/plugins/CMakeLists.txt

diff --git a/kernel-shark-qt/src/CMakeLists.txt b/kernel-shark-qt/src/CMakeLists.txt
index cdc28c4..3365413 100644
--- a/kernel-shark-qt/src/CMakeLists.txt
+++ b/kernel-shark-qt/src/CMakeLists.txt
@@ -28,5 +28,7 @@ if (OPENGL_FOUND AND GLUT_FOUND)
 
 endif (OPENGL_FOUND AND GLUT_FOUND)
 
+add_subdirectory(plugins)
+
 configure_file( ${KS_DIR}/build/deff.h.cmake
                 ${KS_DIR}/src/KsDeff.h)
diff --git a/kernel-shark-qt/src/plugins/CMakeLists.txt b/kernel-shark-qt/src/plugins/CMakeLists.txt
new file mode 100644
index 0000000..565f1cb
--- /dev/null
+++ b/kernel-shark-qt/src/plugins/CMakeLists.txt
@@ -0,0 +1,22 @@
+message("\n src/plugins ...")
+
+function(BUILD_PLUGIN)
+    set(options )
+    set(oneValueArgs NAME)
+    set(multiValueArgs SOURCE)
+    cmake_parse_arguments(ADD_PLUGIN "${options}"
+                                     ${oneValueArgs}
+                                     ${multiValueArgs}
+                                     ${ARGN})
+
+    message(STATUS ${ADD_PLUGIN_NAME})
+
+    add_library(${ADD_PLUGIN_NAME} SHARED ${ADD_PLUGIN_SOURCE})
+    set_target_properties(${ADD_PLUGIN_NAME} PROPERTIES PREFIX "plugin-")
+    target_link_libraries(${ADD_PLUGIN_NAME} kshark)
+
+endfunction()
+
+set(PLUGIN_LIST "")
+
+set(PLUGINS ${PLUGIN_LIST} PARENT_SCOPE)
-- 
2.17.1

[PATCH v4 6/7] kernel-shark-qt: Tell Doxygen to enter ../src/plugins/

[Yordan Karadzhov (VMware)]

This Patch adds ../src/plugins/ to the list of input locations used by
Doxygen to search for documented source files.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark-qt/doc/dox_config | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-shark-qt/doc/dox_config b/kernel-shark-qt/doc/dox_config
index b982baa..acc9083 100644
--- a/kernel-shark-qt/doc/dox_config
+++ b/kernel-shark-qt/doc/dox_config
@@ -4,7 +4,7 @@
 DOXYFILE_ENCODING      = UTF-8
 PROJECT_NAME           = "kernel-shark-qt"
 PROJECT_BRIEF = "Kernel Shark is a front-end reader of the Linux kernel tracing data."
-INPUT                  = "../src/"
+INPUT                  = ../src/ ../src/plugins/
 SOURCE_BROWSER         = YES
 QT_AUTOBRIEF           = YES
 TAB_SIZE               = 8
-- 
2.17.1

[PATCH v4 7/7] kernel-shark-qt: Add a plugin for sched events.

[Yordan Karadzhov (VMware)]

The plugin is responsible for the following actions, specific for
"sched" events:

1. Changes the value of the "pid" field of the "sched_switch" entries.
   This alows the sched_switch entry to be plotted as part of the "next"
   task.

2. On "sched_switch" event, the plugin registers the "next" task (if not
   registered already).

3. Plots in green the wake up latency of the task and in red the time
   the task was preempted by another task.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark-qt/src/plugins/CMakeLists.txt  |   5 +
 kernel-shark-qt/src/plugins/SchedEvents.cpp | 263 +++++++++++++++++
 kernel-shark-qt/src/plugins/sched_events.c  | 294 ++++++++++++++++++++
 kernel-shark-qt/src/plugins/sched_events.h  |  76 +++++
 4 files changed, 638 insertions(+)
 create mode 100644 kernel-shark-qt/src/plugins/SchedEvents.cpp
 create mode 100644 kernel-shark-qt/src/plugins/sched_events.c
 create mode 100644 kernel-shark-qt/src/plugins/sched_events.h

diff --git a/kernel-shark-qt/src/plugins/CMakeLists.txt b/kernel-shark-qt/src/plugins/CMakeLists.txt
index 565f1cb..88fd93c 100644
--- a/kernel-shark-qt/src/plugins/CMakeLists.txt
+++ b/kernel-shark-qt/src/plugins/CMakeLists.txt
@@ -19,4 +19,9 @@ endfunction()
 
 set(PLUGIN_LIST "")
 
+BUILD_PLUGIN(NAME sched_events
+             SOURCE sched_events.c SchedEvents.cpp)
+list(APPEND PLUGIN_LIST "sched_events default") # This plugin will be loaded by default
+# list(APPEND PLUGIN_LIST "sched_events") # This plugin isn't loaded by default
+
 set(PLUGINS ${PLUGIN_LIST} PARENT_SCOPE)
diff --git a/kernel-shark-qt/src/plugins/SchedEvents.cpp b/kernel-shark-qt/src/plugins/SchedEvents.cpp
new file mode 100644
index 0000000..04d9a50
--- /dev/null
+++ b/kernel-shark-qt/src/plugins/SchedEvents.cpp
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: LGPL-2.1
+
+/*
+ * Copyright (C) 2018 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
+ */
+
+/**
+ *  @file    SchedEvents.cpp
+ *  @brief   Defines a callback function for Sched events used to plot in green
+ *	     the wake up latency of the task and in red the time the task was
+ *	     preempted by another task.
+ */
+
+// C++
+#include<iostream>
+
+// C++ 11
+#include<functional>
+
+// KernelShark
+#include "libkshark.h"
+#include "plugins/sched_events.h"
+#include "KsPlotTools.hpp"
+#include "KsPlugins.hpp"
+
+//! @cond Doxygen_Suppress
+
+#define PLUGIN_MIN_BOX_SIZE 4
+
+#define PLUGIN_MAX_ENTRIES_PER_BIN 500
+
+//! @endcond
+
+extern struct plugin_sched_context *plugin_sched_context_handler;
+
+static int plugin_get_wakeup_pid(kshark_context *kshark_ctx,
+				 plugin_sched_context *plugin_ctx,
+				 const struct kshark_entry *e)
+{
+	struct tep_record *record;
+	unsigned long long val;
+
+	record = kshark_read_at(kshark_ctx, e->offset);
+	tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
+			      record->data, &val);
+	free(record);
+
+	return val;
+}
+
+/** Sched Event identifier. */
+enum class SchedEvent {
+	/** Sched Switch Event. */
+	Switch,
+
+	/** Sched Wakeup Event. */
+	Wakeup,
+};
+
+static void pluginDraw(plugin_sched_context *plugin_ctx,
+		       kshark_context *kshark_ctx,
+		       kshark_trace_histo *histo,
+		       kshark_entry_collection *col,
+		       SchedEvent e,
+		       int pid,
+		       KsPlot::Graph *graph,
+		       KsPlot::PlotObjList *shapes)
+{
+	std::function<void(int)> ifSchedBack;
+	KsPlot::Rectangle *rec = nullptr;
+	int height = graph->getHeight() * .3;
+
+	auto openBox = [&] (const KsPlot::Point &p)
+	{
+		/*
+		 * First check if we already have an open box. If we don't
+		 * have, open a new one.
+		 */
+		if (!rec)
+			rec = new KsPlot::Rectangle;
+
+		rec->setFill(false);
+		rec->setPoint(0, p.x() - 1, p.y() - height);
+		rec->setPoint(1, p.x() - 1, p.y() - 1);
+	};
+
+	auto closeBox = [&] (const KsPlot::Point &p)
+	{
+		if (rec == nullptr)
+			return;
+
+		int boxSize = rec->getPoint(0)->x;
+		if (boxSize < PLUGIN_MIN_BOX_SIZE) {
+			/* This box is too small. Don't try to plot it. */
+			delete rec;
+			rec = nullptr;
+			return;
+		}
+
+		rec->setPoint(3, p.x() - 1, p.y() - height);
+		rec->setPoint(2, p.x() - 1, p.y() - 1);
+
+		shapes->push_front(rec);
+		rec = nullptr;
+	};
+
+	auto lamIfSchSwitchFront = [&] (int bin)
+	{
+		/*
+		 * Starting from the first element in this bin, go forward
+		 * in time until you find a trace entry that satisfies the
+		 * condition defined by kshark_match_pid.
+		 */
+		const kshark_entry *entryF =
+			ksmodel_get_entry_front(histo, bin, false,
+						kshark_match_pid, pid,
+						col, nullptr);
+
+		if (entryF &&
+		    entryF->pid == pid &&
+		    plugin_ctx->sched_switch_event &&
+		    entryF->event_id == plugin_ctx->sched_switch_event->id) {
+			/*
+			 * entryF is sched_switch_event. Close the box and add
+			 * it to the list of shapes to be ploted.
+			 */
+			closeBox(graph->getBin(bin)._base);
+		}
+	};
+
+	auto lamIfSchWakeupBack = [&] (int bin)
+	{
+		/*
+		 * Starting from the last element in this bin, go backward
+		 * in time until you find a trace entry that satisfies the
+		 * condition defined by plugin_wakeup_match_pid.
+		 */
+		const kshark_entry *entryB =
+			ksmodel_get_entry_back(histo, bin, false,
+					       plugin_wakeup_match_pid, pid,
+					       col, nullptr);
+		int wakeup_pid;
+
+		if (entryB &&
+		    plugin_ctx->sched_wakeup_event &&
+		    entryB->event_id == plugin_ctx->sched_wakeup_event->id) {
+			wakeup_pid =
+				plugin_get_wakeup_pid(kshark_ctx, plugin_ctx, entryB);
+			if (wakeup_pid == pid) {
+				/*
+				 * entryB is a sched_wakeup_event. Open a
+				 * green box here.
+				 */
+				openBox(graph->getBin(bin)._base);
+
+				 /* Green */
+				rec->_color = KsPlot::Color(0, 255, 0);
+			}
+		}
+	};
+
+	auto lamIfSchSwitchBack = [&] (int bin)
+	{
+		/*
+		 * Starting from the last element in this bin, go backward
+		 * in time until you find a trace entry that satisfies the
+		 * condition defined by plugin_switch_match_pid.
+		 */
+		const kshark_entry *entryB =
+			ksmodel_get_entry_back(histo, bin, false,
+					       plugin_switch_match_pid, pid,
+					       col, nullptr);
+
+		if (entryB &&
+		    entryB->pid != pid &&
+		    plugin_ctx->sched_switch_event &&
+		    entryB->event_id == plugin_ctx->sched_switch_event->id) {
+			/*
+			 * entryB is a sched_switch_event. Open a
+			 * red box here.
+			 */
+			openBox(graph->getBin(bin)._base);
+
+			/* Red */
+			rec->_color = KsPlot::Color(255, 0, 0);
+		}
+	};
+
+	if (e == SchedEvent::Switch)
+		ifSchedBack = lamIfSchSwitchBack;
+	else
+		ifSchedBack = lamIfSchWakeupBack;
+
+	for (int bin = 0; bin < graph->size(); ++bin) {
+		/**
+		 * Plotting the latencies makes sense only in the case of a
+		 * deep zoom. Here we set a naive threshold based on the number
+		 * of entries inside the current bin. This cut seems to work
+		 * well in all cases I tested so far, but it may result in
+		 * unexpected behavior with some unusual trace data-sets.
+		 * TODO: find a better criteria for deciding when to start
+		 * plotting latencies.
+		 */
+		if (ksmodel_bin_count(histo, bin) > PLUGIN_MAX_ENTRIES_PER_BIN)
+			continue;
+
+		lamIfSchSwitchFront(bin);
+
+		ifSchedBack(bin);
+	}
+
+	if (rec)
+		delete rec;
+
+	return;
+}
+
+/**
+ * @brief Plugin's draw function.
+ *
+ * @param argv_c: A C pointer to be converted to KsCppArgV (C++ struct).
+ * @param pid: Process Id.
+ * @param draw_action: Draw action identifier.
+ *
+ * @returns True if the Pid of the entry matches the value of "pid".
+ *	    Otherwise false.
+ */
+void plugin_draw(kshark_cpp_argv *argv_c, int pid, int draw_action)
+{
+	plugin_sched_context *plugin_ctx;
+	kshark_context *kshark_ctx(NULL);
+	kshark_entry_collection *col;
+
+	if (draw_action != KSHARK_PLUGIN_TASK_DRAW || pid == 0)
+		return;
+
+	plugin_ctx = plugin_sched_context_handler;
+	if (!plugin_ctx || !kshark_instance(&kshark_ctx))
+		return;
+
+	KsCppArgV *argvCpp = KS_ARGV_TO_CPP(argv_c);
+
+	/*
+	 * Try to find a collections for this task. It is OK if
+	 * coll = NULL.
+	 */
+	col = kshark_find_data_collection(kshark_ctx->collections,
+					  kshark_match_pid, pid);
+
+	try {
+		pluginDraw(plugin_ctx, kshark_ctx,
+			   argvCpp->_histo, col,
+			   SchedEvent::Switch, pid,
+			   argvCpp->_graph, argvCpp->_shapes);
+
+		pluginDraw(plugin_ctx, kshark_ctx,
+			   argvCpp->_histo, col,
+			   SchedEvent::Wakeup, pid,
+			   argvCpp->_graph, argvCpp->_shapes);
+	} catch (const std::exception &exc) {
+		std::cerr << "Exception in SchedEvents\n" << exc.what();
+	}
+}
diff --git a/kernel-shark-qt/src/plugins/sched_events.c b/kernel-shark-qt/src/plugins/sched_events.c
new file mode 100644
index 0000000..d761d7c
--- /dev/null
+++ b/kernel-shark-qt/src/plugins/sched_events.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: LGPL-2.1
+
+/*
+ * Copyright (C) 2018 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
+ */
+
+/**
+ *  @file    sched_events.c
+ *  @brief   Defines a callback function for Sched events used to registers the
+ *	     "next" task (if not registered already) and to changes the value
+ *	     of the "pid" field of the "sched_switch" entries such that, it
+ *	     will be ploted as part of the "next" task.
+ */
+
+// C
+#include <stdlib.h>
+#include <stdio.h>
+
+// KernelShark
+#include "plugins/sched_events.h"
+
+/** Plugin context instance. */
+struct plugin_sched_context *plugin_sched_context_handler = NULL;
+
+static bool plugin_sched_update_context(struct kshark_context *kshark_ctx)
+{
+	struct plugin_sched_context *plugin_ctx;
+	struct event_format *event;
+
+	if (!plugin_sched_context_handler) {
+		plugin_sched_context_handler =
+		 malloc(sizeof(*plugin_sched_context_handler));
+	}
+
+	plugin_ctx = plugin_sched_context_handler;
+	plugin_ctx->handle = kshark_ctx->handle;
+	plugin_ctx->pevent = kshark_ctx->pevent;
+
+	event = tep_find_event_by_name(plugin_ctx->pevent,
+				       "sched", "sched_switch");
+	if (!event)
+		return false;
+
+	plugin_ctx->sched_switch_event = event;
+	plugin_ctx->sched_switch_next_field =
+		tep_find_any_field(event, "next_pid");
+
+	plugin_ctx->sched_switch_comm_field =
+		tep_find_field(event, "next_comm");
+
+	event = tep_find_event_by_name(plugin_ctx->pevent,
+				      "sched", "sched_wakeup");
+	if (!event)
+		return false;
+
+	plugin_ctx->sched_wakeup_event = event;
+	plugin_ctx->sched_wakeup_pid_field =
+		tep_find_any_field(event, "pid");
+
+	plugin_ctx->sched_wakeup_success_field =
+		tep_find_field(event, "success");
+
+	event = tep_find_event_by_name(plugin_ctx->pevent,
+				       "sched", "sched_wakeup_new");
+	if (!event)
+		return false;
+
+	plugin_ctx->sched_wakeup_new_event = event;
+	plugin_ctx->sched_wakeup_new_pid_field =
+		tep_find_any_field(event, "pid");
+
+	plugin_ctx->sched_wakeup_new_success_field =
+		tep_find_field(event, "success");
+
+	return true;
+}
+
+/**
+ * @brief Get the Process Id of the next scheduled task.
+ *
+ * @param record: Input location for a sched_switch record.
+ */
+int plugin_get_next_pid(struct tep_record *record)
+{
+	struct plugin_sched_context *plugin_ctx =
+		plugin_sched_context_handler;
+	unsigned long long val;
+
+	tep_read_number_field(plugin_ctx->sched_switch_next_field,
+			      record->data, &val);
+	return val;
+}
+
+/**
+ * @brief Get the Process Id of the task being woke up.
+ *
+ * @param record: Input location for a sched_wakeup record.
+ */
+int plugin_get_wakeup_pid(struct tep_record *record)
+{
+	struct plugin_sched_context *plugin_ctx =
+		plugin_sched_context_handler;
+	unsigned long long val;
+
+	tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
+			      record->data, &val);
+	return val;
+}
+
+static void plugin_register_command(struct kshark_context *kshark_ctx,
+				    struct tep_record *record,
+				    int pid)
+{
+	struct plugin_sched_context *plugin_ctx =
+		plugin_sched_context_handler;
+	const char *comm;
+
+	if (!plugin_ctx->sched_switch_comm_field)
+		return;
+
+	comm = record->data + plugin_ctx->sched_switch_comm_field->offset;
+
+	if (!tep_pid_is_registered(kshark_ctx->pevent, pid))
+			tep_register_comm(kshark_ctx->pevent, comm, pid);
+}
+
+static int plugin_get_wakeup_new_pid(struct tep_record *record)
+{
+	struct plugin_sched_context *plugin_ctx =
+		plugin_sched_context_handler;
+	unsigned long long val;
+
+	tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
+				 record->data, &val);
+
+	return val;
+}
+
+/**
+ * @brief Process Id matching function adapted for sched_wakeup and
+ *	  sched_wakeup_new events.
+ *
+ * @param kshark_ctx: Input location for the session context pointer.
+ * @param e: kshark_entry to be checked.
+ * @param pid: Matching condition value.
+ *
+ * @returns True if the Pid of the entry matches the value of "pid".
+ *	    Otherwise false.
+ */
+bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
+			     struct kshark_entry *e,
+			     int pid)
+{
+	struct plugin_sched_context *plugin_ctx;
+	struct tep_record *record = NULL;
+	unsigned long long val;
+	int wakeup_pid = -1;
+
+	if (e->pid == pid)
+		return true;
+
+	plugin_ctx = plugin_sched_context_handler;
+	if (!plugin_ctx)
+		return false;
+
+	if (plugin_ctx->sched_wakeup_event &&
+	    e->event_id == plugin_ctx->sched_wakeup_event->id) {
+		record = kshark_read_at(kshark_ctx, e->offset);
+
+		/* We only want those that actually woke up the task. */
+		tep_read_number_field(plugin_ctx->sched_wakeup_success_field,
+				      record->data, &val);
+
+		if (val)
+			wakeup_pid = plugin_get_wakeup_pid(record);
+	}
+
+	if (plugin_ctx->sched_wakeup_new_event &&
+	    e->event_id == plugin_ctx->sched_wakeup_new_event->id) {
+		record = kshark_read_at(kshark_ctx, e->offset);
+
+		/* We only want those that actually woke up the task. */
+		tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field,
+				      record->data, &val);
+
+		if (val)
+			wakeup_pid = plugin_get_wakeup_new_pid(record);
+	}
+
+	free(record);
+
+	if (wakeup_pid >= 0 && wakeup_pid == pid)
+		return true;
+
+	return false;
+}
+
+/**
+ * @brief Process Id matching function adapted for sched_switch events.
+ *
+ * @param kshark_ctx: Input location for the session context pointer.
+ * @param e: kshark_entry to be checked.
+ * @param pid: Matching condition value.
+ *
+ * @returns True if the Pid of the entry matches the value of "pid".
+ *	    Otherwise false.
+ */
+bool plugin_switch_match_pid(struct kshark_context *kshark_ctx,
+			     struct kshark_entry *e,
+			     int pid)
+{
+	struct plugin_sched_context *plugin_ctx;
+	struct tep_record *record = NULL;
+	int switch_pid = -1;
+
+	if (e->pid == pid)
+		return true;
+
+	plugin_ctx = plugin_sched_context_handler;
+
+	if (plugin_ctx->sched_switch_event &&
+	    e->event_id == plugin_ctx->sched_switch_event->id) {
+		record = kshark_read_at(kshark_ctx, e->offset);
+
+		switch_pid = tep_data_pid(plugin_ctx->pevent, record);
+	}
+
+	free(record);
+
+	if (switch_pid >= 0 && switch_pid == pid)
+		return true;
+
+	return false;
+}
+
+static void plugin_sched_action(struct kshark_context *kshark_ctx,
+				struct tep_record *rec,
+				struct kshark_entry *entry)
+{
+	entry->pid = plugin_get_next_pid(rec);
+	plugin_register_command(kshark_ctx, rec, entry->pid);
+}
+
+static int plugin_sched_init(struct kshark_context *kshark_ctx)
+{
+	struct plugin_sched_context *plugin_ctx;
+
+	if (!plugin_sched_update_context(kshark_ctx)) {
+		free(plugin_sched_context_handler);
+		plugin_sched_context_handler = NULL;
+		return 0;
+	}
+
+	plugin_ctx = plugin_sched_context_handler;
+
+	kshark_register_event_handler(&kshark_ctx->event_handlers,
+				      plugin_ctx->sched_switch_event->id,
+				      plugin_sched_action,
+				      plugin_draw);
+
+	return 1;
+}
+
+static int plugin_sched_close(struct kshark_context *kshark_ctx)
+{
+	struct plugin_sched_context *plugin_ctx;
+
+	if (!plugin_sched_context_handler)
+		return 0;
+
+	plugin_ctx = plugin_sched_context_handler;
+
+	kshark_unregister_event_handler(&kshark_ctx->event_handlers,
+					plugin_ctx->sched_switch_event->id,
+					plugin_sched_action,
+					plugin_draw);
+
+	free(plugin_ctx);
+	plugin_sched_context_handler = NULL;
+
+	return 1;
+}
+
+/** Load this plugin. */
+int KSHARK_PLUGIN_INITIALIZER(struct kshark_context *kshark_ctx)
+{
+	return plugin_sched_init(kshark_ctx);
+}
+
+/** Unload this plugin. */
+int KSHARK_PLUGIN_DEINITIALIZER(struct kshark_context *kshark_ctx)
+{
+	return plugin_sched_close(kshark_ctx);
+}
diff --git a/kernel-shark-qt/src/plugins/sched_events.h b/kernel-shark-qt/src/plugins/sched_events.h
new file mode 100644
index 0000000..673f5cf
--- /dev/null
+++ b/kernel-shark-qt/src/plugins/sched_events.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: LGPL-2.1 */
+
+/*
+ * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
+ */
+
+/**
+ *  @file    sched_events.h
+ *  @brief   Plugin for Sched events.
+ */
+
+#ifndef _KS_PLUGIN_SHED_H
+#define _KS_PLUGIN_SHED_H
+
+// KernelShark
+#include "libkshark.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/** Structure representing a plugin-specific context. */
+struct plugin_sched_context {
+	/** Input handle for the trace data file. */
+	struct tracecmd_input	*handle;
+
+	/** Page event used to parse the page. */
+	struct tep_handle	*pevent;
+
+	/** Pointer to the sched_switch_event object. */
+	struct event_format	*sched_switch_event;
+
+	/** Pointer to the sched_switch_next_field format descriptor. */
+	struct format_field	*sched_switch_next_field;
+
+	/** Pointer to the sched_switch_comm_field format descriptor. */
+	struct format_field	*sched_switch_comm_field;
+
+	/** Pointer to the sched_wakeup_event object. */
+	struct event_format	*sched_wakeup_event;
+
+	/** Pointer to the sched_wakeup_pid_field format descriptor. */
+	struct format_field	*sched_wakeup_pid_field;
+
+	/** Pointer to the sched_wakeup_success_field format descriptor. */
+	struct format_field	*sched_wakeup_success_field;
+
+	/** Pointer to the sched_wakeup_new_event object. */
+	struct event_format	*sched_wakeup_new_event;
+
+	/** Pointer to the sched_wakeup_new_pid_field format descriptor. */
+	struct format_field	*sched_wakeup_new_pid_field;
+
+	/**
+	 * Pointer to the sched_wakeup_new_success_field format descriptor.
+	 */
+	struct format_field	*sched_wakeup_new_success_field;
+};
+
+int plugin_get_next_pid(struct tep_record *record);
+
+int plugin_get_wakeup_pid(struct tep_record *record);
+
+bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
+			     struct kshark_entry *e, int pid);
+
+bool plugin_switch_match_pid(struct kshark_context *kshark_ctx,
+			     struct kshark_entry *e, int pid);
+
+void plugin_draw(struct kshark_cpp_argv *argv, int pid, int draw_action);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
-- 
2.17.1

Re: [PATCH v4 1/7] kernel-shark-qt: Add plugin infrastructure to be used by the Qt-baset KS.

[Steven Rostedt]

On Wed, 19 Sep 2018 17:36:51 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> diff --git a/kernel-shark-qt/src/libkshark-plugin.c b/kernel-shark-qt/src/libkshark-plugin.c
> new file mode 100644
> index 0000000..8feb00a
> --- /dev/null
> +++ b/kernel-shark-qt/src/libkshark-plugin.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +
> +/*
> + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
> + */
> +
> + /**
> +  *  @file    libkshark-plugin.c
> +  *  @brief   KernelShark plugins.
> +  */
> +
> +// C
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <dlfcn.h>
> +#include <errno.h>
> +
> +// KernelShark
> +#include "libkshark-plugin.h"
> +#include "libkshark.h"
> +
> +static struct kshark_event_handler *
> +gui_event_handler_alloc(int event_id,
> +			kshark_plugin_event_handler_func evt_func,
> +			kshark_plugin_draw_handler_func dw_func)
> +{
> +	struct kshark_event_handler *handler = malloc(sizeof(*handler));
> +
> +	if (!handler) {
> +		fprintf(stderr,
> +			"failed to allocate memory for gui eventhandler");
> +		return NULL;
> +	}
> +
> +	handler->next = NULL;
> +	handler->id = event_id;
> +	handler->event_func = evt_func;
> +	handler->draw_func = dw_func;
> +
> +	return handler;
> +}
> +
> +/**
> + * @brief Search the list of event handlers for a handle associated with a
> + *	  given event type.
> + *
> + * @param handlers: Input location for the Event handler list.
> + * @param event_id: Event Id to search for.
> + */
> +struct kshark_event_handler *
> +find_event_handler(struct kshark_event_handler *handlers, int event_id)

This is not a static function. Shouldn't it have a "kshark_" prefix?

> +{
> +	for (; handlers; handlers = handlers->next)
> +		if (handlers->id == event_id)
> +			return handlers;
> +
> +	return NULL;
> +}
> +
> +/**
> + * @brief Add new event handler to an existing list of handlers.
> + *
> + * @param handlers: Input location for the Event handler list.
> + * @param event_id: Event Id.
> + * @param evt_func: Input location for an Event action provided by the plugin.
> + * @param dw_func: Input location for a Draw action provided by the plugin.
> + *
> + * @returns Zero on success, or a negative error code on failure.
> + */
> +int kshark_register_event_handler(struct kshark_event_handler **handlers,
> +				  int event_id,
> +				  kshark_plugin_event_handler_func evt_func,
> +				  kshark_plugin_draw_handler_func dw_func)
> +{
> +	struct kshark_event_handler *handler =
> +		gui_event_handler_alloc(event_id, evt_func, dw_func);
> +
> +	if(!handler)
> +		return -ENOMEM;
> +
> +	handler->next = *handlers;
> +	*handlers = handler;
> +	return 0;
> +}
> +
> +/**
> + * @brief Search the list for a specific plugin handle. If such a plugin handle
> + *	  exists, unregister (remove and free) this handle from the list.
> + *
> + * @param handlers: Input location for the Event handler list.
> + * @param event_id: Event Id of the plugin handler to be unregistered.
> + * @param evt_func: Event action function of the handler to be unregistered.
> + * @param dw_func: Draw action function of the handler to be unregistered.
> + */
> +void kshark_unregister_event_handler(struct kshark_event_handler **handlers,
> +				     int event_id,
> +				     kshark_plugin_event_handler_func evt_func,
> +				     kshark_plugin_draw_handler_func dw_func)
> +{
> +	struct kshark_event_handler **last;
> +
> +	for (last = handlers; *last; last = &(*last)->next) {
> +		if ((*last)->id == event_id &&
> +		    (*last)->event_func == evt_func &&
> +		    (*last)->draw_func == dw_func) {
> +			struct kshark_event_handler *this_handler;
> +			this_handler = *last;
> +			*last = this_handler->next;
> +			free(this_handler);
> +
> +			return;
> +		}
> +	}
> +}
> +
> +/**
> + * @brief Free all Event handlers in a given list.
> + *
> + * @param handlers: Input location for the Event handler list.
> + */
> +void kshark_free_event_handler_list(struct kshark_event_handler *handlers)
> +{
> +	struct kshark_event_handler *last;
> +
> +	while (handlers) {
> +		last = handlers;
> +		handlers = handlers->next;
> +		free(last);
> +	}
> +}
> +
> +/**
> + * @brief Allocate memory for a new plugin. Add this plugin to the list of
> + *	  plugins used by the session.
> + *
> + * @param kshark_ctx: Input location for the session context pointer.
> + * @param file: The plugin object file to load.
> + *
> + * @returns Zero on success, or a negative error code on failure.
> + */
> +int kshark_register_plugin(struct kshark_context *kshark_ctx,
> +			   const char *file)
> +{
> +	struct kshark_plugin_list *plugin = kshark_ctx->plugins;
> +	struct stat st;
> +	int ret;
> +
> +	while (plugin) {
> +		if (strcmp(plugin->file, file) == 0)
> +			return -EEXIST;
> +
> +		plugin = plugin->next;
> +	}
> +
> +	ret = stat(file, &st);
> +	if (ret < 0) {
> +		fprintf(stderr, "plugin %s not found\n", file);
> +		return -ENODEV;
> +	}
> +
> +	plugin = calloc(sizeof(struct kshark_plugin_list), 1);
> +	if (!plugin) {
> +		fprintf(stderr, "failed to allocate memory for plugin\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (asprintf(&plugin->file, "%s", file) <= 0) {
> +		fprintf(stderr,
> +			"failed to allocate memory for plugin file name");
> +		return -ENOMEM;
> +	}
> +
> +	plugin->handle = dlopen(plugin->file, RTLD_NOW | RTLD_GLOBAL);
> +	if (!plugin->handle)
> +		goto fail;
> +
> +	plugin->init = dlsym(plugin->handle,
> +			     KSHARK_PLUGIN_INITIALIZER_NAME);
> +
> +	plugin->close = dlsym(plugin->handle,
> +			      KSHARK_PLUGIN_DEINITIALIZER_NAME);
> +
> +	if (!plugin->init || !plugin->close)
> +		goto fail;
> +
> +	plugin->next = kshark_ctx->plugins;
> +	kshark_ctx->plugins = plugin;
> +
> +	return 0;
> +
> + fail:
> +	fprintf(stderr, "cannot load plugin '%s'\n%s\n",
> +		plugin->file, dlerror());
> +
> +	if (plugin->handle) {
> +		dlclose(plugin->handle);
> +		plugin->handle = NULL;
> +	}
> +
> +	free(plugin);
> +
> +	return EFAULT;
> +}
> +
> +/**
> + * @brief Unrgister a plugin.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + * @param file: The plugin object file to unregister.
> + */
> +void kshark_unregister_plugin(struct kshark_context *kshark_ctx,
> +			      const char *file)
> +{
> +	struct kshark_plugin_list **last;
> +
> +	for (last = &kshark_ctx->plugins; *last; last = &(*last)->next) {
> +		if (strcmp((*last)->file, file) == 0) {
> +			struct kshark_plugin_list *this_plugin;
> +			this_plugin = *last;
> +			*last = this_plugin->next;
> +
> +			dlclose(this_plugin->handle);
> +			free(this_plugin);
> +
> +			return;
> +		}
> +	}
> +}
> +
> +/**
> + * @brief Free all plugins in a given list.
> + *
> + * @param plugins: Input location for the plugins list.
> + */
> +void kshark_free_plugin_list(struct kshark_plugin_list *plugins)
> +{
> +	struct kshark_plugin_list *last;
> +
> +	while (plugins) {
> +		last = plugins;
> +		plugins = plugins->next;
> +
> +		free(last->file);
> +		dlclose(last->handle);
> +
> +		free(last);
> +	}
> +}
> +
> +/**
> + * @brief Use this function to initialize/update/deinitialize all registered
> + *	  plugins.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + * @param task_id: Action identifier specifying the action to be executed.
> + *
> + * @returns The number of successful added/removed plugin handlers on success,
> + *	    or a negative error code on failure.
> + */
> +int kshark_handle_plugins(struct kshark_context *kshark_ctx,
> +			  int task_id)

Shouldn't task_id be of type: enum kshark_plugin_actions ?

> +{
> +	struct kshark_plugin_list *plugin;
> +	int handler_count = 0;
> +
> +	if (task_id != KSHARK_PLUGIN_INIT &&
> +	    task_id != KSHARK_PLUGIN_UPDATE &&
> +	    task_id != KSHARK_PLUGIN_CLOSE)
> +		return -EINVAL;

You can simplify the above with:

	switch (task_id) {
	case KSHARK_PLUGIN_INIT:
	case KSHARK_PLUGIN_UPDATE:
	case KSHARK_PLUGIN_CLOSE:
		break;
	default:
		return -EINVAL;
	}

Or, if you want to not even bother, you can add below:

> +
> +	for (plugin = kshark_ctx->plugins; plugin; plugin = plugin->next) {
> +		switch (task_id) {
> +		case KSHARK_PLUGIN_INIT:
> +			handler_count += plugin->init(kshark_ctx);
> +			break;
> +
> +		case KSHARK_PLUGIN_UPDATE:
> +			plugin->close(kshark_ctx);
> +			handler_count += plugin->init(kshark_ctx);
> +			break;
> +
> +		case KSHARK_PLUGIN_CLOSE:
> +			handler_count += plugin->close(kshark_ctx);
> +			break;
		default;
			return -EINVAL;

As a switch needs to have the jumps regardless. There's no need to do
the test first. The only difference is, it wont error if there's no
plugins. But is that a problem?

-- Steve

> +		}
> +	}
> +
> +	return handler_count;
> +}

Re: [PATCH v4 2/7] kernel-shark-qt: Add Plugin event handlers to session.

[Steven Rostedt]

On Wed, 19 Sep 2018 17:36:52 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> Plugin event handlers are added to the Session context descriptor.
> The handlers are used to execute plugin-specific action (callback
> function) during the processing of the trace data.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark-qt/src/libkshark.c | 21 +++++++++++++++++++++
>  kernel-shark-qt/src/libkshark.h | 12 ++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
> index b4a76ae..6fb08b2 100644
> --- a/kernel-shark-qt/src/libkshark.c
> +++ b/kernel-shark-qt/src/libkshark.c
> @@ -33,6 +33,9 @@ static bool kshark_default_context(struct kshark_context **context)
>  	if (!kshark_ctx)
>  		return false;
>  
> +	kshark_ctx->event_handlers = NULL;
> +	kshark_ctx->plugins = NULL;
> +
>  	kshark_ctx->show_task_filter = tracecmd_filter_id_hash_alloc();
>  	kshark_ctx->hide_task_filter = tracecmd_filter_id_hash_alloc();
>  
> @@ -217,6 +220,12 @@ void kshark_free(struct kshark_context *kshark_ctx)
>  	tracecmd_filter_id_hash_free(kshark_ctx->show_event_filter);
>  	tracecmd_filter_id_hash_free(kshark_ctx->hide_event_filter);
>  
> +	if(kshark_ctx->plugins) {

Need space between "if" and "("

> +		kshark_handle_plugins(kshark_ctx, KSHARK_PLUGIN_CLOSE);
> +		kshark_free_plugin_list(kshark_ctx->plugins);
> +		kshark_free_event_handler_list(kshark_ctx->event_handlers);
> +	}
> +
>  	kshark_free_task_list(kshark_ctx);
>  
>  	if (seq.buffer)
> @@ -564,6 +573,7 @@ static void free_rec_list(struct rec_list **rec_list, int n_cpus,
>  static size_t get_records(struct kshark_context *kshark_ctx,
>  			  struct rec_list ***rec_list, enum rec_type type)
>  {
> +	struct kshark_event_handler *evt_handler;
>  	struct event_filter *adv_filter;
>  	struct kshark_task_list *task;
>  	struct tep_record *rec;
> @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx,
>  
>  				entry = &temp_rec->entry;
>  				kshark_set_entry_values(kshark_ctx, rec, entry);
> +
> +				/* Execute all plugin-provided actions (if any). */
> +				evt_handler = kshark_ctx->event_handlers;
> +				while ((evt_handler = find_event_handler(evt_handler,
> +									 entry->event_id))) {
> +					evt_handler->event_func(kshark_ctx, rec, entry);
> +
> +					if ((evt_handler = evt_handler->next))

Please break the above if, to remove the assignment. It's just easier
to read. That is:

					event_handler = event_hanndler->next;
					if (event_handler)
> +						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> +				}
> +
>  				pid = entry->pid;
>  				/* Apply event filtering. */
>  				ret = FILTER_MATCH;
> diff --git a/kernel-shark-qt/src/libkshark.h b/kernel-shark-qt/src/libkshark.h
> index fda133c..203c812 100644
> --- a/kernel-shark-qt/src/libkshark.h
> +++ b/kernel-shark-qt/src/libkshark.h
> @@ -29,6 +29,9 @@ extern "C" {
>  #include "event-parse.h"
>  #include "trace-filter-hash.h"
>  
> +// KernelShark
> +#include "libkshark-plugin.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
> @@ -124,6 +127,9 @@ struct kshark_context {
>  
>  	/** List of Plugins. */
>  	struct kshark_plugin_list	*plugins;
> +
> +	/** List of Plugin Event handlers. */
> +	struct kshark_event_handler	*event_handlers;
>  };
>  
>  bool kshark_instance(struct kshark_context **kshark_ctx);
> @@ -160,6 +166,12 @@ enum kshark_filter_masks {
>  
>  	/** Special mask used whene filtering events. */
>  	KS_EVENT_VIEW_FILTER_MASK	= 1 << 2,
> +
> +	/**
> +	 * Use this mask to check if the content of the entry has been accessed
> +	 * by a plugin-defined function.
> +	 */
> +	KS_PLUGIN_UNTOUCHED_MASK	= 1 << 7

Why the jump to 7?

-- Steve

>  };
>  
>  /** Filter type identifier. */

Re: [PATCH v4 4/7] kernel-shark-qt: Make kshark_read_at() non-static.

[Steven Rostedt]

On Wed, 19 Sep 2018 17:36:54 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:


> +/**
> + * @brief A thread-safe read of a record from a specific offset.
> + *
> + * @param kshark_ctx: Input location for the session context pointer.
> + * @param offset: the offset into the file to find the record.
> + *
> + * @returns The returned pevent_record must be freed.
> + */
> +struct tep_record *kshark_read_at(struct kshark_context *kshark_ctx,
> +				  uint64_t offset)
>  {
>  	/*
>  	 * It turns that tracecmd_read_at() is not thread-safe.

This code still has:

	/*
	 * 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.
	 */

Remind me to explain to you why this is the case. It wont be easy to
have it fixed.

-- Steve

Re: [PATCH v4 7/7] kernel-shark-qt: Add a plugin for sched events.

[Steven Rostedt]

On Wed, 19 Sep 2018 17:36:57 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> The plugin is responsible for the following actions, specific for
> "sched" events:
> 
> 1. Changes the value of the "pid" field of the "sched_switch" entries.
>    This alows the sched_switch entry to be plotted as part of the "next"
>    task.
> 
> 2. On "sched_switch" event, the plugin registers the "next" task (if not
>    registered already).
> 
> 3. Plots in green the wake up latency of the task and in red the time
>    the task was preempted by another task.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark-qt/src/plugins/CMakeLists.txt  |   5 +
>  kernel-shark-qt/src/plugins/SchedEvents.cpp | 263 +++++++++++++++++
>  kernel-shark-qt/src/plugins/sched_events.c  | 294 ++++++++++++++++++++
>  kernel-shark-qt/src/plugins/sched_events.h  |  76 +++++
>  4 files changed, 638 insertions(+)
>  create mode 100644 kernel-shark-qt/src/plugins/SchedEvents.cpp
>  create mode 100644 kernel-shark-qt/src/plugins/sched_events.c
>  create mode 100644 kernel-shark-qt/src/plugins/sched_events.h
> 
> diff --git a/kernel-shark-qt/src/plugins/CMakeLists.txt b/kernel-shark-qt/src/plugins/CMakeLists.txt
> index 565f1cb..88fd93c 100644
> --- a/kernel-shark-qt/src/plugins/CMakeLists.txt
> +++ b/kernel-shark-qt/src/plugins/CMakeLists.txt
> @@ -19,4 +19,9 @@ endfunction()
>  
>  set(PLUGIN_LIST "")
>  
> +BUILD_PLUGIN(NAME sched_events
> +             SOURCE sched_events.c SchedEvents.cpp)
> +list(APPEND PLUGIN_LIST "sched_events default") # This plugin will be loaded by default
> +# list(APPEND PLUGIN_LIST "sched_events") # This plugin isn't loaded by default
> +
>  set(PLUGINS ${PLUGIN_LIST} PARENT_SCOPE)
> diff --git a/kernel-shark-qt/src/plugins/SchedEvents.cpp b/kernel-shark-qt/src/plugins/SchedEvents.cpp
> new file mode 100644
> index 0000000..04d9a50
> --- /dev/null
> +++ b/kernel-shark-qt/src/plugins/SchedEvents.cpp
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +
> +/*
> + * Copyright (C) 2018 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
> + */
> +
> +/**
> + *  @file    SchedEvents.cpp
> + *  @brief   Defines a callback function for Sched events used to plot in green
> + *	     the wake up latency of the task and in red the time the task was
> + *	     preempted by another task.
> + */
> +
> +// C++
> +#include<iostream>
> +
> +// C++ 11
> +#include<functional>
> +
> +// KernelShark
> +#include "libkshark.h"
> +#include "plugins/sched_events.h"
> +#include "KsPlotTools.hpp"
> +#include "KsPlugins.hpp"
> +
> +//! @cond Doxygen_Suppress
> +
> +#define PLUGIN_MIN_BOX_SIZE 4
> +
> +#define PLUGIN_MAX_ENTRIES_PER_BIN 500
> +
> +//! @endcond
> +
> +extern struct plugin_sched_context *plugin_sched_context_handler;
> +
> +static int plugin_get_wakeup_pid(kshark_context *kshark_ctx,
> +				 plugin_sched_context *plugin_ctx,
> +				 const struct kshark_entry *e)
> +{
> +	struct tep_record *record;
> +	unsigned long long val;
> +
> +	record = kshark_read_at(kshark_ctx, e->offset);
> +	tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
> +			      record->data, &val);
> +	free(record);

You can't just free a record. You need to call tep_free_record(record).

> +
> +	return val;
> +}
> +
> +/** Sched Event identifier. */
> +enum class SchedEvent {
> +	/** Sched Switch Event. */
> +	Switch,
> +
> +	/** Sched Wakeup Event. */
> +	Wakeup,
> +};
> +
> +static void pluginDraw(plugin_sched_context *plugin_ctx,
> +		       kshark_context *kshark_ctx,
> +		       kshark_trace_histo *histo,
> +		       kshark_entry_collection *col,
> +		       SchedEvent e,
> +		       int pid,
> +		       KsPlot::Graph *graph,
> +		       KsPlot::PlotObjList *shapes)
> +{
> +	std::function<void(int)> ifSchedBack;
> +	KsPlot::Rectangle *rec = nullptr;
> +	int height = graph->getHeight() * .3;
> +
> +	auto openBox = [&] (const KsPlot::Point &p)
> +	{
> +		/*
> +		 * First check if we already have an open box. If we don't
> +		 * have, open a new one.
> +		 */
> +		if (!rec)
> +			rec = new KsPlot::Rectangle;
> +
> +		rec->setFill(false);
> +		rec->setPoint(0, p.x() - 1, p.y() - height);
> +		rec->setPoint(1, p.x() - 1, p.y() - 1);
> +	};
> +
> +	auto closeBox = [&] (const KsPlot::Point &p)
> +	{
> +		if (rec == nullptr)
> +			return;
> +
> +		int boxSize = rec->getPoint(0)->x;
> +		if (boxSize < PLUGIN_MIN_BOX_SIZE) {
> +			/* This box is too small. Don't try to plot it. */
> +			delete rec;
> +			rec = nullptr;
> +			return;
> +		}
> +
> +		rec->setPoint(3, p.x() - 1, p.y() - height);
> +		rec->setPoint(2, p.x() - 1, p.y() - 1);
> +
> +		shapes->push_front(rec);
> +		rec = nullptr;
> +	};
> +
> +	auto lamIfSchSwitchFront = [&] (int bin)
> +	{
> +		/*
> +		 * Starting from the first element in this bin, go forward
> +		 * in time until you find a trace entry that satisfies the
> +		 * condition defined by kshark_match_pid.
> +		 */
> +		const kshark_entry *entryF =
> +			ksmodel_get_entry_front(histo, bin, false,
> +						kshark_match_pid, pid,
> +						col, nullptr);
> +
> +		if (entryF &&
> +		    entryF->pid == pid &&
> +		    plugin_ctx->sched_switch_event &&
> +		    entryF->event_id == plugin_ctx->sched_switch_event->id) {
> +			/*
> +			 * entryF is sched_switch_event. Close the box and add
> +			 * it to the list of shapes to be ploted.
> +			 */
> +			closeBox(graph->getBin(bin)._base);
> +		}
> +	};
> +
> +	auto lamIfSchWakeupBack = [&] (int bin)
> +	{
> +		/*
> +		 * Starting from the last element in this bin, go backward
> +		 * in time until you find a trace entry that satisfies the
> +		 * condition defined by plugin_wakeup_match_pid.
> +		 */
> +		const kshark_entry *entryB =
> +			ksmodel_get_entry_back(histo, bin, false,
> +					       plugin_wakeup_match_pid, pid,
> +					       col, nullptr);
> +		int wakeup_pid;
> +
> +		if (entryB &&
> +		    plugin_ctx->sched_wakeup_event &&
> +		    entryB->event_id == plugin_ctx->sched_wakeup_event->id) {
> +			wakeup_pid =
> +				plugin_get_wakeup_pid(kshark_ctx, plugin_ctx, entryB);
> +			if (wakeup_pid == pid) {
> +				/*
> +				 * entryB is a sched_wakeup_event. Open a
> +				 * green box here.
> +				 */
> +				openBox(graph->getBin(bin)._base);
> +
> +				 /* Green */
> +				rec->_color = KsPlot::Color(0, 255, 0);
> +			}
> +		}
> +	};
> +
> +	auto lamIfSchSwitchBack = [&] (int bin)
> +	{
> +		/*
> +		 * Starting from the last element in this bin, go backward
> +		 * in time until you find a trace entry that satisfies the
> +		 * condition defined by plugin_switch_match_pid.
> +		 */
> +		const kshark_entry *entryB =
> +			ksmodel_get_entry_back(histo, bin, false,
> +					       plugin_switch_match_pid, pid,
> +					       col, nullptr);
> +
> +		if (entryB &&
> +		    entryB->pid != pid &&
> +		    plugin_ctx->sched_switch_event &&
> +		    entryB->event_id == plugin_ctx->sched_switch_event->id) {
> +			/*
> +			 * entryB is a sched_switch_event. Open a
> +			 * red box here.
> +			 */
> +			openBox(graph->getBin(bin)._base);
> +
> +			/* Red */
> +			rec->_color = KsPlot::Color(255, 0, 0);
> +		}
> +	};
> +
> +	if (e == SchedEvent::Switch)
> +		ifSchedBack = lamIfSchSwitchBack;
> +	else
> +		ifSchedBack = lamIfSchWakeupBack;
> +
> +	for (int bin = 0; bin < graph->size(); ++bin) {
> +		/**
> +		 * Plotting the latencies makes sense only in the case of a
> +		 * deep zoom. Here we set a naive threshold based on the number
> +		 * of entries inside the current bin. This cut seems to work
> +		 * well in all cases I tested so far, but it may result in
> +		 * unexpected behavior with some unusual trace data-sets.
> +		 * TODO: find a better criteria for deciding when to start
> +		 * plotting latencies.
> +		 */
> +		if (ksmodel_bin_count(histo, bin) > PLUGIN_MAX_ENTRIES_PER_BIN)

I don't like this hard coding. Perhaps for now it's OK, but what about
doing a scan of all bins (can we hook in when we do a full scan in the
beginning, I believe we do scan a full plot right?), and calculate the
largest latency then, and then we know our threshold.

We can do this later (considering we need to start finishing up), but
would that work?

> +			continue;
> +
> +		lamIfSchSwitchFront(bin);
> +
> +		ifSchedBack(bin);
> +	}
> +
> +	if (rec)
> +		delete rec;
> +
> +	return;
> +}
> +
> +/**
> + * @brief Plugin's draw function.
> + *
> + * @param argv_c: A C pointer to be converted to KsCppArgV (C++ struct).
> + * @param pid: Process Id.
> + * @param draw_action: Draw action identifier.
> + *
> + * @returns True if the Pid of the entry matches the value of "pid".
> + *	    Otherwise false.
> + */
> +void plugin_draw(kshark_cpp_argv *argv_c, int pid, int draw_action)
> +{
> +	plugin_sched_context *plugin_ctx;
> +	kshark_context *kshark_ctx(NULL);
> +	kshark_entry_collection *col;
> +
> +	if (draw_action != KSHARK_PLUGIN_TASK_DRAW || pid == 0)
> +		return;
> +
> +	plugin_ctx = plugin_sched_context_handler;
> +	if (!plugin_ctx || !kshark_instance(&kshark_ctx))
> +		return;
> +
> +	KsCppArgV *argvCpp = KS_ARGV_TO_CPP(argv_c);
> +
> +	/*
> +	 * Try to find a collections for this task. It is OK if
> +	 * coll = NULL.
> +	 */
> +	col = kshark_find_data_collection(kshark_ctx->collections,
> +					  kshark_match_pid, pid);
> +
> +	try {
> +		pluginDraw(plugin_ctx, kshark_ctx,
> +			   argvCpp->_histo, col,
> +			   SchedEvent::Switch, pid,
> +			   argvCpp->_graph, argvCpp->_shapes);
> +
> +		pluginDraw(plugin_ctx, kshark_ctx,
> +			   argvCpp->_histo, col,
> +			   SchedEvent::Wakeup, pid,
> +			   argvCpp->_graph, argvCpp->_shapes);
> +	} catch (const std::exception &exc) {
> +		std::cerr << "Exception in SchedEvents\n" << exc.what();
> +	}
> +}
> diff --git a/kernel-shark-qt/src/plugins/sched_events.c b/kernel-shark-qt/src/plugins/sched_events.c
> new file mode 100644
> index 0000000..d761d7c
> --- /dev/null
> +++ b/kernel-shark-qt/src/plugins/sched_events.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +
> +/*
> + * Copyright (C) 2018 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
> + */
> +
> +/**
> + *  @file    sched_events.c
> + *  @brief   Defines a callback function for Sched events used to registers the
> + *	     "next" task (if not registered already) and to changes the value
> + *	     of the "pid" field of the "sched_switch" entries such that, it
> + *	     will be ploted as part of the "next" task.
> + */
> +
> +// C
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +// KernelShark
> +#include "plugins/sched_events.h"
> +
> +/** Plugin context instance. */
> +struct plugin_sched_context *plugin_sched_context_handler = NULL;
> +
> +static bool plugin_sched_update_context(struct kshark_context *kshark_ctx)
> +{
> +	struct plugin_sched_context *plugin_ctx;
> +	struct event_format *event;
> +
> +	if (!plugin_sched_context_handler) {
> +		plugin_sched_context_handler =
> +		 malloc(sizeof(*plugin_sched_context_handler));

Need to check if malloc failed.

> +	}
> +
> +	plugin_ctx = plugin_sched_context_handler;
> +	plugin_ctx->handle = kshark_ctx->handle;
> +	plugin_ctx->pevent = kshark_ctx->pevent;
> +
> +	event = tep_find_event_by_name(plugin_ctx->pevent,
> +				       "sched", "sched_switch");
> +	if (!event)
> +		return false;
> +
> +	plugin_ctx->sched_switch_event = event;
> +	plugin_ctx->sched_switch_next_field =
> +		tep_find_any_field(event, "next_pid");
> +
> +	plugin_ctx->sched_switch_comm_field =
> +		tep_find_field(event, "next_comm");
> +
> +	event = tep_find_event_by_name(plugin_ctx->pevent,
> +				      "sched", "sched_wakeup");
> +	if (!event)
> +		return false;
> +
> +	plugin_ctx->sched_wakeup_event = event;
> +	plugin_ctx->sched_wakeup_pid_field =
> +		tep_find_any_field(event, "pid");
> +
> +	plugin_ctx->sched_wakeup_success_field =
> +		tep_find_field(event, "success");
> +
> +	event = tep_find_event_by_name(plugin_ctx->pevent,
> +				       "sched", "sched_wakeup_new");
> +	if (!event)
> +		return false;
> +
> +	plugin_ctx->sched_wakeup_new_event = event;
> +	plugin_ctx->sched_wakeup_new_pid_field =
> +		tep_find_any_field(event, "pid");
> +
> +	plugin_ctx->sched_wakeup_new_success_field =
> +		tep_find_field(event, "success");
> +
> +	return true;
> +}
> +
> +/**
> + * @brief Get the Process Id of the next scheduled task.
> + *
> + * @param record: Input location for a sched_switch record.
> + */
> +int plugin_get_next_pid(struct tep_record *record)
> +{
> +	struct plugin_sched_context *plugin_ctx =
> +		plugin_sched_context_handler;
> +	unsigned long long val;
> +
> +	tep_read_number_field(plugin_ctx->sched_switch_next_field,
> +			      record->data, &val);
> +	return val;
> +}
> +
> +/**
> + * @brief Get the Process Id of the task being woke up.
> + *
> + * @param record: Input location for a sched_wakeup record.
> + */
> +int plugin_get_wakeup_pid(struct tep_record *record)
> +{
> +	struct plugin_sched_context *plugin_ctx =
> +		plugin_sched_context_handler;
> +	unsigned long long val;
> +
> +	tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
> +			      record->data, &val);
> +	return val;
> +}
> +
> +static void plugin_register_command(struct kshark_context *kshark_ctx,
> +				    struct tep_record *record,
> +				    int pid)
> +{
> +	struct plugin_sched_context *plugin_ctx =
> +		plugin_sched_context_handler;
> +	const char *comm;
> +
> +	if (!plugin_ctx->sched_switch_comm_field)
> +		return;
> +
> +	comm = record->data + plugin_ctx->sched_switch_comm_field->offset;

TODO: The above needs a wrapper function. Not KernelShark related, but
libtracevent. This may be something Tzvetomir should handle.


> +
> +	if (!tep_pid_is_registered(kshark_ctx->pevent, pid))
> +			tep_register_comm(kshark_ctx->pevent, comm, pid);
> +}
> +
> +static int plugin_get_wakeup_new_pid(struct tep_record *record)
> +{
> +	struct plugin_sched_context *plugin_ctx =
> +		plugin_sched_context_handler;
> +	unsigned long long val;
> +
> +	tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
> +				 record->data, &val);
> +
> +	return val;
> +}
> +
> +/**
> + * @brief Process Id matching function adapted for sched_wakeup and
> + *	  sched_wakeup_new events.
> + *
> + * @param kshark_ctx: Input location for the session context pointer.
> + * @param e: kshark_entry to be checked.
> + * @param pid: Matching condition value.
> + *
> + * @returns True if the Pid of the entry matches the value of "pid".
> + *	    Otherwise false.
> + */
> +bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
> +			     struct kshark_entry *e,
> +			     int pid)
> +{
> +	struct plugin_sched_context *plugin_ctx;
> +	struct tep_record *record = NULL;
> +	unsigned long long val;
> +	int wakeup_pid = -1;
> +
> +	if (e->pid == pid)
> +		return true;
> +
> +	plugin_ctx = plugin_sched_context_handler;
> +	if (!plugin_ctx)
> +		return false;
> +
> +	if (plugin_ctx->sched_wakeup_event &&
> +	    e->event_id == plugin_ctx->sched_wakeup_event->id) {
> +		record = kshark_read_at(kshark_ctx, e->offset);
> +
> +		/* We only want those that actually woke up the task. */
> +		tep_read_number_field(plugin_ctx->sched_wakeup_success_field,
> +				      record->data, &val);
> +
> +		if (val)
> +			wakeup_pid = plugin_get_wakeup_pid(record);
> +	}
> +
> +	if (plugin_ctx->sched_wakeup_new_event &&
> +	    e->event_id == plugin_ctx->sched_wakeup_new_event->id) {
> +		record = kshark_read_at(kshark_ctx, e->offset);
> +
> +		/* We only want those that actually woke up the task. */
> +		tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field,
> +				      record->data, &val);
> +
> +		if (val)
> +			wakeup_pid = plugin_get_wakeup_new_pid(record);
> +	}
> +
> +	free(record);

Again, this needs to be tep_free_record(record);

> +
> +	if (wakeup_pid >= 0 && wakeup_pid == pid)
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
> + * @brief Process Id matching function adapted for sched_switch events.
> + *
> + * @param kshark_ctx: Input location for the session context pointer.
> + * @param e: kshark_entry to be checked.
> + * @param pid: Matching condition value.
> + *
> + * @returns True if the Pid of the entry matches the value of "pid".
> + *	    Otherwise false.
> + */
> +bool plugin_switch_match_pid(struct kshark_context *kshark_ctx,
> +			     struct kshark_entry *e,
> +			     int pid)
> +{
> +	struct plugin_sched_context *plugin_ctx;
> +	struct tep_record *record = NULL;
> +	int switch_pid = -1;
> +
> +	if (e->pid == pid)
> +		return true;
> +
> +	plugin_ctx = plugin_sched_context_handler;
> +
> +	if (plugin_ctx->sched_switch_event &&
> +	    e->event_id == plugin_ctx->sched_switch_event->id) {
> +		record = kshark_read_at(kshark_ctx, e->offset);
> +
> +		switch_pid = tep_data_pid(plugin_ctx->pevent, record);
> +	}
> +
> +	free(record);

Here too.


-- Steve


> +
> +	if (switch_pid >= 0 && switch_pid == pid)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void plugin_sched_action(struct kshark_context *kshark_ctx,
> +				struct tep_record *rec,
> +				struct kshark_entry *entry)
> +{
> +	entry->pid = plugin_get_next_pid(rec);
> +	plugin_register_command(kshark_ctx, rec, entry->pid);
> +}
> +
> +static int plugin_sched_init(struct kshark_context *kshark_ctx)
> +{
> +	struct plugin_sched_context *plugin_ctx;
> +
> +	if (!plugin_sched_update_context(kshark_ctx)) {
> +		free(plugin_sched_context_handler);
> +		plugin_sched_context_handler = NULL;
> +		return 0;
> +	}
> +
> +	plugin_ctx = plugin_sched_context_handler;
> +
> +	kshark_register_event_handler(&kshark_ctx->event_handlers,
> +				      plugin_ctx->sched_switch_event->id,
> +				      plugin_sched_action,
> +				      plugin_draw);
> +
> +	return 1;
> +}
> +
> +static int plugin_sched_close(struct kshark_context *kshark_ctx)
> +{
> +	struct plugin_sched_context *plugin_ctx;
> +
> +	if (!plugin_sched_context_handler)
> +		return 0;
> +
> +	plugin_ctx = plugin_sched_context_handler;
> +
> +	kshark_unregister_event_handler(&kshark_ctx->event_handlers,
> +					plugin_ctx->sched_switch_event->id,
> +					plugin_sched_action,
> +					plugin_draw);
> +
> +	free(plugin_ctx);
> +	plugin_sched_context_handler = NULL;
> +
> +	return 1;
> +}
> +
> +/** Load this plugin. */
> +int KSHARK_PLUGIN_INITIALIZER(struct kshark_context *kshark_ctx)
> +{
> +	return plugin_sched_init(kshark_ctx);
> +}
> +
> +/** Unload this plugin. */
> +int KSHARK_PLUGIN_DEINITIALIZER(struct kshark_context *kshark_ctx)
> +{
> +	return plugin_sched_close(kshark_ctx);
> +}
> diff --git a/kernel-shark-qt/src/plugins/sched_events.h b/kernel-shark-qt/src/plugins/sched_events.h
> new file mode 100644
> index 0000000..673f5cf
> --- /dev/null
> +++ b/kernel-shark-qt/src/plugins/sched_events.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: LGPL-2.1 */
> +
> +/*
> + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
> + */
> +
> +/**
> + *  @file    sched_events.h
> + *  @brief   Plugin for Sched events.
> + */
> +
> +#ifndef _KS_PLUGIN_SHED_H
> +#define _KS_PLUGIN_SHED_H
> +
> +// KernelShark
> +#include "libkshark.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/** Structure representing a plugin-specific context. */
> +struct plugin_sched_context {
> +	/** Input handle for the trace data file. */
> +	struct tracecmd_input	*handle;
> +
> +	/** Page event used to parse the page. */
> +	struct tep_handle	*pevent;
> +
> +	/** Pointer to the sched_switch_event object. */
> +	struct event_format	*sched_switch_event;
> +
> +	/** Pointer to the sched_switch_next_field format descriptor. */
> +	struct format_field	*sched_switch_next_field;
> +
> +	/** Pointer to the sched_switch_comm_field format descriptor. */
> +	struct format_field	*sched_switch_comm_field;
> +
> +	/** Pointer to the sched_wakeup_event object. */
> +	struct event_format	*sched_wakeup_event;
> +
> +	/** Pointer to the sched_wakeup_pid_field format descriptor. */
> +	struct format_field	*sched_wakeup_pid_field;
> +
> +	/** Pointer to the sched_wakeup_success_field format descriptor. */
> +	struct format_field	*sched_wakeup_success_field;
> +
> +	/** Pointer to the sched_wakeup_new_event object. */
> +	struct event_format	*sched_wakeup_new_event;
> +
> +	/** Pointer to the sched_wakeup_new_pid_field format descriptor. */
> +	struct format_field	*sched_wakeup_new_pid_field;
> +
> +	/**
> +	 * Pointer to the sched_wakeup_new_success_field format descriptor.
> +	 */
> +	struct format_field	*sched_wakeup_new_success_field;
> +};
> +
> +int plugin_get_next_pid(struct tep_record *record);
> +
> +int plugin_get_wakeup_pid(struct tep_record *record);
> +
> +bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
> +			     struct kshark_entry *e, int pid);
> +
> +bool plugin_switch_match_pid(struct kshark_context *kshark_ctx,
> +			     struct kshark_entry *e, int pid);
> +
> +void plugin_draw(struct kshark_cpp_argv *argv, int pid, int draw_action);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif

Re: [PATCH v4 2/7] kernel-shark-qt: Add Plugin event handlers to session.

[Yordan Karadzhov (VMware)]

On 20.09.2018 06:24, Steven Rostedt wrote:
>>   	/** Special mask used whene filtering events. */
>>   	KS_EVENT_VIEW_FILTER_MASK	= 1 << 2,
>> +
>> +	/**
>> +	 * Use this mask to check if the content of the entry has been accessed
>> +	 * by a plugin-defined function.
>> +	 */
>> +	KS_PLUGIN_UNTOUCHED_MASK	= 1 << 7
> Why the jump to 7?

The "visible" field of the entry will use 8 bits. I know that it is 
uint16_t now, but my plan is to use 8 of its bits for the stream_id field.

Currently we use the first 3 bits as different visibility flags and we 
have 4 unused bits available for adding more visibility flags in the 
future. Because the PLUGIN_UNTOUCHED flag has a different meaning I 
decided to place it at the very end.

Is this a problem?

Thanks!
Yordan

Re: [PATCH v4 2/7] kernel-shark-qt: Add Plugin event handlers to session.

[Yordan Karadzhov (VMware)]

On 20.09.2018 06:24, Steven Rostedt wrote:
>> static size_t get_records(struct kshark_context *kshark_ctx,
>>   			  struct rec_list ***rec_list, enum rec_type type)
>>   {
>> +	struct kshark_event_handler *evt_handler;
>>   	struct event_filter *adv_filter;
>>   	struct kshark_task_list *task;
>>   	struct tep_record *rec;
>> @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx,
>>   
>>   				entry = &temp_rec->entry;
>>   				kshark_set_entry_values(kshark_ctx, rec, entry);
>> +
>> +				/* Execute all plugin-provided actions (if any). */
>> +				evt_handler = kshark_ctx->event_handlers;
>> +				while ((evt_handler = find_event_handler(evt_handler,
>> +									 entry->event_id))) {
>> +					evt_handler->event_func(kshark_ctx, rec, entry);
>> +
>> +					if ((evt_handler = evt_handler->next))
> Please break the above if, to remove the assignment. It's just easier
> to read. That is:
> 
> 					event_handler = event_hanndler->next;
> 					if (event_handler)
>> +						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
>> +				}
>> +

Actually this is a bug (typo). I meant to have

					evt_handler = evt_handler->next;
					if (!evt_handler)
						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;

This way the KS_PLUGIN_UNTOUCHED flag gets unset only once and it is 
guaranteed that the value of the bit cannot be changed by the plugin 
callback function.

Thanks!
Yordan


>>   				pid = entry->pid;
>>   				/* Apply event filtering. */
>>   				ret = FILTER_MATCH;

Re: [PATCH v4 2/7] kernel-shark-qt: Add Plugin event handlers to session.

[Steven Rostedt]

On Thu, 20 Sep 2018 16:26:11 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> On 20.09.2018 06:24, Steven Rostedt wrote:
> >>   	/** Special mask used whene filtering events. */
> >>   	KS_EVENT_VIEW_FILTER_MASK	= 1 << 2,
> >> +
> >> +	/**
> >> +	 * Use this mask to check if the content of the entry has been accessed
> >> +	 * by a plugin-defined function.
> >> +	 */
> >> +	KS_PLUGIN_UNTOUCHED_MASK	= 1 << 7  
> > Why the jump to 7?  
> 
> The "visible" field of the entry will use 8 bits. I know that it is 
> uint16_t now, but my plan is to use 8 of its bits for the stream_id field.
> 
> Currently we use the first 3 bits as different visibility flags and we 
> have 4 unused bits available for adding more visibility flags in the 
> future. Because the PLUGIN_UNTOUCHED flag has a different meaning I 
> decided to place it at the very end.
> 
> Is this a problem?

No, it just seems out of place. Can you add a comment after the
VIEW_FILTER_MASK stating that the next 4 bits are reserved for more
VIEW bits.

-- Steve

Re: [PATCH v4 2/7] kernel-shark-qt: Add Plugin event handlers to session.

[Steven Rostedt]

On Thu, 20 Sep 2018 16:29:00 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> On 20.09.2018 06:24, Steven Rostedt wrote:
> >> static size_t get_records(struct kshark_context *kshark_ctx,
> >>   			  struct rec_list ***rec_list, enum rec_type type)
> >>   {
> >> +	struct kshark_event_handler *evt_handler;
> >>   	struct event_filter *adv_filter;
> >>   	struct kshark_task_list *task;
> >>   	struct tep_record *rec;
> >> @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx,
> >>   
> >>   				entry = &temp_rec->entry;
> >>   				kshark_set_entry_values(kshark_ctx, rec, entry);
> >> +
> >> +				/* Execute all plugin-provided actions (if any). */
> >> +				evt_handler = kshark_ctx->event_handlers;
> >> +				while ((evt_handler = find_event_handler(evt_handler,
> >> +									 entry->event_id))) {
> >> +					evt_handler->event_func(kshark_ctx, rec, entry);
> >> +
> >> +					if ((evt_handler = evt_handler->next))  
> > Please break the above if, to remove the assignment. It's just easier
> > to read. That is:
> > 
> > 					event_handler = event_hanndler->next;
> > 					if (event_handler)  
> >> +						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> >> +				}
> >> +  
> 
> Actually this is a bug (typo). I meant to have

 :-)

That's actually one of the reasons the combination is looked down upon
in the Linux kernel. It's more likely to hide bugs from reviewers.

-- Steve

> 
> 					evt_handler = evt_handler->next;
> 					if (!evt_handler)
> 						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> 
> This way the KS_PLUGIN_UNTOUCHED flag gets unset only once and it is 
> guaranteed that the value of the bit cannot be changed by the plugin 
> callback function.
>