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: Tuesday, March 17, 2015 3:43 AM
> To: Iremonger, Bernard
> Cc: John W. Linville; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
> 
> On 2015/03/16 23:47, Iremonger, Bernard wrote:
> >
> >> -----Original Message-----
> >> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> >> Sent: Monday, March 16, 2015 8:57 AM
> >> To: Iremonger, Bernard
> >> Cc: John W. Linville; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
> >>
> >>>>>>> @@ -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
> >>>> dev->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
> > Hi Tetsuya,
> > For the e1000, igb and ixgbe PMD's it is probably  simpler to just check for the primary process in the
> uninit functions and just return without doing anything for secondary processes.
> 
> Thanks for clarifying.
> In the case, is it okay for you to add PROC_PRIMARY_OR_RET() in e1000, igb and ixgbe PMD code?
> If it's okay, we may be able to ACK this patch. :)
> 
> Regards,
> Tetsuya
>

Hi Tetsuya,

I will add the process type check in the e1000, igb amd ixgbe PMD code.

Regards,

Bernard.