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: 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 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.

Regards,

Bernard.