[PATCH 0/3] Improvements and fixes for sched_switch plugin

[PATCH 0/3] Improvements and fixes for sched_switch plugin

[Yordan Karadzhov]

Yordan Karadzhov (3):
  kernel-shark: Define free_contex function for the sched_events plugin
  kernel-shark: Fix a memory leak in the sched_events plugin
  kernel-shark: sched_events plugin init with sched_switch events only

 kernel-shark/src/plugins/sched_events.c | 30 +++++++++++++++----------
 1 file changed, 18 insertions(+), 12 deletions(-)

-- 
2.19.1

[PATCH 1/3] kernel-shark: Define free_contex function for the sched_events plugin

[Yordan Karadzhov]

This static helper function makes the code of the plugin cleaner and
easier to read.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/plugins/sched_events.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index 68734d4..0d6de2d 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -39,6 +39,17 @@ static bool define_wakeup_event(struct tep_handle *tep, const char *wakeup_name,
 	return true;
 }
 
+static void plugin_free_context(struct plugin_sched_context *plugin_ctx)
+{
+	if (!plugin_ctx)
+		return;
+
+	tracecmd_filter_id_hash_free(plugin_ctx->second_pass_hash);
+	kshark_free_collection_list(plugin_ctx->collections);
+
+	free(plugin_ctx);
+}
+
 static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 {
 	struct plugin_sched_context *plugin_ctx;
@@ -339,10 +350,7 @@ static int plugin_sched_close(struct kshark_context *kshark_ctx)
 					plugin_sched_action,
 					plugin_draw);
 
-	tracecmd_filter_id_hash_free(plugin_ctx->second_pass_hash);
-
-	kshark_free_collection_list(plugin_ctx->collections);
-	free(plugin_ctx);
+	plugin_free_context(plugin_ctx);
 	plugin_sched_context_handler = NULL;
 
 	return 1;
-- 
2.19.1

Re: [PATCH 1/3] kernel-shark: Define free_contex function for the sched_events plugin

[Slavomir Kaslev]

On Tue, Mar 05, 2019 at 04:39:22PM +0200, Yordan Karadzhov wrote:
> This static helper function makes the code of the plugin cleaner and
> easier to read.
> 
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/plugins/sched_events.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
> index 68734d4..0d6de2d 100644
> --- a/kernel-shark/src/plugins/sched_events.c
> +++ b/kernel-shark/src/plugins/sched_events.c
> @@ -39,6 +39,17 @@ static bool define_wakeup_event(struct tep_handle *tep, const char *wakeup_name,
>  	return true;
>  }
>  
> +static void plugin_free_context(struct plugin_sched_context *plugin_ctx)
> +{
> +	if (!plugin_ctx)
> +		return;
> +
> +	tracecmd_filter_id_hash_free(plugin_ctx->second_pass_hash);
> +	kshark_free_collection_list(plugin_ctx->collections);
> +
> +	free(plugin_ctx);
> +}
> +
>  static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
>  {
>  	struct plugin_sched_context *plugin_ctx;
> @@ -339,10 +350,7 @@ static int plugin_sched_close(struct kshark_context *kshark_ctx)
>  					plugin_sched_action,
>  					plugin_draw);
>  
> -	tracecmd_filter_id_hash_free(plugin_ctx->second_pass_hash);
> -
> -	kshark_free_collection_list(plugin_ctx->collections);
> -	free(plugin_ctx);
> +	plugin_free_context(plugin_ctx);
>  	plugin_sched_context_handler = NULL;
>  
>  	return 1;

Nit: there's a typo in the commit message

>    kernel-shark: Define free_contex function for the sched_events plugin
                                     ^
:s/free_contex/plugin_free_context/g ?

Other than that it looks good to me.

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Cheers,

-- Slavi

Re: [PATCH 1/3] kernel-shark: Define free_contex function for the sched_events plugin

[Steven Rostedt]

On Tue, 5 Mar 2019 17:54:26 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:


> Nit: there's a typo in the commit message
> 
> >    kernel-shark: Define free_contex function for the sched_events plugin  
>                                      ^
> :s/free_contex/plugin_free_context/g ?

I caught that too, and fixed it.


> 
> Other than that it looks good to me.
> 
> Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Thanks Slavomir,

I'll add your review tag.

-- Steve

[PATCH 2/3] kernel-shark: Fix a memory leak in the sched_events plugin

[Yordan Karadzhov]

When the sched_events plugin fails to initialize not all the
memory allocated for the context of the plugin is freed properly.
The problem gets fixed by using the free_context helper function
defined in the previous patch.

Fixes: 4f392730e ("kernel-shark-qt: Make Sched event plugin use ...")
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/plugins/sched_events.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index 0d6de2d..fe13e6a 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -74,8 +74,12 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 
 	event = tep_find_event_by_name(plugin_ctx->pevent,
 				       "sched", "sched_switch");
-	if (!event)
+	if (!event) {
+		plugin_free_context(plugin_ctx);
+		plugin_sched_context_handler = NULL;
+
 		return false;
+	}
 
 	plugin_ctx->sched_switch_event = event;
 	plugin_ctx->sched_switch_next_field =
@@ -320,11 +324,8 @@ static int plugin_sched_init(struct kshark_context *kshark_ctx)
 {
 	struct plugin_sched_context *plugin_ctx;
 
-	if (!plugin_sched_init_context(kshark_ctx)) {
-		free(plugin_sched_context_handler);
-		plugin_sched_context_handler = NULL;
+	if (!plugin_sched_init_context(kshark_ctx))
 		return 0;
-	}
 
 	plugin_ctx = plugin_sched_context_handler;
 
-- 
2.19.1

Re: [PATCH 2/3] kernel-shark: Fix a memory leak in the sched_events plugin

[Slavomir Kaslev]

On Tue, Mar 05, 2019 at 04:39:23PM +0200, Yordan Karadzhov wrote:
> When the sched_events plugin fails to initialize not all the
> memory allocated for the context of the plugin is freed properly.
> The problem gets fixed by using the free_context helper function
> defined in the previous patch.
> 
> Fixes: 4f392730e ("kernel-shark-qt: Make Sched event plugin use ...")
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/plugins/sched_events.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
> index 0d6de2d..fe13e6a 100644
> --- a/kernel-shark/src/plugins/sched_events.c
> +++ b/kernel-shark/src/plugins/sched_events.c
> @@ -74,8 +74,12 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
>  
>  	event = tep_find_event_by_name(plugin_ctx->pevent,
>  				       "sched", "sched_switch");
> -	if (!event)
> +	if (!event) {
> +		plugin_free_context(plugin_ctx);
> +		plugin_sched_context_handler = NULL;
> +

Nit: drop the above empty line?

>  		return false;
> +	}
>  
>  	plugin_ctx->sched_switch_event = event;
>  	plugin_ctx->sched_switch_next_field =
> @@ -320,11 +324,8 @@ static int plugin_sched_init(struct kshark_context *kshark_ctx)
>  {
>  	struct plugin_sched_context *plugin_ctx;
>  
> -	if (!plugin_sched_init_context(kshark_ctx)) {
> -		free(plugin_sched_context_handler);
> -		plugin_sched_context_handler = NULL;
> +	if (!plugin_sched_init_context(kshark_ctx))
>  		return 0;
> -	}
>  
>  	plugin_ctx = plugin_sched_context_handler;

Looks good to me.

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

-- Slavi

Re: [PATCH 2/3] kernel-shark: Fix a memory leak in the sched_events plugin

[Steven Rostedt]

On Tue, 5 Mar 2019 17:56:28 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> On Tue, Mar 05, 2019 at 04:39:23PM +0200, Yordan Karadzhov wrote:
> > When the sched_events plugin fails to initialize not all the
> > memory allocated for the context of the plugin is freed properly.
> > The problem gets fixed by using the free_context helper function
> > defined in the previous patch.
> > 
> > Fixes: 4f392730e ("kernel-shark-qt: Make Sched event plugin use ...")
> > Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> > ---
> >  kernel-shark/src/plugins/sched_events.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
> > index 0d6de2d..fe13e6a 100644
> > --- a/kernel-shark/src/plugins/sched_events.c
> > +++ b/kernel-shark/src/plugins/sched_events.c
> > @@ -74,8 +74,12 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
> >  
> >  	event = tep_find_event_by_name(plugin_ctx->pevent,
> >  				       "sched", "sched_switch");
> > -	if (!event)
> > +	if (!event) {
> > +		plugin_free_context(plugin_ctx);
> > +		plugin_sched_context_handler = NULL;
> > +  
> 
> Nit: drop the above empty line?

Yeah, it would be better to do that. But for now, I'll take the patch
anyway. Perhaps clean it up later.

> 
> >  		return false;
> > +	}
> >  
> >  	plugin_ctx->sched_switch_event = event;
> >  	plugin_ctx->sched_switch_next_field =
> > @@ -320,11 +324,8 @@ static int plugin_sched_init(struct kshark_context *kshark_ctx)
> >  {
> >  	struct plugin_sched_context *plugin_ctx;
> >  
> > -	if (!plugin_sched_init_context(kshark_ctx)) {
> > -		free(plugin_sched_context_handler);
> > -		plugin_sched_context_handler = NULL;
> > +	if (!plugin_sched_init_context(kshark_ctx))
> >  		return 0;
> > -	}
> >  
> >  	plugin_ctx = plugin_sched_context_handler;  
> 
> Looks good to me.
> 
> Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Thanks Slavomir!

-- Steve

Re: [PATCH 2/3] kernel-shark: Fix a memory leak in the sched_events plugin

[Steven Rostedt]

On Tue,  5 Mar 2019 16:39:23 +0200
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> When the sched_events plugin fails to initialize not all the
> memory allocated for the context of the plugin is freed properly.
> The problem gets fixed by using the free_context helper function
> defined in the previous patch.
> 
> Fixes: 4f392730e ("kernel-shark-qt: Make Sched event plugin use ...")

I took the patch, but don't truncate the fixes line. It's fine to go
over 80 characters:

Fixes: 4f392730e ("kernel-shark-qt: Make Sched event plugin use its own data collections")

-- Steve

> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/plugins/sched_events.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
> index 0d6de2d..fe13e6a 100644
> --- a/kernel-shark/src/plugins/sched_events.c
> +++ b/kernel-shark/src/plugins/sched_events.c
> @@ -74,8 +74,12 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
>  
>  	event = tep_find_event_by_name(plugin_ctx->pevent,
>  				       "sched", "sched_switch");
> -	if (!event)
> +	if (!event) {
> +		plugin_free_context(plugin_ctx);
> +		plugin_sched_context_handler = NULL;
> +
>  		return false;
> +	}
>  
>  	plugin_ctx->sched_switch_event = event;
>  	plugin_ctx->sched_switch_next_field =
> @@ -320,11 +324,8 @@ static int plugin_sched_init(struct kshark_context *kshark_ctx)
>  {
>  	struct plugin_sched_context *plugin_ctx;
>  
> -	if (!plugin_sched_init_context(kshark_ctx)) {
> -		free(plugin_sched_context_handler);
> -		plugin_sched_context_handler = NULL;
> +	if (!plugin_sched_init_context(kshark_ctx))
>  		return 0;
> -	}
>  
>  	plugin_ctx = plugin_sched_context_handler;
>  

[PATCH 3/3] kernel-shark: sched_events plugin init with sched_switch events only

[Yordan Karadzhov]

The sched_events plugin should not be totally disabled when the
wakeup events are missing. The initialization of the plugin must
fail only if the trace.dat file contains no sched_switch events.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/plugins/sched_events.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index fe13e6a..c52fb29 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -104,9 +104,6 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
 					   &plugin_ctx->sched_waking_event,
 					   &plugin_ctx->sched_waking_pid_field);
 
-	if (!wakeup_found)
-		return false;
-
 	plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc();
 
 	return true;
-- 
2.19.1

Re: [PATCH 3/3] kernel-shark: sched_events plugin init with sched_switch events only

[Slavomir Kaslev]

On Tue, Mar 05, 2019 at 04:39:24PM +0200, Yordan Karadzhov wrote:
> The sched_events plugin should not be totally disabled when the
> wakeup events are missing. The initialization of the plugin must
> fail only if the trace.dat file contains no sched_switch events.
> 
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/plugins/sched_events.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
> index fe13e6a..c52fb29 100644
> --- a/kernel-shark/src/plugins/sched_events.c
> +++ b/kernel-shark/src/plugins/sched_events.c
> @@ -104,9 +104,6 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
>  					   &plugin_ctx->sched_waking_event,
>  					   &plugin_ctx->sched_waking_pid_field);
>  
> -	if (!wakeup_found)
> -		return false;
> -
>  	plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc();
>  
>  	return true;

Looks good to me.

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Thank you,

-- Slavi

Re: [PATCH 3/3] kernel-shark: sched_events plugin init with sched_switch events only

[Steven Rostedt]

On Tue, 5 Mar 2019 17:57:55 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> Looks good to me.
> 
> Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Thanks Yordan and Slavomir,

I applied the patches.

-- Steve

Re: [PATCH 3/3] kernel-shark: sched_events plugin init with sched_switch events only

[Steven Rostedt]

On Tue,  5 Mar 2019 16:39:24 +0200
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> The sched_events plugin should not be totally disabled when the
> wakeup events are missing. The initialization of the plugin must
> fail only if the trace.dat file contains no sched_switch events.
> 

I added:

Reported-by: Phil Auld <pauld@redhat.com>

-- Steve

> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/plugins/sched_events.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
> index fe13e6a..c52fb29 100644
> --- a/kernel-shark/src/plugins/sched_events.c
> +++ b/kernel-shark/src/plugins/sched_events.c
> @@ -104,9 +104,6 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx)
>  					   &plugin_ctx->sched_waking_event,
>  					   &plugin_ctx->sched_waking_pid_field);
>  
> -	if (!wakeup_found)
> -		return false;
> -
>  	plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc();
>  
>  	return true;