RE: [PATCH] app/testpmd: fix issue with memory leaks when quit testpmd

["Zhang, Ke1X" <ke1x.zhang@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Saturday, March 5, 2022 12:44 AM
> To: Zhang, Ke1X <ke1x.zhang@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Singh, Aman Deep <aman.deep.singh@intel.com>;
> Zhang, Yuying <yuying.zhang@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH] app/testpmd: fix issue with memory leaks when quit
> testpmd
> 
> On 3/1/2022 2:06 AM, Ke Zhang wrote:
> > when dpdk is compiled in ASan, there is a memory leaks after quit
> > testpmd if set mcast_addr, this patch fix this issue.
> >
> > Error info as following:
> > ERROR: LeakSanitizer: detected memory leaksDirect leak of
> >         192 byte(s)
> > 0 0x7f6a2e0aeffe in __interceptor_realloc
> > 	(/lib/x86_64-linux-gnu/libasan.so.5+0x10dffe)
> > 1 0x565361eb340f in mcast_addr_pool_extend
> > 	../app/test-pmd/config.c:5162
> > 2 0x565361eb3556 in mcast_addr_pool_append
> > 	../app/test-pmd/config.c:5180
> > 3 0x565361eb3aae in mcast_addr_add
> > 	../app/test-pmd/config.c:5243
> >
> > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > ---
> >   app/test-pmd/testpmd.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > fe2ce19f99..fa7f80fdf7 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2742,6 +2742,8 @@ start_port(portid_t pid)
> >   			continue;
> >   		}
> >
> > +		port->mc_addr_pool = NULL;
> > +
> 
> User can do consecutive start / stop / start / ..
> 
> Won't user expect the previous allocated and set MAC address to be valid
> after this?
> 
> What about to free the 'port->mc_addr_pool' in close() instead of stop()?
>

Thanks for your comments,  I checked the code in dpdk, when start, the resource associated with port
Will be allocated, when stop, the resource will be free from hardware and software struct.For example,
In the function ice_dev_stop(), the rx and tx resource will be free, and also in start, resource like tx,
 it will be allocated and request hardware to allocate the resource using ice_aqc_opc_add_txqs admQ
command, and the mc_addr_pool is a port resource. So what's your opinion about this ?
 
> >   		if (port->need_reconfig > 0) {
> >   			struct rte_eth_conf dev_conf;
> >   			int k;
> > @@ -3065,6 +3067,16 @@ stop_port(portid_t pid)
> >   		if (eth_dev_stop_mp(pi) != 0)
> >   			RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for
> port %u\n",
> >   				pi);
> > +		/*
> > +		 * free the pool of multicast addresses. If it is NULL,
> > +		 * it means there is no mc addr.Make sure the mc_addr_pool
> > +		 * is NULL at port init.
> > +		 */
> > +		if (port->mc_addr_pool != NULL) {
> > +			free(port->mc_addr_pool);
> > +			port->mc_addr_pool = NULL;
> > +		}
> > +		port->mc_addr_nb = 0;
> 
> will it work to use 'port->mc_addr_nb' to detect if the 'port->mc_addr_pool'
> pointer is valid
> 
> if (port->mc_addr_nb) {
>    free(port->mc_addr_pool);
>    port->mc_addr_pool = NULL;
>    port->mc_addr_nb = 0;
> }
> 

Yes , it is a better way using " if (port->mc_addr_nb) ", I will modify my code.

> >
> >   		if (port->port_status == RTE_PORT_HANDLING)
> >   			port->port_status = RTE_PORT_STOPPED;