[PATCH 0/8] Various modifications toward KS 1.0

[PATCH 0/8] Various modifications toward KS 1.0

[Yordan Karadzhov]

This series of patches contains various independent modifications and
bug fixes.

Steven,

Do not apply the last patch if you are not sure that KS v1.0 is ready
to be release.

Thanks!
Yordan

Yordan Karadzhov (8):
  kernel-shark: Add more sanity checks for model misbehavior detection
  kernel-shark: Do not copy the Upper Overflow bin when shifting forward
  kernel-shark: Check bin 0 for sched_switch event when plotting task
    graphs
  kernel-shark: Don't use Data collection when checking if the bin is
    empty
  kernel-shark: Make the time labels of the marker more readable
  kernel-shark: Fix the compile warnings about _GNU_SOURCE being
    redefined
  trace-cmd: Fix the printout of the KernelShark executable path
  kernel-shark: Version 1.0.0

 Makefile                              |  4 +--
 kernel-shark/CMakeLists.txt           |  6 ++--
 kernel-shark/src/KsDualMarker.cpp     | 42 +++++++++++++++++----------
 kernel-shark/src/KsPlotTools.cpp      | 34 +++++++++++++++-------
 kernel-shark/src/libkshark-configio.c |  4 +++
 kernel-shark/src/libkshark-model.c    | 38 ++++++++++++++----------
 kernel-shark/src/libkshark-plugin.c   |  5 ++++
 kernel-shark/src/libkshark.c          |  7 ++++-
 8 files changed, 92 insertions(+), 48 deletions(-)

-- 
2.17.1

[PATCH 1/8] kernel-shark: Add more sanity checks for model misbehavior detection

[Yordan Karadzhov]

I found that those checks are very useful for early detection of
misbehavior (bugs) of the visualization model. In particular those
checks helped me a lot when developing the multi-stream  branch of
KernelShark (future version 2.0).

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/libkshark-model.c | 28 +++++++++++++++++++---------
 kernel-shark/src/libkshark.c       |  7 ++++++-
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 2094795..a4041c3 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -299,6 +299,7 @@ static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo,
 static void ksmodel_set_bin_counts(struct kshark_trace_histo *histo)
 {
 	int i = 0, prev_not_empty;
+	ssize_t count_tmp;
 
 	histo->tot_count = 0;
 	memset(&histo->bin_count[0], 0,
@@ -329,12 +330,18 @@ static void ksmodel_set_bin_counts(struct kshark_trace_histo *histo)
 			 * empty bin, which will give us the number of data
 			 * rows in the "prev_not_empty" bin.
 			 */
-			histo->bin_count[prev_not_empty] =
-				histo->map[i] - histo->map[prev_not_empty];
+			count_tmp = histo->map[i] - histo->map[prev_not_empty];
+
+			/*
+			 * We will do a sanity check. The number of data rows
+			 * in the previous not empty bin must be greater than
+			 * zero.
+			 */
+			assert(count_tmp > 0);
+			histo->bin_count[prev_not_empty] = count_tmp;
 
 			if (prev_not_empty != LOB(histo))
-				histo->tot_count +=
-					histo->bin_count[prev_not_empty];
+				histo->tot_count += count_tmp;
 
 			prev_not_empty = i;
 		}
@@ -346,19 +353,22 @@ static void ksmodel_set_bin_counts(struct kshark_trace_histo *histo)
 		 * The Upper Overflow bin is empty. Use the size of the dataset
 		 * to calculate the content of the previouse not empty bin.
 		 */
-		histo->bin_count[prev_not_empty] = histo->data_size -
-						   histo->map[prev_not_empty];
+		count_tmp = histo->data_size - histo->map[prev_not_empty];
 	} else {
 		/*
 		 * Use the index of the first entry inside the Upper Overflow
 		 * bin to calculate the content of the previouse not empty
 		 * bin.
 		 */
-		histo->bin_count[prev_not_empty] = histo->map[UOB(histo)] -
-						   histo->map[prev_not_empty];
+		count_tmp = histo->map[UOB(histo)] - histo->map[prev_not_empty];
 	}
 
-	histo->tot_count += histo->bin_count[prev_not_empty];
+	/*
+	 * We will do a sanity check. The number of data rows in the last not
+	 * empty bin must be greater than zero.
+	 */
+	assert(count_tmp > 0);
+	histo->tot_count += histo->bin_count[prev_not_empty] = count_tmp;
 }
 
 /**
diff --git a/kernel-shark/src/libkshark.c b/kernel-shark/src/libkshark.c
index 5033e47..9a41945 100644
--- a/kernel-shark/src/libkshark.c
+++ b/kernel-shark/src/libkshark.c
@@ -1552,7 +1552,7 @@ const struct kshark_entry dummy_entry = {
 static const struct kshark_entry *
 get_entry(const struct kshark_entry_request *req,
           struct kshark_entry **data,
-          ssize_t *index, size_t start, ssize_t end, int inc)
+          ssize_t *index, ssize_t start, ssize_t end, int inc)
 {
 	struct kshark_context *kshark_ctx = NULL;
 	const struct kshark_entry *e = NULL;
@@ -1564,6 +1564,11 @@ get_entry(const struct kshark_entry_request *req,
 	if (!kshark_instance(&kshark_ctx))
 		return e;
 
+	/*
+	 * We will do a sanity check in order to protect against infinite
+	 * loops.
+	 */
+	assert((inc > 0 && start < end) || (inc < 0 && start > end));
 	for (i = start; i != end; i += inc) {
 		if (req->cond(kshark_ctx, data[i], req->val)) {
 			/*
-- 
2.17.1

[PATCH 2/8] kernel-shark: Do not copy the Upper Overflow bin when shifting forward

[Yordan Karadzhov]

ksmodel_shift_forward() copies the mapping indexes of all overlapping
bins of the model starting from bin "0" of the new histo up to bin
"histo->n_bins - n". Then the mapping index of the old Upper Overflow
bin is considered to be the mapping index of first non-overlapping bin,
which is wrong. It is wrong because in ksmodel_set_upper_edge() the
value of "histo->max" is considered inside the range of the model hence
the Upper Overflow bin starts at "histo->max + 1" but the first
non-overlapping bin will start at exactly "histo->max".

Fixes: f97e31f00 ("kernel-shark-qt: Introduce the visualization model used by the Qt-based KS")
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/libkshark-model.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index a4041c3..b71a9b8 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -500,13 +500,11 @@ void ksmodel_shift_forward(struct kshark_trace_histo *histo, size_t n)
 		sizeof(histo->map[0]) * (histo->n_bins - n));
 
 	/*
-	 * The mapping index of the old Upper Overflow bin is now index of the
-	 * first new bin.
+	 * Calculate only the content of the new (non-overlapping) bins.
+	 * Start from the last copied bin and set the edge of each consecutive
+	 * bin.
 	 */
-	bin = UOB(histo) - n;
-	histo->map[bin] = histo->map[UOB(histo)];
-
-	/* Calculate only the content of the new (non-overlapping) bins. */
+	bin = histo->n_bins - n - 1;
 	for (; bin < histo->n_bins; ++bin) {
 		ksmodel_set_next_bin_edge(histo, bin, last_row);
 		if (histo->map[bin + 1] > 0)
-- 
2.17.1

[PATCH 3/8] kernel-shark: Check bin 0 for sched_switch event when plotting task graphs

[Yordan Karadzhov]

Handle the case when task is active in the Lower Overflow bin and the
sched_switch event that close the task graph happens to be in bin 0.

Fixes: ba206aaa45 ("kernel-shark-qt: Add C++ API for drawing of Graphs")
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsPlotTools.cpp | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/kernel-shark/src/KsPlotTools.cpp b/kernel-shark/src/KsPlotTools.cpp
index 816fa56..f97c6e4 100644
--- a/kernel-shark/src/KsPlotTools.cpp
+++ b/kernel-shark/src/KsPlotTools.cpp
@@ -1020,16 +1020,26 @@ void Graph::fillTaskGraph(int pid)
 		if (cpuFront >= 0) {
 			/*
 			 * The Lower Overflow Bin contains data from this Task.
-			 * Now look again in the Lower Overflow Bin and find
-			 * the Pid of the last active task on the same CPU.
+			 * Now look again in the Lower Overflow Bin and Bim 0
+			 * and find the Pid of the last active task on the same
+			 * CPU.
 			 */
-			int pidCpu = ksmodel_get_pid_back(_histoPtr,
-							  LOWER_OVERFLOW_BIN,
-							  cpuFront,
-							  false,
-							  _collectionPtr,
-							  nullptr);
-			if (pidCpu == pid) {
+			int pidCpu0, pidCpuLOB;
+
+			pidCpu0 = ksmodel_get_pid_back(_histoPtr,
+						       0,
+						       cpuFront,
+						       false,
+						       _collectionPtr,
+						       nullptr);
+
+			pidCpuLOB = ksmodel_get_pid_back(_histoPtr,
+							 LOWER_OVERFLOW_BIN,
+							 cpuFront,
+							 false,
+							 _collectionPtr,
+							 nullptr);
+			if (pidCpu0 < 0 && pidCpuLOB == pid) {
 				/*
 				 * The Task is the last one running on this
 				 * CPU. Set the Pid of the bin. In this case
-- 
2.17.1

[PATCH 4/8] kernel-shark: Don't use Data collection when checking if the bin is empty

[Yordan Karadzhov]

When plotting a task graph and no data from the Task is found in the bin,
we check the CPU, previously used by the task, searching for data from
another task running on the same CPU. However this search will always
fail if we use the Data collection of the plotted task.

Fixes: ba206aaa45 ("kernel-shark-qt: Add C++ API for drawing of Graphs")
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsPlotTools.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel-shark/src/KsPlotTools.cpp b/kernel-shark/src/KsPlotTools.cpp
index f97c6e4..d07f414 100644
--- a/kernel-shark/src/KsPlotTools.cpp
+++ b/kernel-shark/src/KsPlotTools.cpp
@@ -929,13 +929,15 @@ void Graph::fillTaskGraph(int pid)
 		} else {
 			/*
 			 * No data from the Task in this bin. Check the CPU,
-			 * previously used by the task.
+			 * previously used by the task. We are looking for
+			 * data from another task running on the same CPU,
+			 * hence we cannot use the collection of this task.
 			 */
 			int cpuPid = ksmodel_get_pid_back(_histoPtr,
 							  bin,
 							  lastCpu,
 							  false,
-							  _collectionPtr,
+							  nullptr, // No collection
 							  nullptr);
 
 			if (cpuPid != KS_EMPTY_BIN) {
-- 
2.17.1

[PATCH 5/8] kernel-shark: Make the time labels of the marker more readable

[Yordan Karadzhov]

The precision of the displayed time of the two markers is set to
1 microsecond (as in the table). In the same time the precision of
the time difference (Delta) is now 1 nanosecond, because we expect
that the user do not care that much about the absolute time of the
event, but may want to measure the interval between two events with
the highest possible precision. The displayed time is formatted
(spaces added) in a way that aims to make it easy to read the
milliseconds and the microseconds.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsDualMarker.cpp | 42 +++++++++++++++++++------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/kernel-shark/src/KsDualMarker.cpp b/kernel-shark/src/KsDualMarker.cpp
index 43bc177..5dcbaae 100644
--- a/kernel-shark/src/KsDualMarker.cpp
+++ b/kernel-shark/src/KsDualMarker.cpp
@@ -329,29 +329,39 @@ void KsDualMarkerSM::updateMarkers(const KsDataStore &data,
  */
 void KsDualMarkerSM::updateLabels()
 {
-	QString mark, delta;
+	char separator(' ');
+	int precision(6); // 1 microsecond precision.
+
+	auto lamSetTimeLabel = [&precision, &separator] (QLabel &l, int64_t t) {
+		QString time = KsUtils::Ts2String(t, precision);
+		int i = time.indexOf('.') + 4;
+
+		/* Insert separators for milliseconds amd microseconds. */
+		while (i < time.size()) {
+			time.insert(i, separator);
+			i = i + 4;
+		}
+
+		l.setText(time);
+	};
 
 	// Marker A
-	if (_markA._isSet) {
-		mark = KsUtils::Ts2String(_markA._ts, 7);
-		_labelMA.setText(mark);
-	} else {
-		_labelMA.setText("");
-	}
+	if (_markA._isSet)
+		lamSetTimeLabel(_labelMA, _markA._ts);
+	else
+		_labelMA.clear();
 
 	// Marker B
-	if (_markB._isSet) {
-		mark = KsUtils::Ts2String(_markB._ts, 7);
-		_labelMB.setText(mark);
-	} else {
-		_labelMB.setText("");
-	}
+	if (_markB._isSet)
+		lamSetTimeLabel(_labelMB, _markB._ts);
+	else
+		_labelMB.clear();
 
 	// Delta
 	if (_markA._isSet && _markB._isSet) {
-		delta = KsUtils::Ts2String(_markB._ts - _markA._ts, 7);
-		_labelDelta.setText(delta);
+		precision = 9; // 1 nanoseconds precision.
+		lamSetTimeLabel(_labelDelta, _markB._ts - _markA._ts);
 	} else {
-		_labelDelta.setText("");
+		_labelDelta.clear();
 	}
 }
-- 
2.17.1

[PATCH 6/8] kernel-shark: Fix the compile warnings about _GNU_SOURCE being redefined

[Yordan Karadzhov]

Fixes wornings from:

kernel-shark/src/libkshark-configio.c

and

kernel-shark/src/libkshark-plugin.c

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/libkshark-configio.c | 4 ++++
 kernel-shark/src/libkshark-plugin.c   | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/kernel-shark/src/libkshark-configio.c b/kernel-shark/src/libkshark-configio.c
index d7b8a69..ab88864 100644
--- a/kernel-shark/src/libkshark-configio.c
+++ b/kernel-shark/src/libkshark-configio.c
@@ -10,8 +10,12 @@
   */
 
 // C
+#ifndef _GNU_SOURCE
 /** Use GNU C Library. */
 #define _GNU_SOURCE
+
+#endif
+
 #include <stdio.h>
 #include <sys/stat.h>
 
diff --git a/kernel-shark/src/libkshark-plugin.c b/kernel-shark/src/libkshark-plugin.c
index 10e4647..4b21392 100644
--- a/kernel-shark/src/libkshark-plugin.c
+++ b/kernel-shark/src/libkshark-plugin.c
@@ -10,7 +10,12 @@
   */
 
 // C
+#ifndef _GNU_SOURCE
+/** Use GNU C Library. */
 #define _GNU_SOURCE
+
+#endif
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-- 
2.17.1

[PATCH 7/8] trace-cmd: Fix the printout of the KernelShark executable path

[Yordan Karadzhov]

Adding  ‘@’ to the line that is echoing in order to suppress echoing
the line itself.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f0f7e92..b780718 100644
--- a/Makefile
+++ b/Makefile
@@ -258,8 +258,8 @@ $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt
 
 gui: force $(CMD_TARGETS) $(kshark-dir)/build/Makefile
 	$(Q)$(MAKE) $(S) -C $(kshark-dir)/build
-	echo "gui build complete"
-	echo "  kernelshark located at $(kshark-dir)/bin"
+	@echo "gui build complete"
+	@echo "  kernelshark located at $(kshark-dir)/bin"
 
 trace-cmd: force $(LIBTRACEEVENT_STATIC) $(LIBTRACECMD_STATIC)
 	$(Q)$(MAKE) -C $(src)/tracecmd $(obj)/tracecmd/$@
-- 
2.17.1

[PATCH 8/8] kernel-shark: Version 1.0.0

[Yordan Karadzhov]

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/CMakeLists.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
index 9460026..a1859af 100644
--- a/kernel-shark/CMakeLists.txt
+++ b/kernel-shark/CMakeLists.txt
@@ -4,9 +4,9 @@ cmake_minimum_required(VERSION 2.8.11 FATAL_ERROR)
 # Set the name and version of the project
 project(kernel-shark)
 
-set(KS_VERSION_MAJOR 0)
-set(KS_VERSION_MINOR 9)
-set(KS_VERSION_PATCH 8)
+set(KS_VERSION_MAJOR 1)
+set(KS_VERSION_MINOR 0)
+set(KS_VERSION_PATCH 0)
 set(KS_VERSION_STRING ${KS_VERSION_MAJOR}.${KS_VERSION_MINOR}.${KS_VERSION_PATCH})
 message("\n project: Kernel Shark: (version: ${KS_VERSION_STRING})\n")
 
-- 
2.17.1