[dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init

[dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init

[Vipin Varghese]

When procinfo is run multiple times against primary application, it
consumes huge page memory by rte_service_init. Which is not released
at exit of application.

Invoking rte_service_finalize to real memory and prevent memory leak.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/proc_info/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/proc_info/main.c b/app/proc_info/main.c
index 94d53f5..1884e06 100644
--- a/app/proc_info/main.c
+++ b/app/proc_info/main.c
@@ -29,6 +29,7 @@
 #include <rte_branch_prediction.h>
 #include <rte_string_fns.h>
 #include <rte_metrics.h>
+#include <rte_service.h>
 
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64
@@ -660,5 +661,7 @@ static void collectd_resolve_cnt_type(char *cnt_type, size_t cnt_type_len,
 	if (enable_metrics)
 		metrics_display(RTE_METRICS_GLOBAL);
 
+	rte_service_finalize();
+
 	return 0;
 }
-- 
1.9.1

Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init

[Van Haaren, Harry]

> From: Varghese, Vipin
> Sent: Thursday, January 11, 2018 7:48 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 v2] app/procinfo: Fix memory leak by rte_service_init
> 
> When procinfo is run multiple times against primary application, it
> consumes huge page memory by rte_service_init. Which is not released
> at exit of application.
> 
> Invoking rte_service_finalize to real memory and prevent memory leak.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>

Thanks Vipin.

Note that this fixes a hugepages memory leak that would otherwise
occur when a secondary process initializes EAL and then quits.

Note that this patch depends on the patch adding rte_service_finalize()
http://dpdk.org/dev/patchwork/patch/34555/

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

Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init

[Thomas Monjalon]

11/01/2018 20:47, Vipin Varghese:
> When procinfo is run multiple times against primary application, it
> consumes huge page memory by rte_service_init. Which is not released
> at exit of application.
> 
> Invoking rte_service_finalize to real memory and prevent memory leak.

I don't think it is correct to call rte_service_finalize in applications,
while rte_service_init is called in EAL.

Maybe we need a new function in EAL.

Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init

[Van Haaren, Harry]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, January 26, 2018 5:00 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Cc: stable@dpdk.org; dev@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by
> rte_service_init
> 
> 11/01/2018 20:47, Vipin Varghese:
> > When procinfo is run multiple times against primary application, it
> > consumes huge page memory by rte_service_init. Which is not released
> > at exit of application.
> >
> > Invoking rte_service_finalize to real memory and prevent memory leak.
> 
> I don't think it is correct to call rte_service_finalize in applications,
> while rte_service_init is called in EAL.
> 
> Maybe we need a new function in EAL.

Yes correct - we need a rte_eal_deinit(), cleanup() or finalize() or something. This ties in with splitting EAL to be more modular on startup, and DPDK in general behaving more like a library and less like a single-monolith.

For the 18.02 timeframe, the simplest solution to solve the secondary process mem-leak issue than to merge into these applications, unfortunately. 

The only other option I see is to add an rte_eal_finalize() function, and hide this call behind it, however it is quite late to add such a function, and what do we do with cases like rte_panic(), rte_exit(), or system signals like SIGINT, SIGHUP etc? It seems too complicated to add "quickly" to me.

If there is technically a better solution viable in the given timeframe, I'm open to suggestions?

Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init

[Thomas Monjalon]

26/01/2018 18:15, Van Haaren, Harry:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 11/01/2018 20:47, Vipin Varghese:
> > > When procinfo is run multiple times against primary application, it
> > > consumes huge page memory by rte_service_init. Which is not released
> > > at exit of application.
> > >
> > > Invoking rte_service_finalize to real memory and prevent memory leak.
> > 
> > I don't think it is correct to call rte_service_finalize in applications,
> > while rte_service_init is called in EAL.
> > 
> > Maybe we need a new function in EAL.
> 
> Yes correct - we need a rte_eal_deinit(), cleanup() or finalize() or something. This ties in with splitting EAL to be more modular on startup, and DPDK in general behaving more like a library and less like a single-monolith.
> 
> For the 18.02 timeframe, the simplest solution to solve the secondary process mem-leak issue than to merge into these applications, unfortunately. 
> 
> The only other option I see is to add an rte_eal_finalize() function, and hide this call behind it, however it is quite late to add such a function, and what do we do with cases like rte_panic(), rte_exit(), or system signals like SIGINT, SIGHUP etc? It seems too complicated to add "quickly" to me.
> 
> If there is technically a better solution viable in the given timeframe, I'm open to suggestions?

I think it is better to keep the leak in 18.02,
and takes time to fix it properly in 18.05.

If you really think it is a major bug, we can try to expose a new
EAL function now and refine it in 18.05.

More opinions?