[PATCH 1/2] kernel-shark: Move common APIs and definitions out to avoid duplication

[PATCH 1/2] kernel-shark: Move common APIs and definitions out to avoid duplication

[Hongzhan Chen]

To avoid code duplication, move some common APIs and definitions
out from plugin SchedEvent to share with other plugins.

Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>

diff --git a/src/KsPlugins.hpp b/src/KsPlugins.hpp
index d41d094..4b29fa6 100644
--- a/src/KsPlugins.hpp
+++ b/src/KsPlugins.hpp
@@ -13,6 +13,7 @@
 #define _KS_PLUGINS_H
 
 // C++
+#include <vector>
 #include <functional>
 
 // KernelShark
@@ -101,4 +102,66 @@ void eventFieldIntervalPlot(KsCppArgV *argvCpp,
 			    KsPlot::Color col,
 			    float size);
 
+/**
+ * This class represents the graphical element visualizing the latency between
+ * two events such as sched_waking and sched_switch events for common Linux
+ * kernel or cobalt_switch_contexts for Xenomai.
+ */
+class LatencyBox : public KsPlot::Rectangle
+{
+	/** On double click do. */
+	void _doubleClick() const override {}
+
+public:
+	/** The trace record data that corresponds to this LatencyBox. */
+	std::vector<kshark_data_field_int64 *>	_data;
+
+	/**
+	 * @brief Distance between the click and the shape. Used to decide if
+	 *	  the double click action must be executed.
+	 *
+	 * @param x: X coordinate of the click.
+	 * @param y: Y coordinate of the click.
+	 *
+	 * @returns If the click is inside the box, the distance is zero.
+	 *	    Otherwise infinity.
+	 */
+	double distance(int x, int y) const override
+	{
+		if (x < pointX(0) || x > pointX(2))
+			return std::numeric_limits<double>::max();
+
+		if (y < pointY(0) || y > pointY(1))
+			return std::numeric_limits<double>::max();
+
+		return 0;
+	}
+};
+
+template<class T> KsPlot::PlotObject *
+makeLatencyBox(std::vector<const KsPlot::Graph *> graph,
+			     std::vector<int> bins,
+			     std::vector<kshark_data_field_int64 *> data,
+			     KsPlot::Color col, float size)
+{
+	LatencyBox *rec = new T;
+	rec->_data = data;
+
+	KsPlot::Point p0 = graph[0]->bin(bins[0])._base;
+	KsPlot::Point p1 = graph[0]->bin(bins[1])._base;
+	int height = graph[0]->height() * .3;
+
+	rec->setFill(false);
+	rec->setPoint(0, p0.x() - 1, p0.y() - height);
+	rec->setPoint(1, p0.x() - 1, p0.y() - 1);
+
+	rec->setPoint(3, p1.x() - 1, p1.y() - height);
+	rec->setPoint(2, p1.x() - 1, p1.y() - 1);
+
+	rec->_size = size;
+	rec->_color = col;
+
+	return rec;
+}
+
 #endif
diff --git a/src/plugins/SchedEvents.cpp b/src/plugins/SchedEvents.cpp
index b73e45f..573e4e3 100644
--- a/src/plugins/SchedEvents.cpp
+++ b/src/plugins/SchedEvents.cpp
@@ -11,9 +11,6 @@
  *	     preempted by another task.
  */
 
-// C++
-#include <vector>
-
 // KernelShark
 #include "libkshark.h"
 #include "libkshark-plugin.h"
@@ -24,7 +21,7 @@
 
 using namespace KsPlot;
 
-static KsMainWindow *ks_ptr;
+static KsMainWindow *ks_schedevent_ptr;
 
 /**
  * @brief Provide the plugin with a pointer to the KsMainWindow object (the GUI
@@ -32,7 +29,7 @@ static KsMainWindow *ks_ptr;
  */
 __hidden void *plugin_set_gui_ptr(void *gui_ptr)
 {
-	ks_ptr = static_cast<KsMainWindow *>(gui_ptr);
+	ks_schedevent_ptr = static_cast<KsMainWindow *>(gui_ptr);
 	return nullptr;
 }
 
@@ -40,64 +37,15 @@ __hidden void *plugin_set_gui_ptr(void *gui_ptr)
  * This class represents the graphical element visualizing the latency between
  *  sched_waking and sched_switch events.
  */
-class LatencyBox : public Rectangle
+class SchedLatencyBox : public LatencyBox
 {
 	/** On double click do. */
 	void _doubleClick() const override
 	{
-		ks_ptr->markEntry(_data[1]->entry, DualMarkerState::B);
-		ks_ptr->markEntry(_data[0]->entry, DualMarkerState::A);
+		ks_schedevent_ptr->markEntry(_data[1]->entry, DualMarkerState::B);
+		ks_schedevent_ptr->markEntry(_data[0]->entry, DualMarkerState::A);
 	}
 
-public:
-	/** The trace record data that corresponds to this LatencyBox. */
-	std::vector<kshark_data_field_int64 *>	_data;
-
-	/**
-	 * @brief Distance between the click and the shape. Used to decide if
-	 *	  the double click action must be executed.
-	 *
-	 * @param x: X coordinate of the click.
-	 * @param y: Y coordinate of the click.
-	 *
-	 * @returns If the click is inside the box, the distance is zero.
-	 *	    Otherwise infinity.
-	 */
-	double distance(int x, int y) const override
-	{
-		if (x < pointX(0) || x > pointX(2))
-			return std::numeric_limits<double>::max();
-
-		if (y < pointY(0) || y > pointY(1))
-			return std::numeric_limits<double>::max();
-
-		return 0;
-	}
-};
-
-static PlotObject *makeShape(std::vector<const Graph *> graph,
-			     std::vector<int> bins,
-			     std::vector<kshark_data_field_int64 *> data,
-			     Color col, float size)
-{
-	LatencyBox *rec = new LatencyBox;
-	rec->_data = data;
-
-	Point p0 = graph[0]->bin(bins[0])._base;
-	Point p1 = graph[0]->bin(bins[1])._base;
-	int height = graph[0]->height() * .3;
-
-	rec->setFill(false);
-	rec->setPoint(0, p0.x() - 1, p0.y() - height);
-	rec->setPoint(1, p0.x() - 1, p0.y() - 1);
-
-	rec->setPoint(3, p1.x() - 1, p1.y() - height);
-	rec->setPoint(2, p1.x() - 1, p1.y() - 1);
-
-	rec->_size = size;
-	rec->_color = col;
-
-	return rec;
 };
 
 /*
@@ -191,14 +139,14 @@ __hidden void plugin_draw(kshark_cpp_argv *argv_c,
 	eventFieldIntervalPlot(argvCpp,
 			       plugin_ctx->sw_data, checkFieldSW,
 			       plugin_ctx->ss_data, checkEntryPid,
-			       makeShape,
+			       makeLatencyBox<SchedLatencyBox>,
 			       {0, 255, 0}, // Green
 			       -1);         // Default size
 
 	eventFieldIntervalPlot(argvCpp,
 			       plugin_ctx->ss_data, checkFieldSS,
 			       plugin_ctx->ss_data, checkEntryPid,
-			       makeShape,
+			       makeLatencyBox<SchedLatencyBox>,
 			       {255, 0, 0}, // Red
 			       -1);         // Default size
 }
diff --git a/src/plugins/common_sched.h b/src/plugins/common_sched.h
new file mode 100644
index 0000000..1d564c0
--- /dev/null
+++ b/src/plugins/common_sched.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: LGPL-2.1 */
+
+/*
+ * Copyright (C) 2021 Intel Inc, Hongzhan Chen <hongzhan.chen@intel.com>
+ */
+
+/**
+ *  @file    common_sched.h
+ *  @brief   Plugin for common sched.
+ */
+
+#ifndef _KS_PLUGIN_COMMON_SCHED_H
+#define _KS_PLUGIN_COMMON_SCHED_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef unsigned long long tep_num_field_t;
+
+/** The type of the data field stored in the kshark_data_container object. */
+typedef int64_t ks_num_field_t;
+
+#define PREV_STATE_SHIFT	((int) ((sizeof(ks_num_field_t) - 1) * 8))
+
+#define PREV_STATE_MASK		(((ks_num_field_t) 1 << 8) - 1)
+
+#define PID_MASK		(((ks_num_field_t) 1 << PREV_STATE_SHIFT) - 1)
+
+static inline void plugin_sched_set_pid(ks_num_field_t *field,
+				 tep_num_field_t pid)
+{
+	*field &= ~PID_MASK;
+	*field = pid & PID_MASK;
+}
+
+/**
+ * @brief Retrieve the PID value from the data field stored in the
+ *	  kshark_data_container object.
+ *
+ * @param field: Input location for the data field.
+ */
+static inline int plugin_sched_get_pid(ks_num_field_t field)
+{
+	return field & PID_MASK;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c
index 198ed49..3bb9bc2 100644
--- a/src/plugins/sched_events.c
+++ b/src/plugins/sched_events.c
@@ -22,36 +22,6 @@
 
 /** Plugin context instance. */
 
-//! @cond Doxygen_Suppress
-
-typedef unsigned long long tep_num_field_t;
-
-#define PREV_STATE_SHIFT	((int) ((sizeof(ks_num_field_t) - 1) * 8))
-
-#define PREV_STATE_MASK		(((ks_num_field_t) 1 << 8) - 1)
-
-#define PID_MASK		(((ks_num_field_t) 1 << PREV_STATE_SHIFT) - 1)
-
-//! @endcond
-
-static void plugin_sched_set_pid(ks_num_field_t *field,
-				 tep_num_field_t pid)
-{
-	*field &= ~PID_MASK;
-	*field = pid & PID_MASK;
-}
-
-/**
- * @brief Retrieve the PID value from the data field stored in the
- *	  kshark_data_container object.
- *
- * @param field: Input location for the data field.
- */
-__hidden int plugin_sched_get_pid(ks_num_field_t field)
-{
-	return field & PID_MASK;
-}
-
 /* Use the most significant byte to store the value of "prev_state". */
 static void plugin_sched_set_prev_state(ks_num_field_t *field,
 					tep_num_field_t prev_state)
diff --git a/src/plugins/sched_events.h b/src/plugins/sched_events.h
index 2c540fd..1032075 100644
--- a/src/plugins/sched_events.h
+++ b/src/plugins/sched_events.h
@@ -9,12 +9,13 @@
  *  @brief   Plugin for Sched events.
  */
 
-#ifndef _KS_PLUGIN_SHED_H
-#define _KS_PLUGIN_SHED_H
+#ifndef _KS_PLUGIN_SCHED_H
+#define _KS_PLUGIN_SCHED_H
 
 // KernelShark
 #include "libkshark.h"
 #include "libkshark-plugin.h"
+#include "plugins/common_sched.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -55,10 +56,6 @@ struct plugin_sched_context {
 
 KS_DECLARE_PLUGIN_CONTEXT_METHODS(struct plugin_sched_context)
 
-/** The type of the data field stored in the kshark_data_container object. */
-typedef int64_t ks_num_field_t;
-
-int plugin_sched_get_pid(ks_num_field_t field);
 
 int plugin_sched_get_prev_state(ks_num_field_t field);
 
-- 
2.17.1

[PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch

[Hongzhan Chen]

For Xenomai-cobalt enabled system, cobalt_switch_context means
that there is schedule and context switch in companion core(realtime
core), which we may need to do special treatment and take correct
action as main kernel sched_switch to visualize out-of-band state
of realtime tasks running in cobalt core. To achive our target,
we implement following:

  1. store corresponding cobalt_switch_context events into
     container data.
  2. modify pid stored in entry to be equal to next_pid to
     show correct color in cpu bar when cobalt_switch_context
     event happen.
  3. show blue hollow box to mark out-of-band state according to
     cobalt_switch_context events.
  4. clickable cobalt_switch_context plugin shapes.

Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>

diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
index 9740ed9..e933b39 100644
--- a/src/libkshark-tepdata.c
+++ b/src/libkshark-tepdata.c
@@ -1208,6 +1208,7 @@ static void kshark_tep_init_methods(struct kshark_generic_stream_interface *inte
 /** A list of built in default plugins for FTRACE (trace-cmd) data. */
 const char *tep_plugin_names[] = {
 	"sched_events",
+	"xenomai_cobalt_switch_events",
 	"missed_events",
 	"kvm_combo",
 };
diff --git a/src/plugins/CMakeLists.txt b/src/plugins/CMakeLists.txt
index 3e170fa..16f080b 100644
--- a/src/plugins/CMakeLists.txt
+++ b/src/plugins/CMakeLists.txt
@@ -44,6 +44,10 @@ if (Qt5Widgets_FOUND AND TT_FONT_FILE)
                      SOURCE sched_events.c SchedEvents.cpp)
     list(APPEND PLUGIN_LIST "sched_events")
 
+    BUILD_GUI_PLUGIN(NAME xenomai_cobalt_switch_events
+                     SOURCE xenomai_cobalt_switch_events.c CobaltSwitchEvents.cpp)
+    list(APPEND PLUGIN_LIST "xenomai_cobalt_switch_events")
+
     BUILD_GUI_PLUGIN(NAME event_field_plot
                      MOC EventFieldDialog.hpp
                      SOURCE event_field_plot.c EventFieldDialog.cpp EventFieldPlot.cpp)
diff --git a/src/plugins/CobaltSwitchEvents.cpp b/src/plugins/CobaltSwitchEvents.cpp
new file mode 100644
index 0000000..0f90842
--- /dev/null
+++ b/src/plugins/CobaltSwitchEvents.cpp
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: LGPL-2.1
+
+/*
+ * Copyright (C) 2021 Intel Inc, Hongzhan Chen <hongzhan.chen@intel.com>
+ */
+
+/**
+ *  @file    CobaltSwitchEvents.cpp
+ *  @brief   Defines a callback function for Xenomai Cobalt context switch
+ *           events used to plot in blue the out-of-band state of the task
+ */
+
+// KernelShark
+#include "libkshark.h"
+#include "libkshark-plugin.h"
+#include "plugins/xenomai_cobalt_switch_events.h"
+#include "KsPlotTools.hpp"
+#include "KsPlugins.hpp"
+#include "KsMainWindow.hpp"
+
+static KsMainWindow *ks_xenomai_ptr;
+
+/**
+ * @brief Provide the plugin with a pointer to the KsMainWindow object (the GUI
+ * itself) such that the plugin can manipulate the GUI.
+ */
+__hidden void *plugin_set_gui_ptr(void *gui_ptr)
+{
+	ks_xenomai_ptr = static_cast<KsMainWindow *>(gui_ptr);
+	return nullptr;
+}
+
+/**
+ * This class represents the graphical element visualizing OOB state between
+ *  two cobalt_switch_context events.
+ */
+class XenomaiSwitchBox : public LatencyBox
+{
+	/** On double click do. */
+	void _doubleClick() const override
+	{
+		ks_xenomai_ptr->markEntry(_data[1]->entry, DualMarkerState::B);
+		ks_xenomai_ptr->markEntry(_data[0]->entry, DualMarkerState::A);
+	}
+
+};
+
+/*
+ * Ideally, the cobalt_switch_context has to be the last trace event recorded before
+ * the task is preempted. Because of this, when the data is loaded (the first pass),
+ * the "pid" field of the cobalt_switch_context entries gets edited by this plugin
+ * to be equal to the "next pid" of the cobalt_switch_context event. However, in
+ * reality the cobalt_switch_context event may be followed by some trailing events
+ * from the same task (printk events for example). This has the effect of extending
+ * the graph of the task outside of the actual duration of the task. The "second
+ * pass" over the data is used to fix this problem. It takes advantage of the
+ * "next" field of the entry (this field is set during the first pass) to search
+ * for trailing events after the "cobalt_switch_context". In addition, when
+ * we find that it try to switch in-band because next-pid is zero, we prefer to
+ * skip this event because it try to leave oob not enterring.
+ */
+static void secondPass(plugin_cobalt_context *plugin_ctx)
+{
+	kshark_data_container *cSS;
+	kshark_entry *e;
+	int pid_rec, switch_inband;
+
+	cSS = plugin_ctx->cs_data;
+	for (ssize_t i = 0; i < cSS->size; ++i) {
+		switch_inband = plugin_cobalt_check_switch_inband(
+				cSS->data[i]->field);
+
+		/* we skip cobalt_switch_context that try to
+		 * switch into in-band state because we just handle
+		 * out-of-band
+		 */
+		if (switch_inband)
+			continue;
+		pid_rec = plugin_sched_get_pid(cSS->data[i]->field);
+		e = cSS->data[i]->entry;
+		if (!e->next || e->pid == 0 ||
+		    e->event_id == e->next->event_id ||
+		    pid_rec != e->next->pid)
+			continue;
+
+		e = e->next;
+		/* Find all trailing events. */
+		for (; e->next; e = e->next) {
+			if (e->pid == plugin_sched_get_pid(
+						cSS->data[i]->field)) {
+				/*
+				 * Change the "pid" to be equal to the "next
+				 * pid" of the cobalt_switch_context event
+				 * and leave a sign that you edited this
+				 * entry.
+				 */
+				e->pid = cSS->data[i]->entry->pid;
+				e->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
+
+				/*  This is the last trailing event, we finish
+				 *  this round.
+				 */
+				if (e->next->pid != plugin_sched_get_pid(
+						cSS->data[i]->field))
+					break;
+			}
+		}
+	}
+}
+
+/**
+ * @brief Plugin's draw function.
+ *
+ * @param argv_c: A C pointer to be converted to KsCppArgV (C++ struct).
+ * @param sd: Data stream identifier.
+ * @param pid: Process Id.
+ * @param draw_action: Draw action identifier.
+ */
+__hidden void plugin_cobalt_draw(kshark_cpp_argv *argv_c,
+			  int sd, int pid, int draw_action)
+{
+	plugin_cobalt_context *plugin_ctx;
+
+	if (!(draw_action & KSHARK_TASK_DRAW) || pid == 0)
+		return;
+
+	plugin_ctx = __get_context(sd);
+	if (!plugin_ctx)
+		return;
+
+	KsCppArgV *argvCpp = KS_ARGV_TO_CPP(argv_c);
+
+	if (!plugin_ctx->second_pass_done) {
+		/* The second pass is not done yet. */
+		secondPass(plugin_ctx);
+		plugin_ctx->second_pass_done = true;
+	}
+
+	IsApplicableFunc checkFieldCS = [=] (kshark_data_container *d,
+					     ssize_t i) {
+		return !(plugin_cobalt_check_switch_inband(d->data[i]->field)) &&
+			d->data[i]->entry->pid == pid;
+	};
+
+	IsApplicableFunc checkEntryPid = [=] (kshark_data_container *d,
+					      ssize_t i) {
+		return plugin_sched_get_pid(d->data[i]->field) == pid;
+	};
+
+	eventFieldIntervalPlot(argvCpp,
+			       plugin_ctx->cs_data, checkFieldCS,
+			       plugin_ctx->cs_data, checkEntryPid,
+			       makeLatencyBox<XenomaiSwitchBox>,
+			       {0, 0, 255}, // Blue
+			       -1);         // Default size
+}
diff --git a/src/plugins/xenomai_cobalt_switch_events.c b/src/plugins/xenomai_cobalt_switch_events.c
new file mode 100644
index 0000000..f285552
--- /dev/null
+++ b/src/plugins/xenomai_cobalt_switch_events.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: LGPL-2.1
+
+/*
+ * Copyright (C) 2021 Intel Inc, Hongzhan Chen <hongzhan.chen@intel.com>
+ */
+
+/**
+ *  @file    xenomai_cobalt_switch_events.c
+ *  @brief   handle xenomai cobalt switch context event
+ */
+
+// C
+#include <stdlib.h>
+#include <stdio.h>
+
+// trace-cmd
+#include <trace-cmd.h>
+
+// KernelShark
+#include "plugins/xenomai_cobalt_switch_events.h"
+#include "libkshark-tepdata.h"
+
+/** Plugin context instance. */
+
+//! @cond Doxygen_Suppress
+
+#define SWITCH_INBAND_SHIFT	PREV_STATE_SHIFT
+
+#define SWITCH_INBAND_MASK	PREV_STATE_MASK
+
+//! @endcond
+
+static void cobalt_free_context(struct plugin_cobalt_context *plugin_ctx)
+{
+	if (!plugin_ctx)
+		return;
+
+	kshark_free_data_container(plugin_ctx->cs_data);
+}
+
+/* Use the most significant byte to store state marking switch in-band. */
+static void plugin_cobalt_set_switch_inband_state(ks_num_field_t *field,
+					ks_num_field_t inband_state)
+{
+	unsigned long long mask = SWITCH_INBAND_MASK << SWITCH_INBAND_SHIFT;
+	*field &= ~mask;
+	*field |= (inband_state & SWITCH_INBAND_MASK) << SWITCH_INBAND_SHIFT;
+}
+
+/**
+ * @brief Retrieve the state of switch-in-band from the data field stored in
+ *        the kshark_data_container object.
+ *
+ * @param field: Input location for the data field.
+ */
+__hidden int plugin_cobalt_check_switch_inband(ks_num_field_t field)
+{
+	unsigned long long mask = SWITCH_INBAND_MASK << SWITCH_INBAND_SHIFT;
+
+	return (field & mask) >> SWITCH_INBAND_SHIFT;
+}
+
+/** A general purpose macro is used to define plugin context. */
+KS_DEFINE_PLUGIN_CONTEXT(struct plugin_cobalt_context, cobalt_free_context);
+
+static bool plugin_cobalt_init_context(struct kshark_data_stream *stream,
+				      struct plugin_cobalt_context *plugin_ctx)
+{
+	struct tep_event *event;
+
+	if (!kshark_is_tep(stream))
+		return false;
+
+	plugin_ctx->tep = kshark_get_tep(stream);
+
+	event = tep_find_event_by_name(plugin_ctx->tep,
+					"cobalt_core", "cobalt_switch_context");
+	if (!event)
+		return false;
+
+	plugin_ctx->cobalt_switch_event = event;
+	plugin_ctx->cobalt_switch_next_field =
+		tep_find_any_field(event, "next_pid");
+
+	plugin_ctx->second_pass_done = false;
+
+	plugin_ctx->cs_data = kshark_init_data_container();
+	if (!plugin_ctx->cs_data)
+		return false;
+
+	return true;
+}
+
+static void plugin_cobalt_switch_action(struct kshark_data_stream *stream,
+				      void *rec, struct kshark_entry *entry)
+{
+	struct tep_record *record = (struct tep_record *) rec;
+	struct plugin_cobalt_context *plugin_ctx;
+	unsigned long long next_pid;
+	ks_num_field_t ks_field;
+	int ret;
+
+	plugin_ctx = __get_context(stream->stream_id);
+	if (!plugin_ctx)
+		return;
+
+	ret = tep_read_number_field(plugin_ctx->cobalt_switch_next_field,
+				    record->data, &next_pid);
+
+	if (ret == 0 && next_pid >= 0) {
+		plugin_sched_set_pid(&ks_field, entry->pid);
+		if (next_pid == 0) {
+			plugin_cobalt_set_switch_inband_state(&ks_field,
+					SWITCH_INBAND_STATE);
+		}
+
+		kshark_data_container_append(plugin_ctx->cs_data,
+				entry, ks_field);
+
+		if (next_pid > 0)
+			entry->pid = next_pid;
+	}
+}
+
+/** Load this plugin. */
+int KSHARK_PLOT_PLUGIN_INITIALIZER(struct kshark_data_stream *stream)
+{
+	struct plugin_cobalt_context *plugin_ctx;
+
+	plugin_ctx = __init(stream->stream_id);
+	if (!plugin_ctx || !plugin_cobalt_init_context(stream, plugin_ctx)) {
+		__close(stream->stream_id);
+		return 0;
+	}
+
+	if (plugin_ctx->cobalt_switch_event) {
+		kshark_register_event_handler(stream,
+						plugin_ctx->cobalt_switch_event->id,
+						plugin_cobalt_switch_action);
+	}
+
+	kshark_register_draw_handler(stream, plugin_cobalt_draw);
+
+	return 1;
+}
+
+/** Unload this plugin. */
+int KSHARK_PLOT_PLUGIN_DEINITIALIZER(struct kshark_data_stream *stream)
+{
+	struct plugin_cobalt_context *plugin_ctx = __get_context(stream->stream_id);
+	int ret = 0;
+
+	if (plugin_ctx) {
+		if (plugin_ctx->cobalt_switch_event) {
+			kshark_unregister_event_handler(stream,
+							plugin_ctx->cobalt_switch_event->id,
+							plugin_cobalt_switch_action);
+		}
+
+		kshark_unregister_draw_handler(stream, plugin_cobalt_draw);
+
+		ret = 1;
+	}
+
+	__close(stream->stream_id);
+
+	return ret;
+}
+
+/** Initialize the control interface of the plugin. */
+void *KSHARK_MENU_PLUGIN_INITIALIZER(void *gui_ptr)
+{
+	return plugin_set_gui_ptr(gui_ptr);
+}
diff --git a/src/plugins/xenomai_cobalt_switch_events.h b/src/plugins/xenomai_cobalt_switch_events.h
new file mode 100644
index 0000000..4258401
--- /dev/null
+++ b/src/plugins/xenomai_cobalt_switch_events.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: LGPL-2.1 */
+
+/*
+ * Copyright (C) 2021 Intel Inc, Hongzhan Chen<hongzhan.chen@intel.com>
+ */
+
+/**
+ *  @file    xenomai_cobalt_switch_events.h
+ *  @brief   Plugin for xenomai cobalt switch context event
+ */
+
+#ifndef _KS_PLUGIN_SHED_H
+#define _KS_PLUGIN_SHED_H
+
+// KernelShark
+#include "libkshark.h"
+#include "plugins/common_sched.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define SWITCH_INBAND_STATE	1
+
+/** Structure representing a plugin-specific context. */
+struct plugin_cobalt_context {
+	/** Page event used to parse the page. */
+	struct tep_handle	*tep;
+
+	/** Pointer to the cobalt_switch_event object. */
+	struct tep_event	*cobalt_switch_event;
+
+	 /** Pointer to the cobalt_switch_next_field format descriptor. */
+	struct tep_format_field *cobalt_switch_next_field;
+
+	/** True if the second pass is already done. */
+	bool	second_pass_done;
+
+	/** Data container for cobalt_switch_context data. */
+	struct kshark_data_container	*cs_data;
+
+};
+
+KS_DECLARE_PLUGIN_CONTEXT_METHODS(struct plugin_cobalt_context)
+
+int plugin_cobalt_check_switch_inband(ks_num_field_t field);
+
+void plugin_cobalt_draw(struct kshark_cpp_argv *argv, int sd, int pid,
+		 int draw_action);
+
+void *plugin_set_gui_ptr(void *gui_ptr);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
-- 
2.17.1

Re: [PATCH 1/2] kernel-shark: Move common APIs and definitions out to avoid duplication

[Yordan Karadzhov]

Hi Hongzhan,

This patch needs few final touches before going upstream.
See my comments below.

On 7.01.22 г. 4:18 ч., Hongzhan Chen wrote:
> To avoid code duplication, move some common APIs and definitions
> out from plugin SchedEvent to share with other plugins.
> 
> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
> 
> diff --git a/src/KsPlugins.hpp b/src/KsPlugins.hpp
> index d41d094..4b29fa6 100644
> --- a/src/KsPlugins.hpp
> +++ b/src/KsPlugins.hpp
> @@ -13,6 +13,7 @@
>   #define _KS_PLUGINS_H
>   
>   // C++
> +#include <vector>
>   #include <functional>
>   
>   // KernelShark
> @@ -101,4 +102,66 @@ void eventFieldIntervalPlot(KsCppArgV *argvCpp,
>   			    KsPlot::Color col,
>   			    float size);
>   
> +/**
> + * This class represents the graphical element visualizing the latency between
> + * two events such as sched_waking and sched_switch events for common Linux
> + * kernel or cobalt_switch_contexts for Xenomai.
> + */
> +class LatencyBox : public KsPlot::Rectangle
> +{
> +	/** On double click do. */
> +	void _doubleClick() const override {}
> +
> +public:
> +	/** The trace record data that corresponds to this LatencyBox. */
> +	std::vector<kshark_data_field_int64 *>	_data;
> +
> +	/**
> +	 * @brief Distance between the click and the shape. Used to decide if
> +	 *	  the double click action must be executed.
> +	 *
> +	 * @param x: X coordinate of the click.
> +	 * @param y: Y coordinate of the click.
> +	 *
> +	 * @returns If the click is inside the box, the distance is zero.
> +	 *	    Otherwise infinity.
> +	 */
> +	double distance(int x, int y) const override

Move the implementation to src/KsPlugins.cpp

> +	{
> +		if (x < pointX(0) || x > pointX(2))
> +			return std::numeric_limits<double>::max();
> +
> +		if (y < pointY(0) || y > pointY(1))
> +			return std::numeric_limits<double>::max();
> +
> +		return 0;
> +	}
> +};
> +
> +template<class T> KsPlot::PlotObject *
> +makeLatencyBox(std::vector<const KsPlot::Graph *> graph,
> +			     std::vector<int> bins,
> +			     std::vector<kshark_data_field_int64 *> data,
> +			     KsPlot::Color col, float size)
> +{
> +	LatencyBox *rec = new T;
> +	rec->_data = data;
> +
> +	KsPlot::Point p0 = graph[0]->bin(bins[0])._base;
> +	KsPlot::Point p1 = graph[0]->bin(bins[1])._base;
> +	int height = graph[0]->height() * .3;
> +
> +	rec->setFill(false);
> +	rec->setPoint(0, p0.x() - 1, p0.y() - height);
> +	rec->setPoint(1, p0.x() - 1, p0.y() - 1);
> +
> +	rec->setPoint(3, p1.x() - 1, p1.y() - height);
> +	rec->setPoint(2, p1.x() - 1, p1.y() - 1);
> +
> +	rec->_size = size;
> +	rec->_color = col;
> +
> +	return rec;
> +}
> +
>   #endif
> diff --git a/src/plugins/SchedEvents.cpp b/src/plugins/SchedEvents.cpp
> index b73e45f..573e4e3 100644
> --- a/src/plugins/SchedEvents.cpp
> +++ b/src/plugins/SchedEvents.cpp
> @@ -11,9 +11,6 @@
>    *	     preempted by another task.
>    */
>   
> -// C++
> -#include <vector>
> -
>   // KernelShark
>   #include "libkshark.h"
>   #include "libkshark-plugin.h"
> @@ -24,7 +21,7 @@
>   
>   using namespace KsPlot;
>   
> -static KsMainWindow *ks_ptr;
> +static KsMainWindow *ks_schedevent_ptr;
This name is a bit confusing. It reads as pointer to a sched event while it is a pointer to the KS GUI.
What about ks4sched_ptr?


>   
>   /**
>    * @brief Provide the plugin with a pointer to the KsMainWindow object (the GUI
> @@ -32,7 +29,7 @@ static KsMainWindow *ks_ptr;
>    */
>   __hidden void *plugin_set_gui_ptr(void *gui_ptr)
>   {
> -	ks_ptr = static_cast<KsMainWindow *>(gui_ptr);
> +	ks_schedevent_ptr = static_cast<KsMainWindow *>(gui_ptr);
>   	return nullptr;
>   }
>   
> @@ -40,64 +37,15 @@ __hidden void *plugin_set_gui_ptr(void *gui_ptr)
>    * This class represents the graphical element visualizing the latency between
>    *  sched_waking and sched_switch events.

Add here one sentence explaining that this child class is defined to re-implement the handler for double-click.

>    */
> -class LatencyBox : public Rectangle
> +class SchedLatencyBox : public LatencyBox
>   {
>   	/** On double click do. */
>   	void _doubleClick() const override
>   	{
> -		ks_ptr->markEntry(_data[1]->entry, DualMarkerState::B);
> -		ks_ptr->markEntry(_data[0]->entry, DualMarkerState::A);
> +		ks_schedevent_ptr->markEntry(_data[1]->entry, DualMarkerState::B);
> +		ks_schedevent_ptr->markEntry(_data[0]->entry, DualMarkerState::A);
>   	}
>   
> -public:
> -	/** The trace record data that corresponds to this LatencyBox. */
> -	std::vector<kshark_data_field_int64 *>	_data;
> -
> -	/**
> -	 * @brief Distance between the click and the shape. Used to decide if
> -	 *	  the double click action must be executed.
> -	 *
> -	 * @param x: X coordinate of the click.
> -	 * @param y: Y coordinate of the click.
> -	 *
> -	 * @returns If the click is inside the box, the distance is zero.
> -	 *	    Otherwise infinity.
> -	 */
> -	double distance(int x, int y) const override
> -	{
> -		if (x < pointX(0) || x > pointX(2))
> -			return std::numeric_limits<double>::max();
> -
> -		if (y < pointY(0) || y > pointY(1))
> -			return std::numeric_limits<double>::max();
> -
> -		return 0;
> -	}
> -};
> -
> -static PlotObject *makeShape(std::vector<const Graph *> graph,
> -			     std::vector<int> bins,
> -			     std::vector<kshark_data_field_int64 *> data,
> -			     Color col, float size)
> -{
> -	LatencyBox *rec = new LatencyBox;
> -	rec->_data = data;
> -
> -	Point p0 = graph[0]->bin(bins[0])._base;
> -	Point p1 = graph[0]->bin(bins[1])._base;
> -	int height = graph[0]->height() * .3;
> -
> -	rec->setFill(false);
> -	rec->setPoint(0, p0.x() - 1, p0.y() - height);
> -	rec->setPoint(1, p0.x() - 1, p0.y() - 1);
> -
> -	rec->setPoint(3, p1.x() - 1, p1.y() - height);
> -	rec->setPoint(2, p1.x() - 1, p1.y() - 1);
> -
> -	rec->_size = size;
> -	rec->_color = col;
> -
> -	return rec;
>   };
>   
>   /*
> @@ -191,14 +139,14 @@ __hidden void plugin_draw(kshark_cpp_argv *argv_c,
>   	eventFieldIntervalPlot(argvCpp,
>   			       plugin_ctx->sw_data, checkFieldSW,
>   			       plugin_ctx->ss_data, checkEntryPid,
> -			       makeShape,
> +			       makeLatencyBox<SchedLatencyBox>,
>   			       {0, 255, 0}, // Green
>   			       -1);         // Default size
>   
>   	eventFieldIntervalPlot(argvCpp,
>   			       plugin_ctx->ss_data, checkFieldSS,
>   			       plugin_ctx->ss_data, checkEntryPid,
> -			       makeShape,
> +			       makeLatencyBox<SchedLatencyBox>,
>   			       {255, 0, 0}, // Red
>   			       -1);         // Default size
>   }
> diff --git a/src/plugins/common_sched.h b/src/plugins/common_sched.h
> new file mode 100644
> index 0000000..1d564c0
> --- /dev/null
> +++ b/src/plugins/common_sched.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: LGPL-2.1 */
> +
> +/*
> + * Copyright (C) 2021 Intel Inc, Hongzhan Chen <hongzhan.chen@intel.com>

Since this is a 'copy-paste' code I think you should keep the original copyright statement on top and add the new one 
(2021, Intel, Hongzhan) on the next line.

> + */
> +
> +/**
> + *  @file    common_sched.h
> + *  @brief   Plugin for common sched.
Perhaps you mean 'Common definitions for sched plugins?'

> + */
> +
> +#ifndef _KS_PLUGIN_COMMON_SCHED_H
> +#define _KS_PLUGIN_COMMON_SCHED_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +typedef unsigned long long tep_num_field_t;
> +
> +/** The type of the data field stored in the kshark_data_container object. */
> +typedef int64_t ks_num_field_t;
> +
> +#define PREV_STATE_SHIFT	((int) ((sizeof(ks_num_field_t) - 1) * 8))
> +
> +#define PREV_STATE_MASK		(((ks_num_field_t) 1 << 8) - 1)
> +
> +#define PID_MASK		(((ks_num_field_t) 1 << PREV_STATE_SHIFT) - 1)
> +
> +static inline void plugin_sched_set_pid(ks_num_field_t *field,
> +				 tep_num_field_t pid)
> +{
> +	*field &= ~PID_MASK;
> +	*field = pid & PID_MASK;

Hmm, this looks like a bug in the schedevent plugin. This line should be
	*field |= pid & PID_MASK;

> +}
> +
> +/**
> + * @brief Retrieve the PID value from the data field stored in the
> + *	  kshark_data_container object.
> + *
> + * @param field: Input location for the data field.
> + */
> +static inline int plugin_sched_get_pid(ks_num_field_t field)
> +{
> +	return field & PID_MASK;
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c
> index 198ed49..3bb9bc2 100644
> --- a/src/plugins/sched_events.c
> +++ b/src/plugins/sched_events.c
> @@ -22,36 +22,6 @@
>   
>   /** Plugin context instance. */
>   
> -//! @cond Doxygen_Suppress
> -
> -typedef unsigned long long tep_num_field_t;
> -
> -#define PREV_STATE_SHIFT	((int) ((sizeof(ks_num_field_t) - 1) * 8))
> -
> -#define PREV_STATE_MASK		(((ks_num_field_t) 1 << 8) - 1)
> -
> -#define PID_MASK		(((ks_num_field_t) 1 << PREV_STATE_SHIFT) - 1)
> -
> -//! @endcond
> -
> -static void plugin_sched_set_pid(ks_num_field_t *field,
> -				 tep_num_field_t pid)
> -{
> -	*field &= ~PID_MASK;
> -	*field = pid & PID_MASK;
> -}
> -
> -/**
> - * @brief Retrieve the PID value from the data field stored in the
> - *	  kshark_data_container object.
> - *
> - * @param field: Input location for the data field.
> - */
> -__hidden int plugin_sched_get_pid(ks_num_field_t field)
> -{
> -	return field & PID_MASK;
> -}
> -
>   /* Use the most significant byte to store the value of "prev_state". */
>   static void plugin_sched_set_prev_state(ks_num_field_t *field,
>   					tep_num_field_t prev_state)
> diff --git a/src/plugins/sched_events.h b/src/plugins/sched_events.h
> index 2c540fd..1032075 100644
> --- a/src/plugins/sched_events.h
> +++ b/src/plugins/sched_events.h
> @@ -9,12 +9,13 @@
>    *  @brief   Plugin for Sched events.
>    */
>   
> -#ifndef _KS_PLUGIN_SHED_H
> -#define _KS_PLUGIN_SHED_H
> +#ifndef _KS_PLUGIN_SCHED_H
> +#define _KS_PLUGIN_SCHED_H


Good catch, please make this a separate patch.

Thanks!
Yordan

>   
>   // KernelShark
>   #include "libkshark.h"
>   #include "libkshark-plugin.h"
> +#include "plugins/common_sched.h"
>   
>   #ifdef __cplusplus
>   extern "C" {
> @@ -55,10 +56,6 @@ struct plugin_sched_context {
>   
>   KS_DECLARE_PLUGIN_CONTEXT_METHODS(struct plugin_sched_context)
>   
> -/** The type of the data field stored in the kshark_data_container object. */
> -typedef int64_t ks_num_field_t;
> -
> -int plugin_sched_get_pid(ks_num_field_t field);
>   
>   int plugin_sched_get_prev_state(ks_num_field_t field);
>   
> 

Re: [PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch

[Yordan Karadzhov]

Hi Hongzhan and Jan,

Have you decided something about the way this plugin will be maintained?


On 7.01.22 г. 4:18 ч., Hongzhan Chen wrote:
> For Xenomai-cobalt enabled system, cobalt_switch_context means
> that there is schedule and context switch in companion core(realtime
> core), which we may need to do special treatment and take correct
> action as main kernel sched_switch to visualize out-of-band state
> of realtime tasks running in cobalt core. To achive our target,
> we implement following:
> 
>    1. store corresponding cobalt_switch_context events into
>       container data.
>    2. modify pid stored in entry to be equal to next_pid to
>       show correct color in cpu bar when cobalt_switch_context
>       event happen.
>    3. show blue hollow box to mark out-of-band state according to
>       cobalt_switch_context events.
>    4. clickable cobalt_switch_context plugin shapes.
> 
> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
> 
> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
> index 9740ed9..e933b39 100644
> --- a/src/libkshark-tepdata.c
> +++ b/src/libkshark-tepdata.c
> @@ -1208,6 +1208,7 @@ static void kshark_tep_init_methods(struct kshark_generic_stream_interface *inte
>   /** A list of built in default plugins for FTRACE (trace-cmd) data. */
>   const char *tep_plugin_names[] = {
>   	"sched_events",
> +	"xenomai_cobalt_switch_events",

I am not sure we want to make this plugin load by default. Even if we do so, this change must go with a separate patch.

>   	"missed_events",
>   	"kvm_combo",
>   };
> diff --git a/src/plugins/CMakeLists.txt b/src/plugins/CMakeLists.txt
> index 3e170fa..16f080b 100644
> --- a/src/plugins/CMakeLists.txt
> +++ b/src/plugins/CMakeLists.txt
> @@ -44,6 +44,10 @@ if (Qt5Widgets_FOUND AND TT_FONT_FILE)
>                        SOURCE sched_events.c SchedEvents.cpp)
>       list(APPEND PLUGIN_LIST "sched_events")
>   
> +    BUILD_GUI_PLUGIN(NAME xenomai_cobalt_switch_events
> +                     SOURCE xenomai_cobalt_switch_events.c CobaltSwitchEvents.cpp)
> +    list(APPEND PLUGIN_LIST "xenomai_cobalt_switch_events")
> +

You do not need this if the plugin will live in its own repository.

>       BUILD_GUI_PLUGIN(NAME event_field_plot
>                        MOC EventFieldDialog.hpp
>                        SOURCE event_field_plot.c EventFieldDialog.cpp EventFieldPlot.cpp)
> diff --git a/src/plugins/CobaltSwitchEvents.cpp b/src/plugins/CobaltSwitchEvents.cpp
> new file mode 100644
> index 0000000..0f90842
> --- /dev/null
> +++ b/src/plugins/CobaltSwitchEvents.cpp
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +
> +/*
> + * Copyright (C) 2021 Intel Inc, Hongzhan Chen <hongzhan.chen@intel.com>
> + */
> +
> +/**
> + *  @file    CobaltSwitchEvents.cpp
> + *  @brief   Defines a callback function for Xenomai Cobalt context switch
> + *           events used to plot in blue the out-of-band state of the task
> + */
> +
> +// KernelShark
> +#include "libkshark.h"
> +#include "libkshark-plugin.h"
> +#include "plugins/xenomai_cobalt_switch_events.h"
> +#include "KsPlotTools.hpp"
> +#include "KsPlugins.hpp"
> +#include "KsMainWindow.hpp"
> +
> +static KsMainWindow *ks_xenomai_ptr;
> +
> +/**
> + * @brief Provide the plugin with a pointer to the KsMainWindow object (the GUI
> + * itself) such that the plugin can manipulate the GUI.
> + */
> +__hidden void *plugin_set_gui_ptr(void *gui_ptr)
> +{
> +	ks_xenomai_ptr = static_cast<KsMainWindow *>(gui_ptr);
> +	return nullptr;
> +}
> +
> +/**
> + * This class represents the graphical element visualizing OOB state between
> + *  two cobalt_switch_context events.
> + */
> +class XenomaiSwitchBox : public LatencyBox
> +{
> +	/** On double click do. */
> +	void _doubleClick() const override
> +	{
> +		ks_xenomai_ptr->markEntry(_data[1]->entry, DualMarkerState::B);
> +		ks_xenomai_ptr->markEntry(_data[0]->entry, DualMarkerState::A);
> +	}
> +
> +};
> +
> +/*
> + * Ideally, the cobalt_switch_context has to be the last trace event recorded before
> + * the task is preempted. Because of this, when the data is loaded (the first pass),
> + * the "pid" field of the cobalt_switch_context entries gets edited by this plugin
> + * to be equal to the "next pid" of the cobalt_switch_context event. However, in
> + * reality the cobalt_switch_context event may be followed by some trailing events
> + * from the same task (printk events for example). This has the effect of extending
> + * the graph of the task outside of the actual duration of the task. The "second
> + * pass" over the data is used to fix this problem. It takes advantage of the
> + * "next" field of the entry (this field is set during the first pass) to search
> + * for trailing events after the "cobalt_switch_context". In addition, when
> + * we find that it try to switch in-band because next-pid is zero, we prefer to
> + * skip this event because it try to leave oob not enterring.
> + */
> +static void secondPass(plugin_cobalt_context *plugin_ctx)
> +{
> +	kshark_data_container *cSS;
> +	kshark_entry *e;
> +	int pid_rec, switch_inband;
> +
> +	cSS = plugin_ctx->cs_data;
> +	for (ssize_t i = 0; i < cSS->size; ++i) {
> +		switch_inband = plugin_cobalt_check_switch_inband(
> +				cSS->data[i]->field);
> +
> +		/* we skip cobalt_switch_context that try to
> +		 * switch into in-band state because we just handle
> +		 * out-of-band
> +		 */
> +		if (switch_inband)
> +			continue;
> +		pid_rec = plugin_sched_get_pid(cSS->data[i]->field);
> +		e = cSS->data[i]->entry;
> +		if (!e->next || e->pid == 0 ||
> +		    e->event_id == e->next->event_id ||
> +		    pid_rec != e->next->pid)
> +			continue;
> +
> +		e = e->next;
> +		/* Find all trailing events. */
> +		for (; e->next; e = e->next) {
> +			if (e->pid == plugin_sched_get_pid(
> +						cSS->data[i]->field)) {
> +				/*
> +				 * Change the "pid" to be equal to the "next
> +				 * pid" of the cobalt_switch_context event
> +				 * and leave a sign that you edited this
> +				 * entry.
> +				 */
> +				e->pid = cSS->data[i]->entry->pid;
> +				e->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> +
> +				/*  This is the last trailing event, we finish
> +				 *  this round.
> +				 */
> +				if (e->next->pid != plugin_sched_get_pid(
> +						cSS->data[i]->field))
> +					break;
> +			}
> +		}
> +	}
> +}
> +
> +/**
> + * @brief Plugin's draw function.
> + *
> + * @param argv_c: A C pointer to be converted to KsCppArgV (C++ struct).
> + * @param sd: Data stream identifier.
> + * @param pid: Process Id.
> + * @param draw_action: Draw action identifier.
> + */
> +__hidden void plugin_cobalt_draw(kshark_cpp_argv *argv_c,
> +			  int sd, int pid, int draw_action)
> +{
> +	plugin_cobalt_context *plugin_ctx;
> +
> +	if (!(draw_action & KSHARK_TASK_DRAW) || pid == 0)
> +		return;
> +
> +	plugin_ctx = __get_context(sd);
> +	if (!plugin_ctx)
> +		return;
> +
> +	KsCppArgV *argvCpp = KS_ARGV_TO_CPP(argv_c);
> +
> +	if (!plugin_ctx->second_pass_done) {
> +		/* The second pass is not done yet. */
> +		secondPass(plugin_ctx);
> +		plugin_ctx->second_pass_done = true;
> +	}
> +
> +	IsApplicableFunc checkFieldCS = [=] (kshark_data_container *d,
> +					     ssize_t i) {
> +		return !(plugin_cobalt_check_switch_inband(d->data[i]->field)) &&
> +			d->data[i]->entry->pid == pid;
> +	};
> +
> +	IsApplicableFunc checkEntryPid = [=] (kshark_data_container *d,
> +					      ssize_t i) {
> +		return plugin_sched_get_pid(d->data[i]->field) == pid;
> +	};
> +
> +	eventFieldIntervalPlot(argvCpp,
> +			       plugin_ctx->cs_data, checkFieldCS,
> +			       plugin_ctx->cs_data, checkEntryPid,
> +			       makeLatencyBox<XenomaiSwitchBox>,
> +			       {0, 0, 255}, // Blue

Perhaps you can use 'cobalt blue' instead of just blue ;-)

cheers,
Yordan

> +			       -1);         // Default size
> +}
> diff --git a/src/plugins/xenomai_cobalt_switch_events.c b/src/plugins/xenomai_cobalt_switch_events.c
> new file mode 100644
> index 0000000..f285552
> --- /dev/null
> +++ b/src/plugins/xenomai_cobalt_switch_events.c
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +
> +/*
> + * Copyright (C) 2021 Intel Inc, Hongzhan Chen <hongzhan.chen@intel.com>
> + */
> +
> +/**
> + *  @file    xenomai_cobalt_switch_events.c
> + *  @brief   handle xenomai cobalt switch context event
> + */
> +
> +// C
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +// trace-cmd
> +#include <trace-cmd.h>
> +
> +// KernelShark
> +#include "plugins/xenomai_cobalt_switch_events.h"
> +#include "libkshark-tepdata.h"
> +
> +/** Plugin context instance. */
> +
> +//! @cond Doxygen_Suppress
> +
> +#define SWITCH_INBAND_SHIFT	PREV_STATE_SHIFT
> +
> +#define SWITCH_INBAND_MASK	PREV_STATE_MASK
> +
> +//! @endcond
> +
> +static void cobalt_free_context(struct plugin_cobalt_context *plugin_ctx)
> +{
> +	if (!plugin_ctx)
> +		return;
> +
> +	kshark_free_data_container(plugin_ctx->cs_data);
> +}
> +
> +/* Use the most significant byte to store state marking switch in-band. */
> +static void plugin_cobalt_set_switch_inband_state(ks_num_field_t *field,
> +					ks_num_field_t inband_state)
> +{
> +	unsigned long long mask = SWITCH_INBAND_MASK << SWITCH_INBAND_SHIFT;
> +	*field &= ~mask;
> +	*field |= (inband_state & SWITCH_INBAND_MASK) << SWITCH_INBAND_SHIFT;
> +}
> +
> +/**
> + * @brief Retrieve the state of switch-in-band from the data field stored in
> + *        the kshark_data_container object.
> + *
> + * @param field: Input location for the data field.
> + */
> +__hidden int plugin_cobalt_check_switch_inband(ks_num_field_t field)
> +{
> +	unsigned long long mask = SWITCH_INBAND_MASK << SWITCH_INBAND_SHIFT;
> +
> +	return (field & mask) >> SWITCH_INBAND_SHIFT;
> +}
> +
> +/** A general purpose macro is used to define plugin context. */
> +KS_DEFINE_PLUGIN_CONTEXT(struct plugin_cobalt_context, cobalt_free_context);
> +
> +static bool plugin_cobalt_init_context(struct kshark_data_stream *stream,
> +				      struct plugin_cobalt_context *plugin_ctx)
> +{
> +	struct tep_event *event;
> +
> +	if (!kshark_is_tep(stream))
> +		return false;
> +
> +	plugin_ctx->tep = kshark_get_tep(stream);
> +
> +	event = tep_find_event_by_name(plugin_ctx->tep,
> +					"cobalt_core", "cobalt_switch_context");
> +	if (!event)
> +		return false;
> +
> +	plugin_ctx->cobalt_switch_event = event;
> +	plugin_ctx->cobalt_switch_next_field =
> +		tep_find_any_field(event, "next_pid");
> +
> +	plugin_ctx->second_pass_done = false;
> +
> +	plugin_ctx->cs_data = kshark_init_data_container();
> +	if (!plugin_ctx->cs_data)
> +		return false;
> +
> +	return true;
> +}
> +
> +static void plugin_cobalt_switch_action(struct kshark_data_stream *stream,
> +				      void *rec, struct kshark_entry *entry)
> +{
> +	struct tep_record *record = (struct tep_record *) rec;
> +	struct plugin_cobalt_context *plugin_ctx;
> +	unsigned long long next_pid;
> +	ks_num_field_t ks_field;
> +	int ret;
> +
> +	plugin_ctx = __get_context(stream->stream_id);
> +	if (!plugin_ctx)
> +		return;
> +
> +	ret = tep_read_number_field(plugin_ctx->cobalt_switch_next_field,
> +				    record->data, &next_pid);
> +
> +	if (ret == 0 && next_pid >= 0) {
> +		plugin_sched_set_pid(&ks_field, entry->pid);
> +		if (next_pid == 0) {
> +			plugin_cobalt_set_switch_inband_state(&ks_field,
> +					SWITCH_INBAND_STATE);
> +		}
> +
> +		kshark_data_container_append(plugin_ctx->cs_data,
> +				entry, ks_field);
> +
> +		if (next_pid > 0)
> +			entry->pid = next_pid;
> +	}
> +}
> +
> +/** Load this plugin. */
> +int KSHARK_PLOT_PLUGIN_INITIALIZER(struct kshark_data_stream *stream)
> +{
> +	struct plugin_cobalt_context *plugin_ctx;
> +
> +	plugin_ctx = __init(stream->stream_id);
> +	if (!plugin_ctx || !plugin_cobalt_init_context(stream, plugin_ctx)) {
> +		__close(stream->stream_id);
> +		return 0;
> +	}
> +
> +	if (plugin_ctx->cobalt_switch_event) {
> +		kshark_register_event_handler(stream,
> +						plugin_ctx->cobalt_switch_event->id,
> +						plugin_cobalt_switch_action);
> +	}
> +
> +	kshark_register_draw_handler(stream, plugin_cobalt_draw);
> +
> +	return 1;
> +}
> +
> +/** Unload this plugin. */
> +int KSHARK_PLOT_PLUGIN_DEINITIALIZER(struct kshark_data_stream *stream)
> +{
> +	struct plugin_cobalt_context *plugin_ctx = __get_context(stream->stream_id);
> +	int ret = 0;
> +
> +	if (plugin_ctx) {
> +		if (plugin_ctx->cobalt_switch_event) {
> +			kshark_unregister_event_handler(stream,
> +							plugin_ctx->cobalt_switch_event->id,
> +							plugin_cobalt_switch_action);
> +		}
> +
> +		kshark_unregister_draw_handler(stream, plugin_cobalt_draw);
> +
> +		ret = 1;
> +	}
> +
> +	__close(stream->stream_id);
> +
> +	return ret;
> +}
> +
> +/** Initialize the control interface of the plugin. */
> +void *KSHARK_MENU_PLUGIN_INITIALIZER(void *gui_ptr)
> +{
> +	return plugin_set_gui_ptr(gui_ptr);
> +}
> diff --git a/src/plugins/xenomai_cobalt_switch_events.h b/src/plugins/xenomai_cobalt_switch_events.h
> new file mode 100644
> index 0000000..4258401
> --- /dev/null
> +++ b/src/plugins/xenomai_cobalt_switch_events.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: LGPL-2.1 */
> +
> +/*
> + * Copyright (C) 2021 Intel Inc, Hongzhan Chen<hongzhan.chen@intel.com>
> + */
> +
> +/**
> + *  @file    xenomai_cobalt_switch_events.h
> + *  @brief   Plugin for xenomai cobalt switch context event
> + */
> +
> +#ifndef _KS_PLUGIN_SHED_H
> +#define _KS_PLUGIN_SHED_H
> +
> +// KernelShark
> +#include "libkshark.h"
> +#include "plugins/common_sched.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define SWITCH_INBAND_STATE	1
> +
> +/** Structure representing a plugin-specific context. */
> +struct plugin_cobalt_context {
> +	/** Page event used to parse the page. */
> +	struct tep_handle	*tep;
> +
> +	/** Pointer to the cobalt_switch_event object. */
> +	struct tep_event	*cobalt_switch_event;
> +
> +	 /** Pointer to the cobalt_switch_next_field format descriptor. */
> +	struct tep_format_field *cobalt_switch_next_field;
> +
> +	/** True if the second pass is already done. */
> +	bool	second_pass_done;
> +
> +	/** Data container for cobalt_switch_context data. */
> +	struct kshark_data_container	*cs_data;
> +
> +};
> +
> +KS_DECLARE_PLUGIN_CONTEXT_METHODS(struct plugin_cobalt_context)
> +
> +int plugin_cobalt_check_switch_inband(ks_num_field_t field);
> +
> +void plugin_cobalt_draw(struct kshark_cpp_argv *argv, int sd, int pid,
> +		 int draw_action);
> +
> +void *plugin_set_gui_ptr(void *gui_ptr);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> 

Re: [PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch

[Jan Kiszka]

On 07.01.22 12:10, Yordan Karadzhov wrote:
> Hi Hongzhan and Jan,
> 
> Have you decided something about the way this plugin will be maintained?
> 

No decision yet. I'm still waiting for an answer on
https://lore.kernel.org/linux-trace-devel/9ac9a1af-6829-425a-7943-755decf7c273@gmail.com/
regarding maintenance of different tracepoint revisions. That would be
implicit when keeping the plugin in lock-step with the tracepoints in
the same repo.

But then the question would be how mature the interface lifecycle
management of kernelshark and libs is /wrt external plugins. Those may
need to account for potential changes in those APIs over the time, right?

We will likely pick the path that is least inconvenient.

[...]

>> +    eventFieldIntervalPlot(argvCpp,
>> +                   plugin_ctx->cs_data, checkFieldCS,
>> +                   plugin_ctx->cs_data, checkEntryPid,
>> +                   makeLatencyBox<XenomaiSwitchBox>,
>> +                   {0, 0, 255}, // Blue
> 
> Perhaps you can use 'cobalt blue' instead of just blue ;-)
> 

Makes sense ;)

Seriously: Is there some caption somewhere that explains the meaning of
colors? If so, how to hook into that?

Thanks,
Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch

[Yordan Karadzhov]

On 7.01.22 г. 13:57 ч., Jan Kiszka wrote:
> On 07.01.22 12:10, Yordan Karadzhov wrote:
>> Hi Hongzhan and Jan,
>>
>> Have you decided something about the way this plugin will be maintained?
>>
> 
> No decision yet. I'm still waiting for an answer on
> https://lore.kernel.org/linux-trace-devel/9ac9a1af-6829-425a-7943-755decf7c273@gmail.com/
> regarding maintenance of different tracepoint revisions. That would be
> implicit when keeping the plugin in lock-step with the tracepoints in
> the same repo.

OK.

> 
> But then the question would be how mature the interface lifecycle
> management of kernelshark and libs is /wrt external plugins. Those may
> need to account for potential changes in those APIs over the time, right?
> 

The APIs are not supposed to change. I cannot give you an absolute guarantee, but we will do our best to keep the 
interface stable. Anyway, even if we have to do some minor changes in the future, it will be our responsibility to 
communicate this and help with fixing all external plugins we are aware of.

The whole idea behind the design of this interface is to be friendly for external plugins.

> We will likely pick the path that is least inconvenient.
> 
> [...]
> 
>>> +    eventFieldIntervalPlot(argvCpp,
>>> +                   plugin_ctx->cs_data, checkFieldCS,
>>> +                   plugin_ctx->cs_data, checkEntryPid,
>>> +                   makeLatencyBox<XenomaiSwitchBox>,
>>> +                   {0, 0, 255}, // Blue
>>
>> Perhaps you can use 'cobalt blue' instead of just blue ;-)
>>
> 
> Makes sense ;)
> 
> Seriously: Is there some caption somewhere that explains the meaning of
> colors? If so, how to hook into that?

So far we do not have special convention for the meaning of the colors.

Thanks,
Yordan

> 
> Thanks,
> Jan
> 

Re: [PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch

[Jan Kiszka]

On 07.01.22 13:24, Yordan Karadzhov wrote:
> 
> 
> On 7.01.22 г. 13:57 ч., Jan Kiszka wrote:
>> On 07.01.22 12:10, Yordan Karadzhov wrote:
>>> Hi Hongzhan and Jan,
>>>
>>> Have you decided something about the way this plugin will be maintained?
>>>
>>
>> No decision yet. I'm still waiting for an answer on
>> https://lore.kernel.org/linux-trace-devel/9ac9a1af-6829-425a-7943-755decf7c273@gmail.com/
>>
>> regarding maintenance of different tracepoint revisions. That would be
>> implicit when keeping the plugin in lock-step with the tracepoints in
>> the same repo.
> 
> OK.
> 
>>
>> But then the question would be how mature the interface lifecycle
>> management of kernelshark and libs is /wrt external plugins. Those may
>> need to account for potential changes in those APIs over the time, right?
>>
> 
> The APIs are not supposed to change. I cannot give you an absolute
> guarantee, but we will do our best to keep the interface stable. Anyway,
> even if we have to do some minor changes in the future, it will be our
> responsibility to communicate this and help with fixing all external
> plugins we are aware of.
> 
> The whole idea behind the design of this interface is to be friendly for
> external plugins.
> 

OK, that sound motivating to look into the external path. I suppose only
the auto-loading topic should be improved then.

>> We will likely pick the path that is least inconvenient.
>>
>> [...]
>>
>>>> +    eventFieldIntervalPlot(argvCpp,
>>>> +                   plugin_ctx->cs_data, checkFieldCS,
>>>> +                   plugin_ctx->cs_data, checkEntryPid,
>>>> +                   makeLatencyBox<XenomaiSwitchBox>,
>>>> +                   {0, 0, 255}, // Blue
>>>
>>> Perhaps you can use 'cobalt blue' instead of just blue ;-)
>>>
>>
>> Makes sense ;)
>>
>> Seriously: Is there some caption somewhere that explains the meaning of
>> colors? If so, how to hook into that?
> 
> So far we do not have special convention for the meaning of the colors.
> 

But also no textual description of their semantics yet, right? Where
could this be added best to the UI?

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch

[Yordan Karadzhov]

On 7.01.22 г. 14:27 ч., Jan Kiszka wrote:
>> The whole idea behind the design of this interface is to be friendly for
>> external plugins.
>>
> OK, that sound motivating to look into the external path. I suppose only
> the auto-loading topic should be improved then.
> 

Sounds like a plan.
Y.

RE: [PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch

[Chen, Hongzhan]

>-----Original Message-----
>From: Yordan Karadzhov <y.karadz@gmail.com> 
>Sent: Friday, January 7, 2022 8:31 PM
>To: Kiszka, Jan <jan.kiszka@siemens.com>; Chen, Hongzhan <hongzhan.chen@intel.com>; linux-trace-devel@vger.kernel.org
>Subject: Re: [PATCH 2/2] kernel-shark: Add plugin for handling Xenomai cobalt_context_switch
>
>
>
>On 7.01.22 ?. 14:27 ?., Jan Kiszka wrote:
>>> The whole idea behind the design of this interface is to be friendly for
>>> external plugins.
>>>
>> OK, that sound motivating to look into the external path. I suppose only
>> the auto-loading topic should be improved then.
>> 
>
>Sounds like a plan.

Thanks for all your comments to make the thing clear for me. Please review my V4 patchset.

Regards

Hongzhan Chen

>Y.
>