patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init
       [not found] <1514735641-8738-1-git-send-email-vipin.varghese@intel.com>
@ 2018-01-11 19:47 ` Vipin Varghese
  2018-01-26 15:44   ` Van Haaren, Harry
  2018-01-26 16:59   ` Thomas Monjalon
  0 siblings, 2 replies; 5+ messages in thread
From: Vipin Varghese @ 2018-01-11 19:47 UTC (permalink / raw)
  To: harry.van.haaren, dev; +Cc: stable, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init
  2018-01-11 19:47 ` [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init Vipin Varghese
@ 2018-01-26 15:44   ` Van Haaren, Harry
  2018-01-26 16:59   ` Thomas Monjalon
  1 sibling, 0 replies; 5+ messages in thread
From: Van Haaren, Harry @ 2018-01-26 15:44 UTC (permalink / raw)
  To: Varghese, Vipin, dev; +Cc: stable

> 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>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init
  2018-01-11 19:47 ` [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init Vipin Varghese
  2018-01-26 15:44   ` Van Haaren, Harry
@ 2018-01-26 16:59   ` Thomas Monjalon
  2018-01-26 17:15     ` Van Haaren, Harry
  1 sibling, 1 reply; 5+ messages in thread
From: Thomas Monjalon @ 2018-01-26 16:59 UTC (permalink / raw)
  To: Vipin Varghese, harry.van.haaren; +Cc: stable, dev

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init
  2018-01-26 16:59   ` Thomas Monjalon
@ 2018-01-26 17:15     ` Van Haaren, Harry
  2018-01-26 17:26       ` Thomas Monjalon
  0 siblings, 1 reply; 5+ messages in thread
From: Van Haaren, Harry @ 2018-01-26 17:15 UTC (permalink / raw)
  To: Thomas Monjalon, Varghese, Vipin; +Cc: stable, dev

> 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?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init
  2018-01-26 17:15     ` Van Haaren, Harry
@ 2018-01-26 17:26       ` Thomas Monjalon
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Monjalon @ 2018-01-26 17:26 UTC (permalink / raw)
  To: Van Haaren, Harry
  Cc: Varghese, Vipin, stable, dev, bruce.richardson,
	konstantin.ananyev, olivier.matz

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?

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-01-26 17:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1514735641-8738-1-git-send-email-vipin.varghese@intel.com>
2018-01-11 19:47 ` [dpdk-stable] [PATCH v2] app/procinfo: Fix memory leak by rte_service_init Vipin Varghese
2018-01-26 15:44   ` Van Haaren, Harry
2018-01-26 16:59   ` Thomas Monjalon
2018-01-26 17:15     ` Van Haaren, Harry
2018-01-26 17:26       ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).