diff mbox

linux-generic: odp_linux: fix memory leak

Message ID 1420032496-30645-1-git-send-email-mike.holmes@linaro.org
State Accepted
Headers show

Commit Message

Mike Holmes Dec. 31, 2014, 1:28 p.m. UTC 
Store the start_args so that they can be freed on join
Fixes numerous leaks reported by valgrind

Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
---
 helper/include/odph_linux.h        |  9 +++++++++
 platform/linux-generic/odp_linux.c | 21 +++++++--------------
 2 files changed, 16 insertions(+), 14 deletions(-)

Comments

Mike Holmes Jan. 9, 2015, 6:37 p.m. UTC | #1 
This appears to be very significant to linux-generic at least since it
fixes all the valgrind issues - review for 0.8.0 ?

On 31 December 2014 at 08:28, Mike Holmes <mike.holmes@linaro.org> wrote:

> Store the start_args so that they can be freed on join
> Fixes numerous leaks reported by valgrind
>
> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> ---
>  helper/include/odph_linux.h        |  9 +++++++++
>  platform/linux-generic/odp_linux.c | 21 +++++++--------------
>  2 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/helper/include/odph_linux.h b/helper/include/odph_linux.h
> index 8671dc0..1b32b26 100644
> --- a/helper/include/odph_linux.h
> +++ b/helper/include/odph_linux.h
> @@ -26,11 +26,20 @@ extern "C" {
>  #include <pthread.h>
>  #include <sys/types.h>
>
> +/** The thread starting arguments */
> +typedef struct {
> +       void *(*start_routine) (void *); /**< The function to run */
> +       void *arg; /**< The functions arguemnts */
> +
> +} odp_start_args_t;
> +
>  /** Linux pthread state information */
>  typedef struct {
>         pthread_t      thread; /**< Pthread ID */
>         pthread_attr_t attr;   /**< Pthread attributes */
>         int            core;   /**< Core ID */
> +       /** Saved starting args for join to later free */
> +       odp_start_args_t *start_args;
>  } odph_linux_pthread_t;
>
>
> diff --git a/platform/linux-generic/odp_linux.c
> b/platform/linux-generic/odp_linux.c
> index 229d24e..96d4d5d 100644
> --- a/platform/linux-generic/odp_linux.c
> +++ b/platform/linux-generic/odp_linux.c
> @@ -26,13 +26,6 @@
>  #include <odp_debug_internal.h>
>
>
> -typedef struct {
> -       void *(*start_routine) (void *);
> -       void *arg;
> -
> -} odp_start_args_t;
> -
> -
>  static void *odp_run_start_routine(void *arg)
>  {
>         odp_start_args_t *start_args = arg;
> @@ -61,7 +54,6 @@ void odph_linux_pthread_create(odph_linux_pthread_t
> *thread_tbl, int num,
>  {
>         int i;
>         cpu_set_t cpu_set;
> -       odp_start_args_t *start_args;
>         int core_count;
>         int cpu;
>
> @@ -83,16 +75,15 @@ void odph_linux_pthread_create(odph_linux_pthread_t
> *thread_tbl, int num,
>                 pthread_attr_setaffinity_np(&thread_tbl[i].attr,
>                                             sizeof(cpu_set_t), &cpu_set);
>
> -               start_args = malloc(sizeof(odp_start_args_t));
> -               if (start_args == NULL)
> +               thread_tbl[i].start_args =
> malloc(sizeof(odp_start_args_t));
> +               if (thread_tbl[i].start_args == NULL)
>                         ODP_ABORT("Malloc failed");
>
> -               memset(start_args, 0, sizeof(odp_start_args_t));
> -               start_args->start_routine = start_routine;
> -               start_args->arg           = arg;
> +               thread_tbl[i].start_args->start_routine = start_routine;
> +               thread_tbl[i].start_args->arg           = arg;
>
>                 pthread_create(&thread_tbl[i].thread, &thread_tbl[i].attr,
> -                              odp_run_start_routine, start_args);
> +                              odp_run_start_routine,
> thread_tbl[i].start_args);
>         }
>  }
>
> @@ -104,7 +95,9 @@ void odph_linux_pthread_join(odph_linux_pthread_t
> *thread_tbl, int num)
>         for (i = 0; i < num; i++) {
>                 /* Wait thread to exit */
>                 pthread_join(thread_tbl[i].thread, NULL);
> +               free(thread_tbl[i].start_args);
>         }
> +
>  }
>
>
> --
> 2.1.0
>
>
Maxim Uvarov Jan. 12, 2015, 5:16 p.m. UTC | #2 
Reviewed and applied.

Maxim.

On 01/09/2015 09:37 PM, Mike Holmes wrote:
> This appears to be very significant to linux-generic at least since it 
> fixes all the valgrind issues - review for 0.8.0 ?
>
> On 31 December 2014 at 08:28, Mike Holmes <mike.holmes@linaro.org 
> <mailto:mike.holmes@linaro.org>> wrote:
>
>     Store the start_args so that they can be freed on join
>     Fixes numerous leaks reported by valgrind
>
>     Signed-off-by: Mike Holmes <mike.holmes@linaro.org
>     <mailto:mike.holmes@linaro.org>>
>     ---
>      helper/include/odph_linux.h        |  9 +++++++++
>      platform/linux-generic/odp_linux.c | 21 +++++++--------------
>      2 files changed, 16 insertions(+), 14 deletions(-)
>
>     diff --git a/helper/include/odph_linux.h b/helper/include/odph_linux.h
>     index 8671dc0..1b32b26 100644
>     --- a/helper/include/odph_linux.h
>     +++ b/helper/include/odph_linux.h
>     @@ -26,11 +26,20 @@ extern "C" {
>      #include <pthread.h>
>      #include <sys/types.h>
>
>     +/** The thread starting arguments */
>     +typedef struct {
>     +       void *(*start_routine) (void *); /**< The function to run */
>     +       void *arg; /**< The functions arguemnts */
>     +
>     +} odp_start_args_t;
>     +
>      /** Linux pthread state information */
>      typedef struct {
>             pthread_t      thread; /**< Pthread ID */
>             pthread_attr_t attr;   /**< Pthread attributes */
>             int            core;   /**< Core ID */
>     +       /** Saved starting args for join to later free */
>     +       odp_start_args_t *start_args;
>      } odph_linux_pthread_t;
>
>
>     diff --git a/platform/linux-generic/odp_linux.c
>     b/platform/linux-generic/odp_linux.c
>     index 229d24e..96d4d5d 100644
>     --- a/platform/linux-generic/odp_linux.c
>     +++ b/platform/linux-generic/odp_linux.c
>     @@ -26,13 +26,6 @@
>      #include <odp_debug_internal.h>
>
>
>     -typedef struct {
>     -       void *(*start_routine) (void *);
>     -       void *arg;
>     -
>     -} odp_start_args_t;
>     -
>     -
>      static void *odp_run_start_routine(void *arg)
>      {
>             odp_start_args_t *start_args = arg;
>     @@ -61,7 +54,6 @@ void
>     odph_linux_pthread_create(odph_linux_pthread_t *thread_tbl, int num,
>      {
>             int i;
>             cpu_set_t cpu_set;
>     -       odp_start_args_t *start_args;
>             int core_count;
>             int cpu;
>
>     @@ -83,16 +75,15 @@ void
>     odph_linux_pthread_create(odph_linux_pthread_t *thread_tbl, int num,
>     pthread_attr_setaffinity_np(&thread_tbl[i].attr,
>     sizeof(cpu_set_t), &cpu_set);
>
>     -               start_args = malloc(sizeof(odp_start_args_t));
>     -               if (start_args == NULL)
>     +               thread_tbl[i].start_args =
>     malloc(sizeof(odp_start_args_t));
>     +               if (thread_tbl[i].start_args == NULL)
>                             ODP_ABORT("Malloc failed");
>
>     -               memset(start_args, 0, sizeof(odp_start_args_t));
>     -               start_args->start_routine = start_routine;
>     -               start_args->arg           = arg;
>     +               thread_tbl[i].start_args->start_routine =
>     start_routine;
>     +               thread_tbl[i].start_args->arg           = arg;
>
>                     pthread_create(&thread_tbl[i].thread,
>     &thread_tbl[i].attr,
>     -                              odp_run_start_routine, start_args);
>     +                              odp_run_start_routine,
>     thread_tbl[i].start_args);
>             }
>      }
>
>     @@ -104,7 +95,9 @@ void
>     odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num)
>             for (i = 0; i < num; i++) {
>                     /* Wait thread to exit */
>                     pthread_join(thread_tbl[i].thread, NULL);
>     +               free(thread_tbl[i].start_args);
>             }
>     +
>      }
>
>
>     --
>     2.1.0
>
>
>
>
> -- 
> *Mike Holmes*
> Linaro  Sr Technical Manager
> LNG - ODP
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/helper/include/odph_linux.h b/helper/include/odph_linux.h
index 8671dc0..1b32b26 100644
--- a/helper/include/odph_linux.h
+++ b/helper/include/odph_linux.h
@@ -26,11 +26,20 @@  extern "C" {
 #include <pthread.h>
 #include <sys/types.h>
 
+/** The thread starting arguments */
+typedef struct {
+	void *(*start_routine) (void *); /**< The function to run */
+	void *arg; /**< The functions arguemnts */
+
+} odp_start_args_t;
+
 /** Linux pthread state information */
 typedef struct {
 	pthread_t      thread; /**< Pthread ID */
 	pthread_attr_t attr;   /**< Pthread attributes */
 	int            core;   /**< Core ID */
+	/** Saved starting args for join to later free */
+	odp_start_args_t *start_args;
 } odph_linux_pthread_t;
 
 
diff --git a/platform/linux-generic/odp_linux.c b/platform/linux-generic/odp_linux.c
index 229d24e..96d4d5d 100644
--- a/platform/linux-generic/odp_linux.c
+++ b/platform/linux-generic/odp_linux.c
@@ -26,13 +26,6 @@ 
 #include <odp_debug_internal.h>
 
 
-typedef struct {
-	void *(*start_routine) (void *);
-	void *arg;
-
-} odp_start_args_t;
-
-
 static void *odp_run_start_routine(void *arg)
 {
 	odp_start_args_t *start_args = arg;
@@ -61,7 +54,6 @@  void odph_linux_pthread_create(odph_linux_pthread_t *thread_tbl, int num,
 {
 	int i;
 	cpu_set_t cpu_set;
-	odp_start_args_t *start_args;
 	int core_count;
 	int cpu;
 
@@ -83,16 +75,15 @@  void odph_linux_pthread_create(odph_linux_pthread_t *thread_tbl, int num,
 		pthread_attr_setaffinity_np(&thread_tbl[i].attr,
 					    sizeof(cpu_set_t), &cpu_set);
 
-		start_args = malloc(sizeof(odp_start_args_t));
-		if (start_args == NULL)
+		thread_tbl[i].start_args = malloc(sizeof(odp_start_args_t));
+		if (thread_tbl[i].start_args == NULL)
 			ODP_ABORT("Malloc failed");
 
-		memset(start_args, 0, sizeof(odp_start_args_t));
-		start_args->start_routine = start_routine;
-		start_args->arg           = arg;
+		thread_tbl[i].start_args->start_routine = start_routine;
+		thread_tbl[i].start_args->arg           = arg;
 
 		pthread_create(&thread_tbl[i].thread, &thread_tbl[i].attr,
-			       odp_run_start_routine, start_args);
+			       odp_run_start_routine, thread_tbl[i].start_args);
 	}
 }
 
@@ -104,7 +95,9 @@  void odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num)
 	for (i = 0; i < num; i++) {
 		/* Wait thread to exit */
 		pthread_join(thread_tbl[i].thread, NULL);
+		free(thread_tbl[i].start_args);
 	}
+
 }