diff mbox series

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

Message ID 20220107021846.893-1-hongzhan.chen@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series [1/2] kernel-shark: Move common APIs and definitions out to avoid duplication | expand

Commit Message

Hongzhan Chen Jan. 7, 2022, 2:18 a.m. UTC 
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>

Comments

Yordan Karadzhov Jan. 7, 2022, 10:39 a.m. UTC | #1 
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);
>   
>
diff mbox series

Patch

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);