[PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing

[PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing

[Steven Rostedt]

Hi Yordan,

I was using kernel shark to examine vsock events, and noticed that when I
only enabled sched_waking events, it did not give me the wake up boxes in
the task plots. I decided to look at the code and modify it to handle it.

When doing that, I noticed that the "success" field of the event was being
used. This is an obsolete field that will hopefully someday be removed. So I
added a patch to remove that. I also did not want to just add cut and paste
code to add the sched_waking, and instead made helper functions to process
the events all the same. And finally, I added the sched_waking code.

Can you review these patches and if they are fine by you, give your
"Reviewed-by" tag. Otherwise let me know if there's an issue that I should
fix.

Thanks!

-- Steve



Tag SHA1: 746c45bb20a4c860d36c48d279286c67202248a0
Head SHA1: 746c45bb20a4c860d36c48d279286c67202248a0


Steven Rostedt (VMware) (4):
      kernel-shark: Remove testing of "success" field of wakeup events
      kernel-shark: Consolidate duplicate code of the sched_wakeup events
      kernel-shark: Remove plugin_get_rec_wakeup_pid()
      kernel-shark: Add sched_waking event processing to sched_waking

----
 kernel-shark/src/plugins/sched_events.c | 147 +++++++++++++++-----------------
 kernel-shark/src/plugins/sched_events.h |  12 ++-
 2 files changed, 76 insertions(+), 83 deletions(-)

[PATCH 1/4] kernel-shark: Remove testing of "success" field of wakeup events

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Since 2011 (or Linux version 3.0) the sched wakeup event "success" field has
been hardcoded to true (or 1). There's no reason to waste cycles checking it
in any newer kernel, which is most of them. Just ignore it. Even when
running on older kernels which may have a success=0 value, the algorithm
should still work.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel-shark/src/plugins/sched_events.c | 25 +++----------------------
 kernel-shark/src/plugins/sched_events.h |  8 --------
 2 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index c0328e804911..5b1af7722624 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -68,9 +68,6 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 	plugin_ctx->sched_wakeup_pid_field =
 		tep_find_any_field(event, "pid");
 
-	plugin_ctx->sched_wakeup_success_field =
-		tep_find_field(event, "success");
-
 	event = tep_find_event_by_name(plugin_ctx->pevent,
 				       "sched", "sched_wakeup_new");
 	if (!event)
@@ -80,9 +77,6 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 	plugin_ctx->sched_wakeup_new_pid_field =
 		tep_find_any_field(event, "pid");
 
-	plugin_ctx->sched_wakeup_new_success_field =
-		tep_find_field(event, "success");
-
 	plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc();
 
 	return true;
@@ -175,8 +169,7 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 {
 	struct plugin_sched_context *plugin_ctx;
 	struct tep_record *record = NULL;
-	unsigned long long val;
-	int ret, wakeup_pid = -1;
+	int wakeup_pid = -1;
 
 	plugin_ctx = plugin_sched_context_handler;
 	if (!plugin_ctx)
@@ -185,25 +178,13 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 	if (plugin_ctx->sched_wakeup_event &&
 	    e->event_id == plugin_ctx->sched_wakeup_event->id) {
 		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
-
-		/* We only want those that actually woke up the task. */
-		ret = tep_read_number_field(plugin_ctx->sched_wakeup_success_field,
-					    record->data, &val);
-
-		if (ret == 0 && val)
-			wakeup_pid = plugin_get_rec_wakeup_pid(record);
+		wakeup_pid = plugin_get_rec_wakeup_pid(record);
 	}
 
 	if (plugin_ctx->sched_wakeup_new_event &&
 	    e->event_id == plugin_ctx->sched_wakeup_new_event->id) {
 		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
-
-		/* We only want those that actually woke up the task. */
-		ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field,
-					    record->data, &val);
-
-		if (ret == 0 && val)
-			wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
+		wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
 	}
 
 	free_record(record);
diff --git a/kernel-shark/src/plugins/sched_events.h b/kernel-shark/src/plugins/sched_events.h
index fb1849ee8ffb..0beb63fe2b48 100644
--- a/kernel-shark/src/plugins/sched_events.h
+++ b/kernel-shark/src/plugins/sched_events.h
@@ -45,20 +45,12 @@ struct plugin_sched_context {
 	/** Pointer to the sched_wakeup_pid_field format descriptor. */
 	struct tep_format_field	*sched_wakeup_pid_field;
 
-	/** Pointer to the sched_wakeup_success_field format descriptor. */
-	struct tep_format_field	*sched_wakeup_success_field;
-
 	/** Pointer to the sched_wakeup_new_event object. */
 	struct tep_event	*sched_wakeup_new_event;
 
 	/** Pointer to the sched_wakeup_new_pid_field format descriptor. */
 	struct tep_format_field	*sched_wakeup_new_pid_field;
 
-	/**
-	 * Pointer to the sched_wakeup_new_success_field format descriptor.
-	 */
-	struct tep_format_field	*sched_wakeup_new_success_field;
-
 	/** List of Data collections used by this plugin. */
 	struct kshark_entry_collection	*collections;
 
-- 
2.20.1

[PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The logic used for sched_wakeup and sched_wakeup_new events are basically
the same. Create helper functions to be called to do the duplicate code with
just passing in the parameters that are unique to the events.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel-shark/src/plugins/sched_events.c | 106 +++++++++++++++---------
 1 file changed, 66 insertions(+), 40 deletions(-)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index 5b1af7722624..0f5c08e3508d 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -23,10 +23,27 @@
 /** Plugin context instance. */
 struct plugin_sched_context *plugin_sched_context_handler = NULL;
 
+static bool define_wakeup_event(struct tep_handle *tep, const char *wakeup_name,
+				struct tep_event **wakeup_event,
+				struct tep_format_field **pid_field)
+{
+	struct tep_event *event;
+
+	event = tep_find_event_by_name(tep, "sched", wakeup_name);
+	if (!event)
+		return false;
+
+	*wakeup_event = event;
+	*pid_field = tep_find_any_field(event, "pid");
+
+	return true;
+}
+
 static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 {
 	struct plugin_sched_context *plugin_ctx;
 	struct tep_event *event;
+	bool wakeup_found;
 
 	/* No context should exist when we initialize the plugin. */
 	assert(plugin_sched_context_handler == NULL);
@@ -59,23 +76,17 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 	plugin_ctx->sched_switch_prev_state_field =
 		tep_find_field(event, "prev_state");
 
-	event = tep_find_event_by_name(plugin_ctx->pevent,
-				      "sched", "sched_wakeup");
-	if (!event)
-		return false;
 
-	plugin_ctx->sched_wakeup_event = event;
-	plugin_ctx->sched_wakeup_pid_field =
-		tep_find_any_field(event, "pid");
+	wakeup_found = define_wakeup_event(kshark_ctx->pevent, "sched_wakeup",
+					   &plugin_ctx->sched_wakeup_event,
+					   &plugin_ctx->sched_wakeup_pid_field);
 
-	event = tep_find_event_by_name(plugin_ctx->pevent,
-				       "sched", "sched_wakeup_new");
-	if (!event)
-		return false;
+	wakeup_found |= define_wakeup_event(kshark_ctx->pevent, "sched_wakeup_new",
+					   &plugin_ctx->sched_wakeup_new_event,
+					   &plugin_ctx->sched_wakeup_new_pid_field);
 
-	plugin_ctx->sched_wakeup_new_event = event;
-	plugin_ctx->sched_wakeup_new_pid_field =
-		tep_find_any_field(event, "pid");
+	if (!wakeup_found)
+		return false;
 
 	plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc();
 
@@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx,
 			tep_register_comm(kshark_ctx->pevent, comm, pid);
 }
 
-static int plugin_get_rec_wakeup_new_pid(struct tep_record *record)
+int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e,
+		    struct tep_event *wakeup_event, struct tep_format_field *pid_field)
 {
-	struct plugin_sched_context *plugin_ctx =
-		plugin_sched_context_handler;
+	struct tep_record *record;
 	unsigned long long val;
 	int ret;
 
-	ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
-				    record->data, &val);
+	if (!wakeup_event || e->event_id != wakeup_event->id)
+		return -1;
 
-	return ret ? : val;
+	record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
+	ret = tep_read_number_field(pid_field, record->data, &val);
+	free_record(record);
+
+	if (ret)
+		return -1;
+
+	return val;
+}
+
+static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx,
+				 struct kshark_context *kshark_ctx,
+				 struct kshark_entry *e,
+				 int pid)
+{
+	struct tep_event *wakeup_events[] = {
+		plugin_ctx->sched_wakeup_event,
+		plugin_ctx->sched_wakeup_new_event,
+	};
+	struct tep_format_field *wakeup_fields[] = {
+		plugin_ctx->sched_wakeup_pid_field,
+		plugin_ctx->sched_wakeup_new_pid_field,
+	};
+	int i, wakeup_pid = -1;
+
+	for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) {
+		wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]);
+		if (wakeup_pid >= 0)
+			break;
+	}
+
+	if (wakeup_pid >= 0 && wakeup_pid == pid)
+		return true;
+
+	return false;
 }
 
 /**
@@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 				 int pid)
 {
 	struct plugin_sched_context *plugin_ctx;
-	struct tep_record *record = NULL;
-	int wakeup_pid = -1;
 
 	plugin_ctx = plugin_sched_context_handler;
 	if (!plugin_ctx)
 		return false;
 
-	if (plugin_ctx->sched_wakeup_event &&
-	    e->event_id == plugin_ctx->sched_wakeup_event->id) {
-		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
-		wakeup_pid = plugin_get_rec_wakeup_pid(record);
-	}
-
-	if (plugin_ctx->sched_wakeup_new_event &&
-	    e->event_id == plugin_ctx->sched_wakeup_new_event->id) {
-		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
-		wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
-	}
-
-	free_record(record);
-
-	if (wakeup_pid >= 0 && wakeup_pid == pid)
-		return true;
-
-	return false;
+	return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid);
 }
 
 /**
-- 
2.20.1

[PATCH 3/4] kernel-shark: Remove plugin_get_rec_wakeup_pid()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The function plugin_get_rec_wakeup_pid() is not used by anything. Remove
it.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel-shark/src/plugins/sched_events.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index 0f5c08e3508d..0cfe5b5367e2 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -111,24 +111,6 @@ int plugin_get_next_pid(struct tep_record *record)
 	return ret ? : val;
 }
 
-/**
- * @brief Get the Process Id of the task being woke up.
- *
- * @param record: Input location for a sched_wakeup record.
- */
-int plugin_get_rec_wakeup_pid(struct tep_record *record)
-{
-	struct plugin_sched_context *plugin_ctx =
-		plugin_sched_context_handler;
-	unsigned long long val;
-	int ret;
-
-	ret = tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
-				    record->data, &val);
-
-	return ret ? : val;
-}
-
 static void plugin_register_command(struct kshark_context *kshark_ctx,
 				    struct tep_record *record,
 				    int pid)
-- 
2.20.1

[PATCH 4/4] kernel-shark: Add sched_waking event processing to sched_waking

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

It is now more common to enable the sched_waking event than to use the
sched_wakeup or sched_wakeup_new, as it encompasses both. But the sched
plugin does not handle that. Fix that by including the sched_waking event as
an event to use for processing schedule wakeup events.

Signen-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel-shark/src/plugins/sched_events.c | 6 ++++++
 kernel-shark/src/plugins/sched_events.h | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index 0cfe5b5367e2..14f8edb2836b 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -85,6 +85,10 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 					   &plugin_ctx->sched_wakeup_new_event,
 					   &plugin_ctx->sched_wakeup_new_pid_field);
 
+	wakeup_found |= define_wakeup_event(kshark_ctx->pevent, "sched_waking",
+					   &plugin_ctx->sched_waking_event,
+					   &plugin_ctx->sched_waking_pid_field);
+
 	if (!wakeup_found)
 		return false;
 
@@ -158,10 +162,12 @@ static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx,
 				 int pid)
 {
 	struct tep_event *wakeup_events[] = {
+		plugin_ctx->sched_waking_event,
 		plugin_ctx->sched_wakeup_event,
 		plugin_ctx->sched_wakeup_new_event,
 	};
 	struct tep_format_field *wakeup_fields[] = {
+		plugin_ctx->sched_waking_pid_field,
 		plugin_ctx->sched_wakeup_pid_field,
 		plugin_ctx->sched_wakeup_new_pid_field,
 	};
diff --git a/kernel-shark/src/plugins/sched_events.h b/kernel-shark/src/plugins/sched_events.h
index 0beb63fe2b48..dbc9963cbc4e 100644
--- a/kernel-shark/src/plugins/sched_events.h
+++ b/kernel-shark/src/plugins/sched_events.h
@@ -51,6 +51,12 @@ struct plugin_sched_context {
 	/** Pointer to the sched_wakeup_new_pid_field format descriptor. */
 	struct tep_format_field	*sched_wakeup_new_pid_field;
 
+	/** Pointer to the sched_waking_event object. */
+	struct tep_event        *sched_waking_event;
+
+	/** Pointer to the sched_waking_pid_field format descriptor. */
+	struct tep_format_field *sched_waking_pid_field;
+
 	/** List of Data collections used by this plugin. */
 	struct kshark_entry_collection	*collections;
 
-- 
2.20.1

Re: [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events

[Yordan Karadzhov]

On 11.02.19 г. 3:18 ч., Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The logic used for sched_wakeup and sched_wakeup_new events are basically
> the same. Create helper functions to be called to do the duplicate code with
> just passing in the parameters that are unique to the events.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>   kernel-shark/src/plugins/sched_events.c | 106 +++++++++++++++---------
>   1 file changed, 66 insertions(+), 40 deletions(-)
> 
> diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
> index 5b1af7722624..0f5c08e3508d 100644
> --- a/kernel-shark/src/plugins/sched_events.c
> +++ b/kernel-shark/src/plugins/sched_events.c
> @@ -23,10 +23,27 @@
>   /** Plugin context instance. */
>   struct plugin_sched_context *plugin_sched_context_handler = NULL;
>   
> +static bool define_wakeup_event(struct tep_handle *tep, const char *wakeup_name,
> +				struct tep_event **wakeup_event,
> +				struct tep_format_field **pid_field)
> +{
> +	struct tep_event *event;
> +
> +	event = tep_find_event_by_name(tep, "sched", wakeup_name);
> +	if (!event)
> +		return false;
> +
> +	*wakeup_event = event;
> +	*pid_field = tep_find_any_field(event, "pid");
> +
> +	return true;
> +}
> +
>   static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
>   {
>   	struct plugin_sched_context *plugin_ctx;
>   	struct tep_event *event;
> +	bool wakeup_found;
>   
>   	/* No context should exist when we initialize the plugin. */
>   	assert(plugin_sched_context_handler == NULL);
> @@ -59,23 +76,17 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
>   	plugin_ctx->sched_switch_prev_state_field =
>   		tep_find_field(event, "prev_state");
>   
> -	event = tep_find_event_by_name(plugin_ctx->pevent,
> -				      "sched", "sched_wakeup");
> -	if (!event)
> -		return false;
>   
> -	plugin_ctx->sched_wakeup_event = event;
> -	plugin_ctx->sched_wakeup_pid_field =
> -		tep_find_any_field(event, "pid");
> +	wakeup_found = define_wakeup_event(kshark_ctx->pevent, "sched_wakeup",
> +					   &plugin_ctx->sched_wakeup_event,
> +					   &plugin_ctx->sched_wakeup_pid_field);
>   
> -	event = tep_find_event_by_name(plugin_ctx->pevent,
> -				       "sched", "sched_wakeup_new");
> -	if (!event)
> -		return false;
> +	wakeup_found |= define_wakeup_event(kshark_ctx->pevent, "sched_wakeup_new",
> +					   &plugin_ctx->sched_wakeup_new_event,
> +					   &plugin_ctx->sched_wakeup_new_pid_field);
>   
> -	plugin_ctx->sched_wakeup_new_event = event;
> -	plugin_ctx->sched_wakeup_new_pid_field =
> -		tep_find_any_field(event, "pid");
> +	if (!wakeup_found)
> +		return false;
>   
>   	plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc();
>   
> @@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx,
>   			tep_register_comm(kshark_ctx->pevent, comm, pid);
>   }
>   
> -static int plugin_get_rec_wakeup_new_pid(struct tep_record *record)

This patch silently removes the "plugin_get_rec_wakeup_new_pid()" 
function which is the equivalent of "plugin_get_rec_wakeup_pid()", but 
for "wakeup_new" events. I guess, you are removing 
"plugin_get_rec_wakeup_new_pid()" here, because it is defined static but 
in fact there is no reason for "plugin_get_rec_wakeup_pid()" being 
non-static (this is my mistake). So just to keep everything consistent, 
please do the removal of "plugin_get_rec_wakeup_new_pid()" in the next 
patch (together with the removal of "plugin_get_rec_wakeup_pid()").


> +int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e,
> +		    struct tep_event *wakeup_event, struct tep_format_field *pid_field)
>   {
> -	struct plugin_sched_context *plugin_ctx =
> -		plugin_sched_context_handler;
> +	struct tep_record *record;
>   	unsigned long long val;
>   	int ret;
>   
> -	ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
> -				    record->data, &val);
> +	if (!wakeup_event || e->event_id != wakeup_event->id)
> +		return -1;
>   
> -	return ret ? : val;
> +	record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
> +	ret = tep_read_number_field(pid_field, record->data, &val);
> +	free_record(record);
> +
> +	if (ret)
> +		return -1;
> +
> +	return val;
> +}
> +
> +static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx,
> +				 struct kshark_context *kshark_ctx,
> +				 struct kshark_entry *e,
> +				 int pid)
> +{
> +	struct tep_event *wakeup_events[] = {
> +		plugin_ctx->sched_wakeup_event,
> +		plugin_ctx->sched_wakeup_new_event,
> +	};
> +	struct tep_format_field *wakeup_fields[] = {
> +		plugin_ctx->sched_wakeup_pid_field,
> +		plugin_ctx->sched_wakeup_new_pid_field,
> +	};
> +	int i, wakeup_pid = -1;
> +
> +	for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) {
> +		wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]);
> +		if (wakeup_pid >= 0)
> +			break;
> +	}
> +
> +	if (wakeup_pid >= 0 && wakeup_pid == pid)
> +		return true;
> +
> +	return false;
>   }
>   
>   /**
> @@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
>   				 int pid)
>   {
>   	struct plugin_sched_context *plugin_ctx;
> -	struct tep_record *record = NULL;
> -	int wakeup_pid = -1;
>   
>   	plugin_ctx = plugin_sched_context_handler;
>   	if (!plugin_ctx)
>   		return false;
>   
> -	if (plugin_ctx->sched_wakeup_event &&
> -	    e->event_id == plugin_ctx->sched_wakeup_event->id) {
> -		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
> -		wakeup_pid = plugin_get_rec_wakeup_pid(record);
> -	}
> -
> -	if (plugin_ctx->sched_wakeup_new_event &&
> -	    e->event_id == plugin_ctx->sched_wakeup_new_event->id) {
> -		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
> -		wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
> -	}
> -
> -	free_record(record);
> -
> -	if (wakeup_pid >= 0 && wakeup_pid == pid)
> -		return true;
> -
> -	return false;
> +	return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid);
>   }
>

I see no point in defining "static bool wakeup_match_rec_pid()" and then 
having "plugin_wakeup_match_rec_pid()" which is doing nothing but just 
calling this static function.


Everything else in this patch series is OK.

Thank you very much!
Yordan


Reviewed-by: Yordan Karadzhov <ykaradzhov@vmware.com>


>   /**
> 

Re: [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events

[Steven Rostedt]

On Mon, 11 Feb 2019 08:53:16 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

Hi Yordan,

Thanks for reviewing, you pointed out things I expected you to ;-)

> > @@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx,
> >   			tep_register_comm(kshark_ctx->pevent, comm, pid);
> >   }
> >   
> > -static int plugin_get_rec_wakeup_new_pid(struct tep_record *record)  
> 
> This patch silently removes the "plugin_get_rec_wakeup_new_pid()" 
> function which is the equivalent of "plugin_get_rec_wakeup_pid()", but 
> for "wakeup_new" events. I guess, you are removing 
> "plugin_get_rec_wakeup_new_pid()" here, because it is defined static but 
> in fact there is no reason for "plugin_get_rec_wakeup_pid()" being 
> non-static (this is my mistake). So just to keep everything consistent, 
> please do the removal of "plugin_get_rec_wakeup_new_pid()" in the next 
> patch (together with the removal of "plugin_get_rec_wakeup_pid()").
> 

Actually, I didn't really silently remove it. Without removing it, gcc
would complain that there's a static function not being used, and
that's a no-no for a patch to cause a new warning. It's fine to remove
static functions when removing their users in the code that is being
modified.

Now, I would have also removed plugin_get_rec_wakeup_pid(), but because
that's a global function, it doesn't cause the warning, and may also be
possibly used outside of that file (and by code I'm not working with).

That's why I removed the static one here. It's required because without
doing so, it causes gcc to generate a warning. And the reason for
removing the other function was because it is global outside of the
code that I changed, which requires a separate patch. Now if both were
global, I would be able to remove them together in a separate patch, or
if both were static, I would have removed both here. But since one's
global and one's static, they need to be removed in separate places.

Make sense?

> 
> > +int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e,
> > +		    struct tep_event *wakeup_event, struct tep_format_field *pid_field)
> >   {
> > -	struct plugin_sched_context *plugin_ctx =
> > -		plugin_sched_context_handler;
> > +	struct tep_record *record;
> >   	unsigned long long val;
> >   	int ret;
> >   
> > -	ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
> > -				    record->data, &val);
> > +	if (!wakeup_event || e->event_id != wakeup_event->id)
> > +		return -1;
> >   
> > -	return ret ? : val;
> > +	record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
> > +	ret = tep_read_number_field(pid_field, record->data, &val);
> > +	free_record(record);
> > +
> > +	if (ret)
> > +		return -1;
> > +
> > +	return val;
> > +}
> > +
> > +static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx,
> > +				 struct kshark_context *kshark_ctx,
> > +				 struct kshark_entry *e,
> > +				 int pid)
> > +{
> > +	struct tep_event *wakeup_events[] = {
> > +		plugin_ctx->sched_wakeup_event,
> > +		plugin_ctx->sched_wakeup_new_event,
> > +	};
> > +	struct tep_format_field *wakeup_fields[] = {
> > +		plugin_ctx->sched_wakeup_pid_field,
> > +		plugin_ctx->sched_wakeup_new_pid_field,
> > +	};
> > +	int i, wakeup_pid = -1;
> > +
> > +	for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) {
> > +		wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]);
> > +		if (wakeup_pid >= 0)
> > +			break;
> > +	}
> > +
> > +	if (wakeup_pid >= 0 && wakeup_pid == pid)
> > +		return true;
> > +
> > +	return false;
> >   }
> >   
> >   /**
> > @@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
> >   				 int pid)
> >   {
> >   	struct plugin_sched_context *plugin_ctx;
> > -	struct tep_record *record = NULL;
> > -	int wakeup_pid = -1;
> >   
> >   	plugin_ctx = plugin_sched_context_handler;
> >   	if (!plugin_ctx)
> >   		return false;
> >   
> > -	if (plugin_ctx->sched_wakeup_event &&
> > -	    e->event_id == plugin_ctx->sched_wakeup_event->id) {
> > -		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
> > -		wakeup_pid = plugin_get_rec_wakeup_pid(record);
> > -	}
> > -
> > -	if (plugin_ctx->sched_wakeup_new_event &&
> > -	    e->event_id == plugin_ctx->sched_wakeup_new_event->id) {
> > -		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
> > -		wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
> > -	}
> > -
> > -	free_record(record);
> > -
> > -	if (wakeup_pid >= 0 && wakeup_pid == pid)
> > -		return true;
> > -
> > -	return false;
> > +	return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid);
> >   }
> >  
> 
> I see no point in defining "static bool wakeup_match_rec_pid()" and then 
> having "plugin_wakeup_match_rec_pid()" which is doing nothing but just 
> calling this static function.

It's because of the initialization of wakeup_events and wakeup_fields.

	struct tep_event *wakeup_events[] = {
		plugin_ctx->sched_waking_event,
		plugin_ctx->sched_wakeup_event,
		plugin_ctx->sched_wakeup_new_event,
	};
	struct tep_format_field *wakeup_fields[] = {
		plugin_ctx->sched_waking_pid_field,
		plugin_ctx->sched_wakeup_pid_field,
		plugin_ctx->sched_wakeup_new_pid_field,
	};

Which are initialized by the values in plugin_ctx, but plugin_ctx is
not initialized until after the static is set up:

	plugin_ctx = plugin_sched_context_handler;
	if (!plugin_ctx)
		return false;

Thus to handle this case, I simply moved the code into a helper
function, and had the main function just initialize plugin_ctx (return
if NULL), and then call the helper function that can initialize the
arrays on the stack.

Make sense?

-- Steve


> 
> Everything else in this patch series is OK.
> 
> Thank you very much!
> Yordan
> 
> 
> Reviewed-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> 
> 
> >   /**
> >   

Re: [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events

[Yordan Karadzhov]

On 11.02.19 г. 16:44 ч., Steven Rostedt wrote:
> On Mon, 11 Feb 2019 08:53:16 +0000
> Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> 
> Hi Yordan,
> 
> Thanks for reviewing, you pointed out things I expected you to ;-)
> 
>>> @@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx,
>>>    			tep_register_comm(kshark_ctx->pevent, comm, pid);
>>>    }
>>>    
>>> -static int plugin_get_rec_wakeup_new_pid(struct tep_record *record)
>>
>> This patch silently removes the "plugin_get_rec_wakeup_new_pid()"
>> function which is the equivalent of "plugin_get_rec_wakeup_pid()", but
>> for "wakeup_new" events. I guess, you are removing
>> "plugin_get_rec_wakeup_new_pid()" here, because it is defined static but
>> in fact there is no reason for "plugin_get_rec_wakeup_pid()" being
>> non-static (this is my mistake). So just to keep everything consistent,
>> please do the removal of "plugin_get_rec_wakeup_new_pid()" in the next
>> patch (together with the removal of "plugin_get_rec_wakeup_pid()").
>>
> 
> Actually, I didn't really silently remove it. Without removing it, gcc
> would complain that there's a static function not being used, and
> that's a no-no for a patch to cause a new warning. It's fine to remove
> static functions when removing their users in the code that is being
> modified.
> 
> Now, I would have also removed plugin_get_rec_wakeup_pid(), but because
> that's a global function, it doesn't cause the warning, and may also be
> possibly used outside of that file (and by code I'm not working with).
> 
> That's why I removed the static one here. It's required because without
> doing so, it causes gcc to generate a warning. And the reason for
> removing the other function was because it is global outside of the
> code that I changed, which requires a separate patch. Now if both were
> global, I would be able to remove them together in a separate patch, or
> if both were static, I would have removed both here. But since one's
> global and one's static, they need to be removed in separate places.
> 
> Make sense?

Yes, this makes sense.

> 
>>
>>> +int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e,
>>> +		    struct tep_event *wakeup_event, struct tep_format_field *pid_field)
>>>    {
>>> -	struct plugin_sched_context *plugin_ctx =
>>> -		plugin_sched_context_handler;
>>> +	struct tep_record *record;
>>>    	unsigned long long val;
>>>    	int ret;
>>>    
>>> -	ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
>>> -				    record->data, &val);
>>> +	if (!wakeup_event || e->event_id != wakeup_event->id)
>>> +		return -1;
>>>    
>>> -	return ret ? : val;
>>> +	record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
>>> +	ret = tep_read_number_field(pid_field, record->data, &val);
>>> +	free_record(record);
>>> +
>>> +	if (ret)
>>> +		return -1;
>>> +
>>> +	return val;
>>> +}
>>> +
>>> +static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx,
>>> +				 struct kshark_context *kshark_ctx,
>>> +				 struct kshark_entry *e,
>>> +				 int pid)
>>> +{
>>> +	struct tep_event *wakeup_events[] = {
>>> +		plugin_ctx->sched_wakeup_event,
>>> +		plugin_ctx->sched_wakeup_new_event,
>>> +	};
>>> +	struct tep_format_field *wakeup_fields[] = {
>>> +		plugin_ctx->sched_wakeup_pid_field,
>>> +		plugin_ctx->sched_wakeup_new_pid_field,
>>> +	};
>>> +	int i, wakeup_pid = -1;
>>> +
>>> +	for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) {
>>> +		wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]);
>>> +		if (wakeup_pid >= 0)
>>> +			break;
>>> +	}
>>> +
>>> +	if (wakeup_pid >= 0 && wakeup_pid == pid)
>>> +		return true;
>>> +
>>> +	return false;
>>>    }
>>>    
>>>    /**
>>> @@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
>>>    				 int pid)
>>>    {
>>>    	struct plugin_sched_context *plugin_ctx;
>>> -	struct tep_record *record = NULL;
>>> -	int wakeup_pid = -1;
>>>    
>>>    	plugin_ctx = plugin_sched_context_handler;
>>>    	if (!plugin_ctx)
>>>    		return false;
>>>    
>>> -	if (plugin_ctx->sched_wakeup_event &&
>>> -	    e->event_id == plugin_ctx->sched_wakeup_event->id) {
>>> -		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
>>> -		wakeup_pid = plugin_get_rec_wakeup_pid(record);
>>> -	}
>>> -
>>> -	if (plugin_ctx->sched_wakeup_new_event &&
>>> -	    e->event_id == plugin_ctx->sched_wakeup_new_event->id) {
>>> -		record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL);
>>> -		wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
>>> -	}
>>> -
>>> -	free_record(record);
>>> -
>>> -	if (wakeup_pid >= 0 && wakeup_pid == pid)
>>> -		return true;
>>> -
>>> -	return false;
>>> +	return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid);
>>>    }
>>>   
>>
>> I see no point in defining "static bool wakeup_match_rec_pid()" and then
>> having "plugin_wakeup_match_rec_pid()" which is doing nothing but just
>> calling this static function.
> 
> It's because of the initialization of wakeup_events and wakeup_fields.
> 
> 	struct tep_event *wakeup_events[] = {
> 		plugin_ctx->sched_waking_event,
> 		plugin_ctx->sched_wakeup_event,
> 		plugin_ctx->sched_wakeup_new_event,
> 	};
> 	struct tep_format_field *wakeup_fields[] = {
> 		plugin_ctx->sched_waking_pid_field,
> 		plugin_ctx->sched_wakeup_pid_field,
> 		plugin_ctx->sched_wakeup_new_pid_field,
> 	};
> 
> Which are initialized by the values in plugin_ctx, but plugin_ctx is
> not initialized until after the static is set up:
> 
> 	plugin_ctx = plugin_sched_context_handler;
> 	if (!plugin_ctx)
> 		return false;
> 
> Thus to handle this case, I simply moved the code into a helper
> function, and had the main function just initialize plugin_ctx (return
> if NULL), and then call the helper function that can initialize the
> arrays on the stack.
> 
> Make sense?
> 

This is OK as well.

Thank you very much!
Yordan

> -- Steve
> 
> 
>>
>> Everything else in this patch series is OK.
>>
>> Thank you very much!
>> Yordan
>>
>>
>> Reviewed-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>>
>>
>>>    /**
>>>    
>