[PATCH] af_packet: make the device detachable

[PATCH] af_packet: make the device detachable

[Wojciech Zmuda]

Fix memory leak when detaching virtual device. Set dev_flags to
RTE_ETH_DEV_DETACHABLE and implement pmd_af_packet_drv.uninit method.
Copy device name to ethdev->data to make it compatibile with
rte_eth_dev_allocated().

Signed-off-by: Wojciech Zmuda <woz@semihalf.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 767f36b..7ef65ff 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -667,11 +667,13 @@ rte_pmd_init_internals(const char *name,
 	data->nb_tx_queues = (uint16_t)nb_queues;
 	data->dev_link = pmd_link;
 	data->mac_addrs = &(*internals)->eth_addr;
+	strncpy(data->name,
+		(*eth_dev)->data->name, strlen((*eth_dev)->data->name));
 
 	(*eth_dev)->data = data;
 	(*eth_dev)->dev_ops = &ops;
 	(*eth_dev)->driver = NULL;
-	(*eth_dev)->data->dev_flags = 0;
+	(*eth_dev)->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
 	(*eth_dev)->data->drv_name = drivername;
 	(*eth_dev)->data->kdrv = RTE_KDRV_NONE;
 	(*eth_dev)->data->numa_node = numa_node;
@@ -836,10 +838,35 @@ exit:
 	return ret;
 }
 
+static int
+rte_pmd_af_packet_devuninit(const char *name)
+{
+	struct rte_eth_dev *eth_dev = NULL;
+
+	RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
+			rte_socket_id());
+
+	if (name == NULL)
+		return -1;
+
+	/* reserve an ethdev entry */
+	eth_dev = rte_eth_dev_allocated(name);
+	if (eth_dev == NULL)
+		return -1;
+
+	rte_free(eth_dev->data->dev_private);
+	rte_free(eth_dev->data);
+
+	rte_eth_dev_release_port(eth_dev);
+
+	return 0;
+}
+
 static struct rte_driver pmd_af_packet_drv = {
 	.name = "eth_af_packet",
 	.type = PMD_VDEV,
 	.init = rte_pmd_af_packet_devinit,
+	.uninit = rte_pmd_af_packet_devuninit,
 };
 
 PMD_REGISTER_DRIVER(pmd_af_packet_drv);
-- 
1.9.1

Re: [PATCH] af_packet: make the device detachable

[Iremonger, Bernard]

Hi Wojciech,


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wojciech Zmuda
> Sent: Tuesday, January 5, 2016 2:05 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] af_packet: make the device detachable
> 
> Fix memory leak when detaching virtual device. Set dev_flags to
> RTE_ETH_DEV_DETACHABLE and implement pmd_af_packet_drv.uninit
> method.
> Copy device name to ethdev->data to make it compatibile with

Typo, compatibile should be compatible.

> rte_eth_dev_allocated().
> 
> Signed-off-by: Wojciech Zmuda <woz@semihalf.com>
> ---
>  drivers/net/af_packet/rte_eth_af_packet.c | 29
> ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
> b/drivers/net/af_packet/rte_eth_af_packet.c
> index 767f36b..7ef65ff 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -667,11 +667,13 @@ rte_pmd_init_internals(const char *name,
>  	data->nb_tx_queues = (uint16_t)nb_queues;
>  	data->dev_link = pmd_link;
>  	data->mac_addrs = &(*internals)->eth_addr;
> +	strncpy(data->name,
> +		(*eth_dev)->data->name, strlen((*eth_dev)->data-
> >name));
> 
>  	(*eth_dev)->data = data;
>  	(*eth_dev)->dev_ops = &ops;
>  	(*eth_dev)->driver = NULL;
> -	(*eth_dev)->data->dev_flags = 0;
> +	(*eth_dev)->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
>  	(*eth_dev)->data->drv_name = drivername;
>  	(*eth_dev)->data->kdrv = RTE_KDRV_NONE;
>  	(*eth_dev)->data->numa_node = numa_node; @@ -836,10 +838,35
> @@ exit:
>  	return ret;
>  }
> 
> +static int
> +rte_pmd_af_packet_devuninit(const char *name) {
> +	struct rte_eth_dev *eth_dev = NULL;
> +
> +	RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket
> %u\n",
> +			rte_socket_id());
> +
> +	if (name == NULL)
> +		return -1;
> +
> +	/* reserve an ethdev entry */

Above comment should be 	/* find the ethdev entry */

> +	eth_dev = rte_eth_dev_allocated(name);
> +	if (eth_dev == NULL)
> +		return -1;
> +

In rte_pmd_init_internals() memory is allocated for rx and tx queues this should also be freed.

Line 606: rx_queue->rd = rte_zmalloc_socket(name, rdsize, 0, numa_node);
Line 620:tx_queue->rd = rte_zmalloc_socket(name, rdsize, 0, numa_node);

	rte_free((*internals)->rx_queue[q].rd);
	rte_free((*internals)->tx_queue[q].rd);

> +	rte_free(eth_dev->data->dev_private);
> +	rte_free(eth_dev->data);
> +
> +	rte_eth_dev_release_port(eth_dev);
> +
> +	return 0;
> +}
> +
>  static struct rte_driver pmd_af_packet_drv = {
>  	.name = "eth_af_packet",
>  	.type = PMD_VDEV,
>  	.init = rte_pmd_af_packet_devinit,
> +	.uninit = rte_pmd_af_packet_devuninit,
>  };
> 
>  PMD_REGISTER_DRIVER(pmd_af_packet_drv);
> --
> 1.9.1

Re: [PATCH] af_packet: make the device detachable

[Iremonger, Bernard]

 Hi Wojciech

<snip>

> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wojciech Zmuda
> > Sent: Tuesday, January 5, 2016 2:05 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] af_packet: make the device detachable
> >
> > Fix memory leak when detaching virtual device. Set dev_flags to
> > RTE_ETH_DEV_DETACHABLE and implement pmd_af_packet_drv.uninit
> method.
> > Copy device name to ethdev->data to make it compatibile with
> 
> Typo, compatibile should be compatible.
> 
> > rte_eth_dev_allocated().
> >
> > Signed-off-by: Wojciech Zmuda <woz@semihalf.com>
> > ---
> >  drivers/net/af_packet/rte_eth_af_packet.c | 29
> > ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
> > b/drivers/net/af_packet/rte_eth_af_packet.c
> > index 767f36b..7ef65ff 100644
> > --- a/drivers/net/af_packet/rte_eth_af_packet.c
> > +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> > @@ -667,11 +667,13 @@ rte_pmd_init_internals(const char *name,
> >  	data->nb_tx_queues = (uint16_t)nb_queues;
> >  	data->dev_link = pmd_link;
> >  	data->mac_addrs = &(*internals)->eth_addr;
> > +	strncpy(data->name,
> > +		(*eth_dev)->data->name, strlen((*eth_dev)->data-
> > >name));
> >
> >  	(*eth_dev)->data = data;
> >  	(*eth_dev)->dev_ops = &ops;
> >  	(*eth_dev)->driver = NULL;
> > -	(*eth_dev)->data->dev_flags = 0;
> > +	(*eth_dev)->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
> >  	(*eth_dev)->data->drv_name = drivername;
> >  	(*eth_dev)->data->kdrv = RTE_KDRV_NONE;
> >  	(*eth_dev)->data->numa_node = numa_node; @@ -836,10 +838,35
> @@ exit:
> >  	return ret;
> >  }
> >
> > +static int
> > +rte_pmd_af_packet_devuninit(const char *name) {
> > +	struct rte_eth_dev *eth_dev = NULL;
> > +
> > +	RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket
> > %u\n",
> > +			rte_socket_id());
> > +
> > +	if (name == NULL)
> > +		return -1;
> > +
> > +	/* reserve an ethdev entry */
> 
> Above comment should be 	/* find the ethdev entry */
> 
> > +	eth_dev = rte_eth_dev_allocated(name);
> > +	if (eth_dev == NULL)
> > +		return -1;
> > +
> 
> In rte_pmd_init_internals() memory is allocated for rx and tx queues this
> should also be freed.
> 
> Line 606: rx_queue->rd = rte_zmalloc_socket(name, rdsize, 0, numa_node);
> Line 620:tx_queue->rd = rte_zmalloc_socket(name, rdsize, 0, numa_node);
> 
> 	rte_free((*internals)->rx_queue[q].rd);
> 	rte_free((*internals)->tx_queue[q].rd);
> 
> > +	rte_free(eth_dev->data->dev_private);
> > +	rte_free(eth_dev->data);
> > +
> > +	rte_eth_dev_release_port(eth_dev);
> > +
> > +	return 0;
> > +}
> > +
> >  static struct rte_driver pmd_af_packet_drv = {
> >  	.name = "eth_af_packet",
> >  	.type = PMD_VDEV,
> >  	.init = rte_pmd_af_packet_devinit,
> > +	.uninit = rte_pmd_af_packet_devuninit,
> >  };
> >
> >  PMD_REGISTER_DRIVER(pmd_af_packet_drv);
> > --
> > 1.9.1

Forgot to mention that there should probably be and entry in the 2.3 release notes, new features section, for this.

Regards,

Bernard.

[PATCH v2] af_packet: make the device detachable

[Wojciech Zmuda]

Implement rte_pmd_af_packet_devuninit() exposed through struct
rte_driver.uninit() and set dev_flags to RTE_ETH_DEV_DETACHABLE,
to allow af_packet device deinitialization with API function
rte_eth_dev_detach(). This fixes memory leak by freeing memory
allocated during initialization.
During device initialization copy device name to ethdev->data to make
it compatible with rte_eth_dev_allocated().

Signed-off-by: Wojciech Zmuda <woz@semihalf.com>
---
v2:
* Fixed typo and a comment.
* Added feature to the 2.3 release notes.
* Free memory allocated for rx and tx queues.

 doc/guides/rel_notes/release_2_3.rst      |  4 ++++
 drivers/net/af_packet/rte_eth_af_packet.c | 37 ++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
index 7945694..4694646 100644
--- a/doc/guides/rel_notes/release_2_3.rst
+++ b/doc/guides/rel_notes/release_2_3.rst
@@ -39,6 +39,10 @@ This section should contain new features added in this release. Sample format:
 
   Enabled virtio 1.0 support for virtio pmd driver.
 
+* **Added af_packet driver deinitialization function.**
+
+  Implemented rte_pmd_af_packet_devuninit() exposed through struct
+  rte_driver.uninit() to allow af_packet device deinitialization with API function.
 
 Resolved Issues
 ---------------
diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 767f36b..eaed916 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -667,11 +667,13 @@ rte_pmd_init_internals(const char *name,
 	data->nb_tx_queues = (uint16_t)nb_queues;
 	data->dev_link = pmd_link;
 	data->mac_addrs = &(*internals)->eth_addr;
+	strncpy(data->name,
+		(*eth_dev)->data->name, strlen((*eth_dev)->data->name));
 
 	(*eth_dev)->data = data;
 	(*eth_dev)->dev_ops = &ops;
 	(*eth_dev)->driver = NULL;
-	(*eth_dev)->data->dev_flags = 0;
+	(*eth_dev)->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
 	(*eth_dev)->data->drv_name = drivername;
 	(*eth_dev)->data->kdrv = RTE_KDRV_NONE;
 	(*eth_dev)->data->numa_node = numa_node;
@@ -836,10 +838,43 @@ exit:
 	return ret;
 }
 
+static int
+rte_pmd_af_packet_devuninit(const char *name)
+{
+	struct rte_eth_dev *eth_dev = NULL;
+	struct pmd_internals *internals;
+	unsigned q;
+
+	RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
+			rte_socket_id());
+
+	if (name == NULL)
+		return -1;
+
+	/* find the ethdev entry */
+	eth_dev = rte_eth_dev_allocated(name);
+	if (eth_dev == NULL)
+		return -1;
+
+	internals = eth_dev->data->dev_private;
+	for (q = 0; q < internals->nb_queues; q++) {
+		rte_free(internals->rx_queue[q].rd);
+		rte_free(internals->tx_queue[q].rd);
+	}
+
+	rte_free(eth_dev->data->dev_private);
+	rte_free(eth_dev->data);
+
+	rte_eth_dev_release_port(eth_dev);
+
+	return 0;
+}
+
 static struct rte_driver pmd_af_packet_drv = {
 	.name = "eth_af_packet",
 	.type = PMD_VDEV,
 	.init = rte_pmd_af_packet_devinit,
+	.uninit = rte_pmd_af_packet_devuninit,
 };
 
 PMD_REGISTER_DRIVER(pmd_af_packet_drv);
-- 
1.9.1

Re: [PATCH v2] af_packet: make the device detachable

[Bruce Richardson]

On Tue, Feb 09, 2016 at 05:09:06PM +0100, Wojciech Zmuda wrote:
> Implement rte_pmd_af_packet_devuninit() exposed through struct
> rte_driver.uninit() and set dev_flags to RTE_ETH_DEV_DETACHABLE,
> to allow af_packet device deinitialization with API function
> rte_eth_dev_detach(). This fixes memory leak by freeing memory
> allocated during initialization.
> During device initialization copy device name to ethdev->data to make
> it compatible with rte_eth_dev_allocated().
> 
> Signed-off-by: Wojciech Zmuda <woz@semihalf.com>
> ---
> v2:
> * Fixed typo and a comment.
> * Added feature to the 2.3 release notes.
> * Free memory allocated for rx and tx queues.
> 
>  doc/guides/rel_notes/release_2_3.rst      |  4 ++++
>  drivers/net/af_packet/rte_eth_af_packet.c | 37 ++++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
> index 7945694..4694646 100644
> --- a/doc/guides/rel_notes/release_2_3.rst
> +++ b/doc/guides/rel_notes/release_2_3.rst
> @@ -39,6 +39,10 @@ This section should contain new features added in this release. Sample format:
>  
>    Enabled virtio 1.0 support for virtio pmd driver.
>  
> +* **Added af_packet driver deinitialization function.**
> +
> +  Implemented rte_pmd_af_packet_devuninit() exposed through struct
> +  rte_driver.uninit() to allow af_packet device deinitialization with API function.
>  

The use of "deinitialization" sounds awkward, and the overall text maybe could be
made less technical. Maybe talk about "allowing dynamic removal" of af_packet
devices [or even hotplug of them]? 

/Bruce

Re: [PATCH v2] af_packet: make the device detachable

[Wojciech Żmuda]

Hi Bruce,

>The use of "deinitialization" sounds awkward

Thank you for your interest. I called it deinitialization in
opposition to an initialization of a device. As I'm not a native
English speaker, I trust your opinion and I'll try to rephrase this.

Hi Bernard,

>What parameters do you use with --vdev option in testpmd

I launch testpmd like this:
# ./testpmd -c f -n 4 --vdev 'eth_af_packet0,iface=eth2' -- -i
--port-topology=chained

Then I can see that af_packet starts:

PMD: Initializing pmd_af_packet for eth_af_packet0
PMD: eth_af_packet0: AF_PACKET MMAP parameters:
PMD: eth_af_packet0:    block size 4096
PMD: eth_af_packet0:    block count 256
PMD: eth_af_packet0:    frame size 2048
PMD: eth_af_packet0:    frame count 512
PMD: eth_af_packet0: creating AF_PACKET-backed ethdev on numa socket

When I get to the CLI, I do as follows:

Checking link statuses...
Port 0 Link Up - speed 10000 Mbps - full-duplex
Done
testpmd> port stop 0
Stopping ports...
Checking link statuses...
Port 0 Link Down
Done
testpmd> port close 0
Closing ports...
Done
testpmd> port detach 0
Detaching a port...
PMD: Closing AF_PACKET ethdev on numa socket 0
Port 'eth_af_packet0' is detached. Now total ports is 0
Done
testpmd>

In opposition, without the patch detach is impossible:

testpmd> port detach 0
Detaching a port...
EAL: Driver, cannot detach the device
testpmd>


Bernard, Bruce, I have a question, if I may. Do you know what is the
reason that rte_pmd_af_packet_devinit() is the only non-static device
initialization function among all the dpdk drivers? There's even a
comment in the rte_eth_af_packet.h:

/**
 * For use by the EAL only. Called as part of EAL init to set up any dummy NICs
 * configured on command line.
 */
int rte_pmd_af_packet_devinit(const char *name, const char *params);

Despite the comment above, I cannot see this function being called
directly anywhere. Is there any reason it is implemented this way? Or
should I change the definition to static, as it should be called via
proper API functions?

Thank you for your time,
Wojtek

2016-02-09 17:37 GMT+01:00 Bruce Richardson <bruce.richardson@intel.com>:
> On Tue, Feb 09, 2016 at 05:09:06PM +0100, Wojciech Zmuda wrote:
>> Implement rte_pmd_af_packet_devuninit() exposed through struct
>> rte_driver.uninit() and set dev_flags to RTE_ETH_DEV_DETACHABLE,
>> to allow af_packet device deinitialization with API function
>> rte_eth_dev_detach(). This fixes memory leak by freeing memory
>> allocated during initialization.
>> During device initialization copy device name to ethdev->data to make
>> it compatible with rte_eth_dev_allocated().
>>
>> Signed-off-by: Wojciech Zmuda <woz@semihalf.com>
>> ---
>> v2:
>> * Fixed typo and a comment.
>> * Added feature to the 2.3 release notes.
>> * Free memory allocated for rx and tx queues.
>>
>>  doc/guides/rel_notes/release_2_3.rst      |  4 ++++
>>  drivers/net/af_packet/rte_eth_af_packet.c | 37 ++++++++++++++++++++++++++++++-
>>  2 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
>> index 7945694..4694646 100644
>> --- a/doc/guides/rel_notes/release_2_3.rst
>> +++ b/doc/guides/rel_notes/release_2_3.rst
>> @@ -39,6 +39,10 @@ This section should contain new features added in this release. Sample format:
>>
>>    Enabled virtio 1.0 support for virtio pmd driver.
>>
>> +* **Added af_packet driver deinitialization function.**
>> +
>> +  Implemented rte_pmd_af_packet_devuninit() exposed through struct
>> +  rte_driver.uninit() to allow af_packet device deinitialization with API function.
>>
>
> The use of "deinitialization" sounds awkward, and the overall text maybe could be
> made less technical. Maybe talk about "allowing dynamic removal" of af_packet
> devices [or even hotplug of them]?
>
> /Bruce

Re: [PATCH v2] af_packet: make the device detachable

[John W. Linville]

On Wed, Feb 10, 2016 at 04:42:53PM +0100, Wojciech Żmuda wrote:

> Bernard, Bruce, I have a question, if I may. Do you know what is the
> reason that rte_pmd_af_packet_devinit() is the only non-static device
> initialization function among all the dpdk drivers? There's even a
> comment in the rte_eth_af_packet.h:
> 
> /**
>  * For use by the EAL only. Called as part of EAL init to set up any dummy NICs
>  * configured on command line.
>  */
> int rte_pmd_af_packet_devinit(const char *name, const char *params);
> 
> Despite the comment above, I cannot see this function being called
> directly anywhere. Is there any reason it is implemented this way? Or
> should I change the definition to static, as it should be called via
> proper API functions?

The af_packet driver structure was essentially copied from the pcap
driver.  Way in the past there was the EAL initialization of the
"virtual" drivers, and the original af_packet code hooked into that.
Somewhere along the way the driver initialization code changed and
I guess the EAL initialization bits disappeared.  The comment above
rte_pmd_af_packet_devinit was overlooked.

Long story, short -- I think you can remove the comment and add the
static modifier and everything will be fine.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.