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

[Hideyuki Yamashita <yamashita.hideyuki@po.ntt-tx.co.jp>]

Hi, 

> 25/10/2018 04:54, Hideyuki Yamashita:
> > Hi,
> > 
> > > Yes it may work with most of the drivers.
> > Question for my understadnding.
> > You said that most of the drivers assign only one 
> > port when hotplug_add is called, right?
> > Then what is the exception?
> > What kind of device/driver assign multiple ports?
> 
> At least Mellanox and Chelsio, maybe more.

You are saying about physical device(e.g. NIC).
Is my understanding correct ?

> > My program attach to 
> >  - vhost pmd
> >  - pcap pmd
> >  - null pmd
> > and I understand those device(?) assign only one port per attach.
> 
> No problem with vdevs.

In my program, program never attach(hotplug) to physical devices
dynamically.
Like L2fwd example, physical NICs are bound using dpdk/usertoools/
dpdk-setup.sh.
My program only attaches to vdevs dynamically.
In the above case, "rte_eth_dev_get_port_by_name" is sufficient enough.
Is my understanding correct?

BR,
Hideyuki Yamashita
NTT TechnoCross
> 
> > > Hi,
> > > 
> > > 23/10/2018 03:52, Hideyuki Yamashita:
> > > > Hi,
> > > > 
> > > > Thanks for your guidance again.
> > > > 
> > > > Q1.
> > > > Is following my understanding correct?
> > > > If a device has multiple port, then "rte_eth_dev_get_port_by_name"
> > > > will NOT work becauase it uses strcmp and needs "exact match"
> > > > of the device name.
> > > > New iterator RTE_ETH_FOREACH_MATCHING_DEV takes care 
> > > > of this issue and even if the name is "parially matched" with 
> > > > the given parameter(user provided devargs).
> > > 
> > > Yes
> > > 
> > > > Q2. 
> > > > If my program only handles devices which create only one port,
> > > > then "rte_eth_dev_get_port_by_name" may work.
> > > > (Though such a program does not have extensibility and
> > > > only workable under certain limitations)
> > > 
> > > Yes it may work with most of the drivers.
> > > 
> > > > Q3.
> > > > When new iterator  RTE_ETH_FOREACH_MATCHING_DEV
> > > > will be available?
> > > > Do I have to wait 18.11 release or can I get those in git before release?
> > > 
> > > Better to wait 18.11.
> > > 
> > > > I agree with your guidance that RTE_ETH_FOREACH_MATCHING_DEV
> > > > is much better than using "rte_eth_dev_get_port_by_name" with regard
> > > > to handle devices which create multiple port.
> > > > But I need to replace existing deprecated attach/detach APIs to new
> > > > codes to maintain continuity of my product.
> > > 
> > > We do not remove a method without a replacement.
> > > You can use rte_eth_dev_attach() until 18.08,
> > > and switch to the new iterator with 18.11.
> > > 
> > > 
> > > > > 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!
> > > 
> > > 
> > > 
> > > 
> > 
> > 
> > 
> > 
> 
> 
> 
>