[dpdk-dev] [PATCH v1] [app/procinfo] fix memory leak - PCAP & service

[dpdk-dev] [PATCH v1] [app/procinfo] fix memory leak - PCAP & service

[Vipin Varghese]

When procinfo uses the PCAP PMD it is not detached. The library service
also makes of memory but never releases at exit of application. These leads
to memory leak, on multiple runs.

The patch add check for libpcap PMD check and detaches the same. The patch
also frees the service library memory too.

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

diff --git a/app/proc_info/main.c b/app/proc_info/main.c
index 64fbbd0..44d9af9 100644
--- a/app/proc_info/main.c
+++ b/app/proc_info/main.c
@@ -58,6 +58,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
@@ -689,5 +690,24 @@ static void collectd_resolve_cnt_type(char *cnt_type, size_t cnt_type_len,
 	if (enable_metrics)
 		metrics_display(RTE_METRICS_GLOBAL);
 
+	for (i = 0; i < nb_ports; i++) {
+		struct rte_eth_dev_info dev_info = {0};
+		char name[RTE_DEV_NAME_MAX_LEN] = {0};
+
+		rte_eth_dev_info_get(i, &dev_info);
+		if (strncmp(dev_info.driver_name, "net_pcap", 8) == 0) {
+			printf("port: %d driver_name: %s\n",
+				i, dev_info.driver_name);
+			rte_eth_dev_stop(i);
+			rte_eth_dev_close(i);
+
+			ret = rte_eth_dev_detach(i, name);
+			if (ret)
+				rte_panic("Failed to detach %s\n", name);
+		}
+	}
+
+	rte_service_deinit();
+
 	return 0;
 }
-- 
1.9.1

Re: [dpdk-dev] [PATCH v1] [app/procinfo] fix memory leak - PCAP & service

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vipin Varghese
> Sent: Sunday, December 31, 2017 3:54 PM
> To: dev@dpdk.org
> Cc: Hunt, David <david.hunt@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>; Varghese, Vipin <vipin.varghese@intel.com>
> Subject: [dpdk-dev] [PATCH v1] [app/procinfo] fix memory leak - PCAP &
> service
> 
> When procinfo uses the PCAP PMD it is not detached. The library service
> also makes of memory but never releases at exit of application. These leads
> to memory leak, on multiple runs.
> 
> The patch add check for libpcap PMD check and detaches the same. The patch
> also frees the service library memory too.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>

Hi,

There are two fixes in this patch, please split into two patches:
1) PCAP PMD detach
2) service library deinit()

For service library half of patch only, you can add:
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

For Ack of PCAP part, please CC PCAP maintainer.

Regards, -Harry

> ---
>  app/proc_info/main.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/app/proc_info/main.c b/app/proc_info/main.c
> index 64fbbd0..44d9af9 100644
> --- a/app/proc_info/main.c
> +++ b/app/proc_info/main.c
> @@ -58,6 +58,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
> @@ -689,5 +690,24 @@ static void collectd_resolve_cnt_type(char *cnt_type,
> size_t cnt_type_len,
>  	if (enable_metrics)
>  		metrics_display(RTE_METRICS_GLOBAL);
> 
> +	for (i = 0; i < nb_ports; i++) {
> +		struct rte_eth_dev_info dev_info = {0};
> +		char name[RTE_DEV_NAME_MAX_LEN] = {0};
> +
> +		rte_eth_dev_info_get(i, &dev_info);
> +		if (strncmp(dev_info.driver_name, "net_pcap", 8) == 0) {
> +			printf("port: %d driver_name: %s\n",
> +				i, dev_info.driver_name);
> +			rte_eth_dev_stop(i);
> +			rte_eth_dev_close(i);
> +
> +			ret = rte_eth_dev_detach(i, name);
> +			if (ret)
> +				rte_panic("Failed to detach %s\n", name);
> +		}
> +	}
> +
> +	rte_service_deinit();
> +
>  	return 0;
>  }
> --
> 1.9.1

[dpdk-dev] [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-dev] [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-dev] [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-dev] [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-dev] [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?