[dpdk-dev,v2] af_packet: make the device detachable
Commit Message
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(-)
Comments
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
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
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
@@ -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
---------------
@@ -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);