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

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Friday, March 13, 2015 12:11 AM
> To: Iremonger, Bernard
> Cc: John W. Linville; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
> 
> 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
> 
Hi Tetsuya,

In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro  PROC_PRIMARY_OR_RET().
So for secondary processes  both functions return without doing anything. 
Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?

For the Physical/Virtual  Functions of the NIC  a lot of the finalization is done in the  dev->dev_ops->dev_stop() and
dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is called, this should probably do nothing for secondary processes  as the dev_stop() and dev_close() functions will not have been executed.

For the Physical/Virtual  Functions of the NIC  the dev_init() is called for both primary and secondary processes, however a subset of the function only is executed for secondary processes.

Regards,

Bernard.