[dpdk-dev] [PATCH v1] service: fix memory leak by rte_service_init

[dpdk-dev] [PATCH v1] service: fix memory leak by rte_service_init

[Vipin Varghese]

This patch fixes the memory leak created by rte_service_init, when
run from secondary application. Running secondary application which
shares the huge page memory from primary multiple times causes memory
to be initialized but not free when application exit.

The rte_service_deinit check if the service is initialized. If yes, it
frees up rte_services & lcore_states. The API has to be called at end of
application run.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 lib/librte_eal/common/include/rte_service.h |  9 +++++++++
 lib/librte_eal/common/rte_service.c         | 14 ++++++++++++++
 lib/librte_eal/rte_eal_version.map          |  1 +
 3 files changed, 24 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 9272440..ad2649e 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -420,6 +420,15 @@ int32_t rte_service_run_iter_on_app_lcore(uint32_t id,
  */
 int32_t rte_service_dump(FILE *f, uint32_t id);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Fress all memory that is internally used.
+ */
+
+void rte_service_deinit(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index ae97e6b..017bc67 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -98,6 +98,20 @@ struct core_state {
 static struct core_state *lcore_states;
 static uint32_t rte_service_library_initialized;
 
+void rte_service_deinit(void)
+{
+	if (rte_service_library_initialized) {
+		if (rte_services)
+			rte_free(rte_services);
+		if (lcore_states)
+			rte_free(lcore_states);
+
+		rte_service_library_initialized = 0;
+	}
+	return;
+}
+
+
 int32_t rte_service_init(void)
 {
 	if (rte_service_library_initialized) {
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index f4f46c1..0f14409 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -234,5 +234,6 @@ EXPERIMENTAL {
 	rte_service_set_runstate_mapped_check;
 	rte_service_set_stats_enable;
 	rte_service_start_with_defaults;
+	rte_service_deinit;
 
 } DPDK_17.11;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v1] service: fix memory leak by rte_service_init

[Van Haaren, Harry]

> From: Varghese, Vipin
> Sent: Sunday, December 31, 2017 2:46 PM
> To: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Varghese, Vipin
> <vipin.varghese@intel.com>
> Subject: [PATCH v1] service: fix memory leak by rte_service_init
> 
> This patch fixes the memory leak created by rte_service_init, when
> run from secondary application. Running secondary application which
> shares the huge page memory from primary multiple times causes memory
> to be initialized but not free when application exit.
> 
> The rte_service_deinit check if the service is initialized. If yes, it
> frees up rte_services & lcore_states. The API has to be called at end of
> application run.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>

<snip>

> +++ b/lib/librte_eal/common/rte_service.c
> @@ -98,6 +98,20 @@ struct core_state {
>  static struct core_state *lcore_states;
>  static uint32_t rte_service_library_initialized;
> 
> +void rte_service_deinit(void)
> +{
> +	if (rte_service_library_initialized) {
> +		if (rte_services)
> +			rte_free(rte_services);
> +		if (lcore_states)
> +			rte_free(lcore_states);
> +
> +		rte_service_library_initialized = 0;
> +	}
> +	return;
> +}

No return required from void functions

> +
> +
>  int32_t rte_service_init(void)
>  {
>  	if (rte_service_library_initialized) {
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index f4f46c1..0f14409 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -234,5 +234,6 @@ EXPERIMENTAL {
>  	rte_service_set_runstate_mapped_check;
>  	rte_service_set_stats_enable;
>  	rte_service_start_with_defaults;
> +	rte_service_deinit;

Alphabetical ordering in the .map files (vim has a nice feature; visual select service functions  then  :sort )

With above changes;
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [dpdk-dev] [PATCH v1] service: fix memory leak by rte_service_init

[Bruce Richardson]

On Mon, Jan 08, 2018 at 10:17:14AM +0000, Van Haaren, Harry wrote:
> > From: Varghese, Vipin
> > Sent: Sunday, December 31, 2017 2:46 PM
> > To: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Varghese, Vipin
> > <vipin.varghese@intel.com>
> > Subject: [PATCH v1] service: fix memory leak by rte_service_init
> > 
> > This patch fixes the memory leak created by rte_service_init, when
> > run from secondary application. Running secondary application which
> > shares the huge page memory from primary multiple times causes memory
> > to be initialized but not free when application exit.
> > 
> > The rte_service_deinit check if the service is initialized. If yes, it
> > frees up rte_services & lcore_states. The API has to be called at end of
> > application run.
> > 
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> 
> <snip>
> 
> > +++ b/lib/librte_eal/common/rte_service.c
> > @@ -98,6 +98,20 @@ struct core_state {
> >  static struct core_state *lcore_states;
> >  static uint32_t rte_service_library_initialized;
> > 
> > +void rte_service_deinit(void)
> > +{
> > +	if (rte_service_library_initialized) {
> > +		if (rte_services)
> > +			rte_free(rte_services);
> > +		if (lcore_states)
> > +			rte_free(lcore_states);
> > +
> > +		rte_service_library_initialized = 0;
> > +	}
> > +	return;
> > +}
> 
> No return required from void functions
> 
> > +
> > +
> >  int32_t rte_service_init(void)
> >  {
> >  	if (rte_service_library_initialized) {
> > diff --git a/lib/librte_eal/rte_eal_version.map
> > b/lib/librte_eal/rte_eal_version.map
> > index f4f46c1..0f14409 100644
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -234,5 +234,6 @@ EXPERIMENTAL {
> >  	rte_service_set_runstate_mapped_check;
> >  	rte_service_set_stats_enable;
> >  	rte_service_start_with_defaults;
> > +	rte_service_deinit;
> 
> Alphabetical ordering in the .map files (vim has a nice feature; visual select service functions  then  :sort )
> 
> With above changes;
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Is the opposite of init not "uninit" rather than "deinit"?
Alternatively, "finalize" could be used as the opposite of "initialize",
which would be close to ".init" and ".fini" as used in ELF files.

/Bruce

[dpdk-dev] [PATCH v2] service: fix memory leak by rte_service_init

[Vipin Varghese]

This patch fixes the memory leak created by rte_service_init. When
secondary application which shares the huge page from primary, is
executed multiple times memory is initialized but not freed on exit

The rte_service_finalize checks if the service is initialized. If yes,
it frees up rte_services & lcore_states. The API has to be called at
end of application run.

renamed the function from rte_service_deinit to rte_service_finalize.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 lib/librte_eal/common/include/rte_service.h | 14 ++++++++++++++
 lib/librte_eal/common/rte_service.c         | 13 +++++++++++++
 lib/librte_eal/rte_eal_version.map          |  1 +
 3 files changed, 28 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 5a76383..45a8035 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -392,6 +392,20 @@ int32_t rte_service_run_iter_on_app_lcore(uint32_t id,
  */
 int32_t rte_service_dump(FILE *f, uint32_t id);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Finalize the service library.
+ *
+ * The service library, which is been initialized occupies DPDK memory.
+ * This routine cleans up the memory. It should be invoked prior to process
+ * termination.
+ *
+ * @retval None
+ */
+void rte_service_finalize(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 372d0bb..a848232 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -108,6 +108,19 @@ int32_t rte_service_init(void)
 	return 0;
 }
 
+void rte_service_finalize(void)
+{
+	if (rte_service_library_initialized) {
+		if (rte_services)
+			rte_free(rte_services);
+		if (lcore_states)
+			rte_free(lcore_states);
+
+		rte_service_library_initialized = 0;
+	}
+}
+
+
 /* returns 1 if service is registered and has not been unregistered
  * Returns 0 if service never registered, or has been unregistered
  */
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index f4f46c1..c73a72f 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -234,5 +234,6 @@ EXPERIMENTAL {
 	rte_service_set_runstate_mapped_check;
 	rte_service_set_stats_enable;
 	rte_service_start_with_defaults;
+	rte_service_finalize;
 
 } DPDK_17.11;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2] service: fix memory leak by rte_service_init

[Thomas Monjalon]

11/01/2018 19:20, Vipin Varghese:
> This patch fixes the memory leak created by rte_service_init. When
> secondary application which shares the huge page from primary, is
> executed multiple times memory is initialized but not freed on exit
> 
> The rte_service_finalize checks if the service is initialized. If yes,
> it frees up rte_services & lcore_states. The API has to be called at
> end of application run.
> 
> renamed the function from rte_service_deinit to rte_service_finalize.

This is a changelog. It should appear below ---

> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>

Why not keeping previous ack?

Your patches have been set to "Not Applicable" in patchwork.
What happened?

Re: [dpdk-dev] [PATCH v2] service: fix memory leak by rte_service_init

[Van Haaren, Harry]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, January 25, 2018 10:11 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>
> Cc: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] service: fix memory leak by
> rte_service_init
> 
> 11/01/2018 19:20, Vipin Varghese:
> > This patch fixes the memory leak created by rte_service_init. When
> > secondary application which shares the huge page from primary, is
> > executed multiple times memory is initialized but not freed on exit
> >
> > The rte_service_finalize checks if the service is initialized. If yes,
> > it frees up rte_services & lcore_states. The API has to be called at
> > end of application run.
> >
> > renamed the function from rte_service_deinit to rte_service_finalize.
> 
> This is a changelog. It should appear below ---
> 
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> 
> Why not keeping previous ack?
> 
> Your patches have been set to "Not Applicable" in patchwork.
> What happened?


There was some confusion in my review-comments, and Vipin and I discussed
the best was to rework - I suggested marking the current patches as Not Applicable.

Is there a better state to put the patches in Patchwork if we don't want commiters to look at them?
Or would an email to the thread stating V+1 in progress be better?

A new version will hit the ML soon.

Re: [dpdk-dev] [PATCH v2] service: fix memory leak by rte_service_init

[Thomas Monjalon]

26/01/2018 11:10, Van Haaren, Harry:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Your patches have been set to "Not Applicable" in patchwork.
> > What happened?
> 
> There was some confusion in my review-comments, and Vipin and I discussed
> the best was to rework - I suggested marking the current patches as Not Applicable.
> 
> Is there a better state to put the patches in Patchwork if we don't want commiters to look at them?
> Or would an email to the thread stating V+1 in progress be better?

Yes, an email is always good to notify about the status.
In patchwork, you can use the state "Changes requested".
"Not Applicable" is used for patches which were never intended
to be applicable.

> A new version will hit the ML soon.

[dpdk-dev] [PATCH v3] service: fix memory leak by rte_service_init

[Vipin Varghese]

The rte_service_finalize routine checks if service is initialized
or not. If yes; releases internal memory for services and lcore
states are freed. This routine is to be invoked at end of application
termination.

Fixes: 21698354c832 ("service: introduce service cores concept")
Cc: harry.van.haaren@intel.com

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V2 Changes:
- renamed routine to finalize (Bruce)
- improved code flow (Harry)

V3 Changes:
- fix order of function in map file
---
 lib/librte_eal/common/include/rte_service.h | 11 +++++++++++
 lib/librte_eal/common/rte_service.c         | 14 ++++++++++++++
 lib/librte_eal/rte_eal_version.map          |  1 +
 3 files changed, 26 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 02b1512..5e3e3a6 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -429,6 +429,17 @@ int32_t rte_service_attr_get(uint32_t id, uint32_t attr_id,
  */
 int32_t rte_service_attr_reset_all(uint32_t id);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Free up the memory that has been initialized. This routine
+ * is to be invoked prior to process termination.
+ *
+ * @retval None
+ */
+void rte_service_finalize(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index b40c3d9..bcd644a 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -114,6 +114,20 @@ int32_t rte_service_init(void)
 	return -ENOMEM;
 }
 
+void rte_service_finalize(void)
+{
+	if (!rte_service_library_initialized)
+		return;
+
+	if (rte_services)
+		rte_free(rte_services);
+
+	if (lcore_states)
+		rte_free(lcore_states);
+
+	rte_service_library_initialized = 0;
+}
+
 /* returns 1 if service is registered and has not been unregistered
  * Returns 0 if service never registered, or has been unregistered
  */
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 7088b72..1a8b1b5 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -223,6 +223,7 @@ EXPERIMENTAL {
 	rte_service_component_unregister;
 	rte_service_component_runstate_set;
 	rte_service_dump;
+	rte_service_finalize;
 	rte_service_get_by_id;
 	rte_service_get_by_name;
 	rte_service_get_count;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3] service: fix memory leak by rte_service_init

[Van Haaren, Harry]

> From: Varghese, Vipin
> Sent: Friday, January 26, 2018 8:56 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Varghese, Vipin <vipin.varghese@intel.com>
> Subject: [PATCH v3] service: fix memory leak by rte_service_init
> 
> The rte_service_finalize routine checks if service is initialized
> or not. If yes; releases internal memory for services and lcore
> states are freed. This routine is to be invoked at end of application
> termination.
> 
> Fixes: 21698354c832 ("service: introduce service cores concept")
> Cc: harry.van.haaren@intel.com
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>


Thanks Vipin!

@Stable - this patches allows for fixes secondary processes that
init and quit often, to avoid leaking hugepages memory. As such,
I think it is valuable to include in stable.

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>


> V2 Changes:
> - renamed routine to finalize (Bruce)
> - improved code flow (Harry)
> 
> V3 Changes:
> - fix order of function in map file

Re: [dpdk-dev] [PATCH v3] service: fix memory leak by rte_service_init

[Thomas Monjalon]

26/01/2018 16:20, Van Haaren, Harry:
> > From: Varghese, Vipin
> > Sent: Friday, January 26, 2018 8:56 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Varghese, Vipin <vipin.varghese@intel.com>
> > Subject: [PATCH v3] service: fix memory leak by rte_service_init
> > 
> > The rte_service_finalize routine checks if service is initialized
> > or not. If yes; releases internal memory for services and lcore
> > states are freed. This routine is to be invoked at end of application
> > termination.
> > 
> > Fixes: 21698354c832 ("service: introduce service cores concept")
> > Cc: harry.van.haaren@intel.com
> > 
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> 
> 
> Thanks Vipin!
> 
> @Stable - this patches allows for fixes secondary processes that
> init and quit often, to avoid leaking hugepages memory. As such,
> I think it is valuable to include in stable.
> 
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Applied, thanks