diff mbox series

[2/8] kernel-shark-qt: Improve the plotting logic of the Sched event plugin

Message ID 20181107161410.22507-3-ykaradzhov@vmware.com (mailing list archive)
State Superseded
Headers show
Series New/improved KernelShark plugins | expand

Commit Message

Yordan Karadzhov Nov. 7, 2018, 4:14 p.m. UTC
This patch aims to make the plotting logic of the Sched event plugin
more robust and easy to understand. It also provides a proper
processing of the case when we have multiple Sched events in one bin.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/plugins/SchedEvents.cpp | 154 +++++++++-----------
 kernel-shark-qt/src/plugins/sched_events.c  |  50 +++++--
 kernel-shark-qt/src/plugins/sched_events.h  |  12 +-
 3 files changed, 116 insertions(+), 100 deletions(-)
diff mbox series

Patch

diff --git a/kernel-shark-qt/src/plugins/SchedEvents.cpp b/kernel-shark-qt/src/plugins/SchedEvents.cpp
index 5f833df..1e872aa 100644
--- a/kernel-shark-qt/src/plugins/SchedEvents.cpp
+++ b/kernel-shark-qt/src/plugins/SchedEvents.cpp
@@ -54,9 +54,11 @@  static void pluginDraw(plugin_sched_context *plugin_ctx,
 		       KsPlot::Graph *graph,
 		       KsPlot::PlotObjList *shapes)
 {
+	const kshark_entry *entryClose, *entryOpen;
 	std::function<void(int)> ifSchedBack;
 	KsPlot::Rectangle *rec = nullptr;
 	int height = graph->getHeight() * .3;
+	ssize_t indexClose(0), indexOpen(0);
 
 	auto openBox = [&] (const KsPlot::Point &p)
 	{
@@ -67,7 +69,13 @@  static void pluginDraw(plugin_sched_context *plugin_ctx,
 		if (!rec)
 			rec = new KsPlot::Rectangle;
 
+		if (e == SchedEvent::Switch)
+			rec->_color = KsPlot::Color(255, 0, 0);
+		else
+			rec->_color = KsPlot::Color(0, 255, 0);
+
 		rec->setFill(false);
+
 		rec->setPoint(0, p.x() - 1, p.y() - height);
 		rec->setPoint(1, p.x() - 1, p.y() - 1);
 	};
@@ -77,7 +85,7 @@  static void pluginDraw(plugin_sched_context *plugin_ctx,
 		if (rec == nullptr)
 			return;
 
-		int boxSize = rec->getPoint(0)->x;
+		int boxSize = p.x() - rec->getPoint(0)->x;
 		if (boxSize < PLUGIN_MIN_BOX_SIZE) {
 			/* This box is too small. Don't try to plot it. */
 			delete rec;
@@ -92,99 +100,78 @@  static void pluginDraw(plugin_sched_context *plugin_ctx,
 		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.
+	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.
 		 */
-		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);
+		if (ksmodel_bin_count(histo, bin) > PLUGIN_MAX_ENTRIES_PER_BIN) {
+			if (rec) {
+				delete rec;
+				rec = nullptr;
+			}
+
+			continue;
 		}
-	};
 
-	auto lamIfSchWakeupBack = [&] (int bin)
-	{
 		/*
-		 * Starting from the last element in this bin, go backward
+		 * Starting from the first element in this bin, go forward
 		 * in time until you find a trace entry that satisfies the
-		 * condition defined by plugin_wakeup_match_pid.
+		 * condition defined by kshark_match_pid.
 		 */
-		const kshark_entry *entryB =
-			ksmodel_get_entry_back(histo, bin, false,
-					       plugin_wakeup_match_pid, pid,
-					       col, nullptr);
+		entryClose = ksmodel_get_entry_back(histo, bin, false,
+						 plugin_switch_match_entry_pid,
+						 pid, col, &indexClose);
 
-		if (entryB) {
+		if (e == SchedEvent::Switch) {
 			/*
-			 * entryB is a sched_wakeup_event. Open a
-			 * green box here.
+			 * 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.
 			 */
-			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);
+			entryOpen =
+				ksmodel_get_entry_back(histo, bin, false,
+						       plugin_switch_match_rec_pid,
+						       pid, col, &indexOpen);
 
-		if (entryB && entryB->pid != pid) {
+		} else {
 			/*
-			 * entryB is a sched_switch_event. Open a
-			 * red box here.
+			 * 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.
 			 */
-			openBox(graph->getBin(bin)._base);
-
-			/* Red */
-			rec->_color = KsPlot::Color(255, 0, 0);
+			entryOpen =
+				ksmodel_get_entry_back(histo, bin, false,
+						       plugin_wakeup_match_rec_pid,
+						       pid, col, &indexOpen);
 		}
-	};
-
-	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) {
+			if (entryClose) {
+				/* Close the box in this bin. */
+				closeBox(graph->getBin(bin)._base);
+				if (entryOpen && indexClose < indexOpen) {
+					/*
+					 * We have a Sched switch entry that
+					 * comes after (in time) the closure of
+					 * the previous box. We have to open a
+					 * new box in this bin.
+					 */
+					openBox(graph->getBin(bin)._base);
+				}
+			}
+		} else {
+			if (entryOpen &&
+			    (!entryClose || indexClose < indexOpen)) {
+				/* Open a new box in this bin. */
+				openBox(graph->getBin(bin)._base);
+			}
+		}
 	}
 
 	if (rec)
@@ -221,7 +208,8 @@  static void secondPass(kshark_entry **data,
 
 		kshark_entry_request *req =
 			kshark_entry_request_alloc(first, n,
-						   plugin_switch_match_pid, pid,
+						   plugin_switch_match_rec_pid,
+						   pid,
 						   false,
 						   KS_GRAPH_VIEW_FILTER_MASK);
 
@@ -303,12 +291,12 @@  void plugin_draw(kshark_cpp_argv *argv_c, int pid, int draw_action)
 	try {
 		pluginDraw(plugin_ctx, kshark_ctx,
 			   argvCpp->_histo, col,
-			   SchedEvent::Switch, pid,
+			   SchedEvent::Wakeup, pid,
 			   argvCpp->_graph, argvCpp->_shapes);
 
 		pluginDraw(plugin_ctx, kshark_ctx,
 			   argvCpp->_histo, col,
-			   SchedEvent::Wakeup, pid,
+			   SchedEvent::Switch, 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
index f23c916..6045341 100644
--- a/kernel-shark-qt/src/plugins/sched_events.c
+++ b/kernel-shark-qt/src/plugins/sched_events.c
@@ -105,7 +105,7 @@  int plugin_get_next_pid(struct tep_record *record)
  *
  * @param record: Input location for a sched_wakeup record.
  */
-int plugin_get_wakeup_pid(struct tep_record *record)
+int plugin_get_rec_wakeup_pid(struct tep_record *record)
 {
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
@@ -137,7 +137,7 @@  static void plugin_register_command(struct kshark_context *kshark_ctx,
 			tep_register_comm(kshark_ctx->pevent, comm, pid);
 }
 
-static int plugin_get_wakeup_new_pid(struct tep_record *record)
+static int plugin_get_rec_wakeup_new_pid(struct tep_record *record)
 {
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
@@ -157,12 +157,12 @@  static int plugin_get_wakeup_new_pid(struct tep_record *record)
  * @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".
+ * @returns True if the Pid of the record matches the value of "pid".
  *	    Otherwise false.
  */
-bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
-			     struct kshark_entry *e,
-			     int pid)
+bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
+				 struct kshark_entry *e,
+				 int pid)
 {
 	struct plugin_sched_context *plugin_ctx;
 	struct tep_record *record = NULL;
@@ -182,7 +182,7 @@  bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
 				      record->data, &val);
 
 		if (val)
-			wakeup_pid = plugin_get_wakeup_pid(record);
+			wakeup_pid = plugin_get_rec_wakeup_pid(record);
 	}
 
 	if (plugin_ctx->sched_wakeup_new_event &&
@@ -194,7 +194,7 @@  bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
 				      record->data, &val);
 
 		if (val)
-			wakeup_pid = plugin_get_wakeup_new_pid(record);
+			wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
 	}
 
 	free_record(record);
@@ -212,12 +212,12 @@  bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
  * @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".
+ * @returns True if the Pid of the record matches the value of "pid".
  *	    Otherwise false.
  */
-bool plugin_switch_match_pid(struct kshark_context *kshark_ctx,
-			     struct kshark_entry *e,
-			     int pid)
+bool plugin_switch_match_rec_pid(struct kshark_context *kshark_ctx,
+				 struct kshark_entry *e,
+				 int pid)
 {
 	struct plugin_sched_context *plugin_ctx;
 	int switch_pid = -1;
@@ -239,6 +239,32 @@  bool plugin_switch_match_pid(struct kshark_context *kshark_ctx,
 	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_entry_pid(struct kshark_context *kshark_ctx,
+				   struct kshark_entry *e,
+				   int pid)
+{
+	struct plugin_sched_context *plugin_ctx;
+
+	plugin_ctx = plugin_sched_context_handler;
+
+	if (plugin_ctx->sched_switch_event &&
+	    e->event_id == plugin_ctx->sched_switch_event->id &&
+	    e->pid == pid)
+		return true;
+
+	return false;
+}
+
 static void plugin_sched_action(struct kshark_context *kshark_ctx,
 				struct tep_record *rec,
 				struct kshark_entry *entry)
diff --git a/kernel-shark-qt/src/plugins/sched_events.h b/kernel-shark-qt/src/plugins/sched_events.h
index 481413f..2036d5d 100644
--- a/kernel-shark-qt/src/plugins/sched_events.h
+++ b/kernel-shark-qt/src/plugins/sched_events.h
@@ -62,13 +62,15 @@  struct plugin_sched_context {
 
 int plugin_get_next_pid(struct tep_record *record);
 
-int plugin_get_wakeup_pid(struct tep_record *record);
+bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
+				 struct kshark_entry *e, int pid);
 
-bool plugin_wakeup_match_pid(struct kshark_context *kshark_ctx,
-			     struct kshark_entry *e, int pid);
+bool plugin_switch_match_rec_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);
+bool plugin_switch_match_entry_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);