Re: [dpdk-dev] How to replace rte_eth_dev_attach with rte_eal_hotplug_add

[Thomas Monjalon <thomas@monjalon.net>]

Hi,

The better approach is using RTE_ETH_FOREACH_MATCHING_DEV for 2 reasons:
	- it is a loop, so work if multiple ports are matching
	- it uses devargs parameter, which is what the user requests

Note: your code assumes that the ethdev name is devargs.name.
It can be true by chance, but nothing forces drivers to assign port names
this way. It will be wrong, for sure, if a device has multiple ports.


22/10/2018 13:24, Hideyuki Yamashita:
> Hello Thomas,
> 
> Thanks for your info.
> What is the difference between using 
> rte_eth_dev_get_port_by_name and 
> RTE_ETH_FOREACH_MATCHING_DEV?
> 
> I think using rte_eth_dev_get_port_by_name is 
> workable.
> (In fact I modified my code already and it worked with no problem)
> 
> So my question is "what is the difference" and "which is better approach".
> 
> Thanks and BR,
> Hideyuki Yamashita
> NTT TechnoCross
> 
> > Hi,
> > 
> > I am actively working on it.
> > Look how rte_eth_dev_attach is replaced in testpmd:
> > 	https://patches.dpdk.org/patch/47019/
> > It is using a new ethdev iterator RTE_ETH_FOREACH_MATCHING_DEV.
> > 
> > 
> > 22/10/2018 06:34, Hideyuki Yamashita:
> > > Dear Thomas and all,
> > > 
> > > About a month ago, I posted the topic related with 
> > > how to replace rte_eth_dev_attach.
> > > 
> > > Following your advice,
> > > my code would be as below:
> > > (Old code using deprecated API is commented out)
> > > 
> > > rte_eth_dev_get_port_by_name is used to retrieve dpdk port
> > > after rte_eal_hotplug_add.
> > > Note that my application is just one of the dpdk applications(in the host)
> > > and within the process, only one thread handles device attatch/detach.
> > > (No race condition with regard to device hot_plug will
> > > not take place)
> > > -------------------------------------------------------------------
> > >         //ret = rte_eth_dev_attach(devargs, &vhost_port_id);
> > >         //if (ret < 0)
> > >         //      return ret;
> > > 
> > >         struct rte_devargs da;
> > > 
> > >         memset(&da, 0, sizeof(da));
> > > 
> > >         /* parse devargs */
> > >         if (rte_devargs_parse(&da, devargs))
> > >                 return -1;
> > >         ret = rte_eal_hotplug_add(da.bus->name, da.name, da.args);
> > >         if (ret < 0) {
> > >                 free(da.args);
> > >                 return ret;
> > >         }
> > >         free(da.args);
> > >         ret = rte_eth_dev_get_port_by_name(da.name, &vhost_port_id);
> > >         if (ret < 0)
> > >                 return ret;
> > > -----------------------------------------------------------------------------
> > > 
> > > If you have any concerns/additional advices, please let me know.
> > > 
> > > BR,
> > > Hideyuki Yamashita
> > > NTT TechnoCross
> > > 
> > > 
> > > > 27/09/2018 12:40, Hideyuki Yamashita:
> > > > > Dear Thomas,
> > > > > 
> > > > > Thansk for your answer.
> > > > > Please see inline.
> > > > > 
> > > > > > 27/09/2018 03:38, Hideyuki Yamashita:
> > > > > > > Dear Thomas,
> > > > > > > 
> > > > > > > Thanks for your answer.
> > > > > > > It took me a little time to digest answer.
> > > > > > > Please see inline.
> > > > > > > 
> > > > > > > 
> > > > > > > > 21/09/2018 09:19, Hideyuki Yamashita:
> > > > > > > > > Dear Gaetan and Thomas, 
> > > > > > > > > 
> > > > > > > > > Thanks for your answer.
> > > > > > > > > Please see inline.
> > > > > > > > > 
> > > > > > > > > > 20/09/2018 11:09, Ga?an Rivet:
> > > > > > > > > > > On Thu, Sep 20, 2018 at 05:46:37PM +0900, Hideyuki Yamashita wrote:
> > > > > > > > > > > > Hello,
> > > > > > > > > > > > 
> > > > > > > > > > > > From dpdk 18.08 release rte_eth_dev_attach and 
> > > > > > > > > > > > rte_eth_dev_detach becom deprecated API and 
> > > > > > > > > > > > it is recommended to replace with rte_eal_hotplug_add
> > > > > > > > > > > > and rte_eal_hotplug_remove.
> > > > > > > > > > > > 
> > > > > > > > > > > > My program uses  above mentioned deprecated APIs
> > > > > > > > > > > > and have to replace those.
> > > > > > > > > > > > Note that my program uses attach to attach vhost, pcap pmd.
> > > > > > > > > > > > 
> > > > > > > > > > > > My question is whether it is correct to replace those as following:
> > > > > > > > > > > > find rte_eth_dev_attach function in rte_ethdev.c and
> > > > > > > > > > > > migrate those content into my program.
> > > > > > > > > > > > 
> > > > > > > > > > > > e.g. 
> > > > > > > > > > > > lib/librte_ethdev/rte_ethdev.c line 643-686 for attach
> > > > > > > > > > > > lib/librte_ethdev/rte_ethdev.c line 690-720 for detach
> > > > > > > > > > > > 
> > > > > > > > > > > > Your advice/guidance are much appreciated.
> > > > > > > > > > > > Thanks!
> > > > > > > > > > > 
> > > > > > > > > > > Hello Hideyuki,
> > > > > > > > > > > 
> > > > > > > > > > > You could use this code for guidance, while leaving the ethdev
> > > > > > > > > > > specificities such as verifying the eth_dev_count_total(). The hotplug
> > > > > > > > > > > function would already return an error if the PMD was not able to create
> > > > > > > > > > > the necessary devices.
> > > > > > > > > > > 
> > > > > > > > > > > The main issue might be to find the port_id of your new port.
> > > > > > > > > > > You won't be able to use eth_dev_last_created_port, so you would have to
> > > > > > > > > > > iterate over the ethdev using RTE_ETH_FOREACH_DEV and find the one
> > > > > > > > > > > matching your parameters (you might for example match the rte_device
> > > > > > > > > > > name with the name you used in hotplug_add, as there is no standard
> > > > > > > > > > > naming scheme at the ethdev level).
> > > > > > > > > First of all, thank for your answering to my question.
> > > > > > > > > But I have questions.
> > > > > > > > > (Sorry, I have poor knowledge about dpdk and have many basic questions)
> > > > > > > > > 
> > > > > > > > > Q1. 
> > > > > > > > > Why eth_dev_last_created_port can not be used?
> > > > > > > > > When I look into rte_eth_dev_atthach in 18.08, it calls 
> > > > > > > > > 
> > > > > > > > > *port_id = eth_dev_last_created_port;
> > > > > > > > > 
> > > > > > > > > at the end of the function.
> > > > > > > > 
> > > > > > > > You can have a race condition.
> > > > > > > Please elaborate me a bit more.
> > > > > > > 
> > > > > > > Is it correct understanding that race condition 
> > > > > > > includes
> > > > > > > - read information before port is available
> > > > > > > - other device may be plugged (or unplugged)
> > > > > > > and so using "eth_dev_last_created_port" is 
> > > > > > > NOT reliable?
> > > > > > 
> > > > > > I am thinking about the second one only.
> > > > > 
> > > > > If we assume there is only one DPDK application 
> > > > > inside the host and within the application, only one thread
> > > > > handles attach/detach of devices, then is it ok to use 
> > > > > > > > > *port_id = eth_dev_last_created_port;
> > > > > because there seems no possiblity race condition
> > > > > takes place?
> > > > 
> > > > If you are never probing a new port outside of this thread,
> > > > I guess it's OK.
> > > > Take care of not attaching from the interrupt thread too!