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

Commit Message

Steven Rostedt Feb. 11, 2019, 1:18 a.m. UTC

From: "Steven Rostedt (VMware)" <[e-mail]>

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) <[e-mail]>
---
 kernel-shark/src/plugins/sched_events.c | 106 +++++++++++++++---------
 1 file changed, 66 insertions(+), 40 deletions(-)

Comments

Yordan Karadzhov Feb. 11, 2019, 8:53 a.m. UTC | #1

On 11.02.19 г. 3:18 ч., Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <[e-mail]>
> 
> 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) <[e-mail]>
> ---
>   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 <[e-mail]>


>   /**
>

Steven Rostedt Feb. 11, 2019, 2:44 p.m. UTC | #2

On Mon, 11 Feb 2019 08:53:16 +0000
Yordan Karadzhov <[e-mail]> 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 <[e-mail]>
> 
> 
> >   /**
> >

Yordan Karadzhov Feb. 12, 2019, 5:09 p.m. UTC | #3

On 11.02.19 г. 16:44 ч., Steven Rostedt wrote:
> On Mon, 11 Feb 2019 08:53:16 +0000
> Yordan Karadzhov <[e-mail]> 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 <[e-mail]>
>>
>>
>>>    /**
>>>    
>

Patch

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);
 }
 
 /**