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

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

On 2015/03/16 17:38, Tetsuya Mukawa wrote:
> On 2015/03/13 19:14, Iremonger, Bernard wrote:
>>> -----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.
> Hi Bernard,
>
> Sorry for my English.
> I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs.
> Not a PMDs for virtual functions on NIC.
>
> For PMDs like a pcap and af_packet PMDs, all data structures are
> allocated per processes.
> (Actually I guess nothing is shared between primary and secondary
> processes, because rte_eth_dev_data is overwritten by each processes.)
> So we need to free per process data when detach() is called.
>
>> 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.
> Because of above, we probably not be able to add PROC_PRIMARY_OR_RET()
> to rte_eth_dev_detach().
> But I agree we should not call rte_eth_dev_detach() for secondary
> process, if PMDs are like e1000 or ixgbe PMD.

Correction:
We should not process rte_eth_dev_detach() for secondary process, if
PMDs are like e1000 or ixgbe PMD and if primary process hasn't called
stop() and close() yet.

Tetsuya

>
> To work like above, how about changing drv_flags dynamically in close()
> callback?
> For example, when primary process calls rte_eth_dev_close(), a callback
> of PMD will be called.
> (In the case of e1000 PMD, eth_em_close() is the callback.)
>
> At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the
> callback.
> It means if primary process hasn't called close() yet,
> rte_eth_dev_detach() will do nothing and return error.
> How about doing like above?
>
> Regards,
> Tetsuya
>
>> Regards,
>>
>> Bernard.
>>  
>>
>>
>>
>>
>>
>>
>