Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Tetsuya Mukawa <mukawa@igel.co.jp>]

2015-03-13 2:05 GMT+09:00 Iremonger, Bernard <bernard.iremonger@intel.com>:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of John W. Linville
>> Sent: Tuesday, March 10, 2015 6:36 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug
>>
>> This patch adds finalization code to free resources allocated by the PMD.  This is based on earlier
>> patches for other PMDs by Tetsuya Mukawa <mukawa@igel.co.jp>, with corrections related to data-
>> >name.
>>
>> Signed-off-by: John W. Linville <linville@tuxdriver.com>
>> Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56 ++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> index 80e9bdf7f819..57998ab19c96 100644
>> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
>>       .stats_reset = eth_stats_reset,
>>  };
>>
>> +static struct eth_driver rte_af_packet_pmd = {
>> +     .pci_drv = {
>> +             .name = "rte_af_packet_pmd",
>> +             .drv_flags = RTE_PCI_DRV_DETACHABLE,
>> +     },
>> +};
>> +
>>  /*
>>   * Opens an AF_PACKET socket
>>   */
>> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
>>       if (*eth_dev == NULL)
>>               goto error;
>>
>> +     /* check length of device name */
>> +     if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
>> +             goto error;
>> +
>>       /*
>>        * now put it all together
>>        * - store queue data in internals,
>> @@ -669,12 +680,14 @@ 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));
>>
>>       pci_dev->numa_node = numa_node;
>>
>>       (*eth_dev)->data = data;
>>       (*eth_dev)->dev_ops = &ops;
>>       (*eth_dev)->pci_dev = pci_dev;
>> +     (*eth_dev)->driver = &rte_af_packet_pmd;
>>
>>       return 0;
>>
>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>>       return 0;
>>  }
>>
>> +static int
>> +rte_pmd_af_packet_devuninit(const char *name) {
>> +     struct rte_eth_dev *eth_dev = NULL;
>> +     struct pmd_internals *internals;
>> +     struct tpacket_req req;
>> +
>> +     unsigned q;
>> +
>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
>> +                     rte_socket_id());
>> +
>> +     if (name == NULL)
>> +             return -1;
>
> Hi  Tetsuya, John,
>
> Before detaching a port, the port must be stopped and closed.
> The stop and close are only allowed for RTE_PROC_PRIMARY.
> Should there be a check for process_type here?
>
> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>         return -EPERM;
>
> Regards,
>
> Bernard
>

Hi Bernard,

I agree with stop() and close() are only called by primary process,
but it may not need to add like above.
Could you please check rte_ethdev.c?

- struct rte_eth_dev_data *rte_eth_dev_data;
This array is shared between processes.
So we need to initialize of finalize carefully like you said.

- struct rte_eth_dev rte_eth_devices[]
This array is per process.
And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.

All PMDs for physical NIC allocates like above when PMDs are initialized.
(Even when a process is secondary, initialization function of PMDs
will be called)
But virtual device PMDs allocate rte_eth_dev_data and overwrite 'data'
variable of rte_eth_devices while initialization.

As a result, primary and secondary process has their own
'rte_eth_dev_data' for a virtual device.
So I guess all processes need to free it not to leak memory.

Thanks,
Tetsuya

>> +
>> +     /* retrieve ethdev entry */
>> +     eth_dev = rte_eth_dev_allocated(name);
>> +     if (eth_dev == NULL)
>> +             return -1;
>> +
>> +     internals = eth_dev->data->dev_private;
>> +     req = internals->req;
>> +
>> +     for (q = 0; q < internals->nb_queues; q++) {
>> +             munmap(internals->rx_queue[q].map,
>> +                    2 * req.tp_block_size * req.tp_block_nr);
>> +             if (internals->rx_queue[q].rd)
>> +                     rte_free(internals->rx_queue[q].rd);
>> +             if (internals->tx_queue[q].rd)
>> +                     rte_free(internals->tx_queue[q].rd);
>> +     }
>> +
>> +     rte_free(internals);
>> +     rte_free(eth_dev->data);
>> +     rte_free(eth_dev->pci_dev);
>> +
>> +     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);
>> --
>> 2.1.0
>