Hello Jan, On Thu, Jan 14, 2016 at 12:46 PM, Jan Viktorin <viktorin at rehivetech.com> wrote: > On Thu, 14 Jan 2016 11:38:16 +0100 > David Marchand <david.marchand at 6wind.com> wrote: > >> Hello all, >> >> Here is a proposal of a big cleanup in ethdev (cryptodev would have to >> follow) and eal structures. >> This is something I wanted to do for quite some time and the arrival of >> a new bus makes me think we need it. >> >> This is an alternative to what Jan proposed [1]. > > As I need to extend DPDK by a non-PCI bus system, I would prefer any such > working solution :). By [1], you probably mean: > > [1] http://comments.gmane.org/gmane.comp.networking.dpdk.devel/30973 > > (I didn't find it in the e-mail.)
Thought I put the reference after my signature. Anyway, yes, I was referring to your thread. >> ABI is most likely broken with this, but I think this discussion can come >> later. > > I was trying in my initial approach to stay as much API and ABI backwards > compatible as possible to be acceptable into upstream. As just a few > people have shown their interest in these changes, I consider the ABI > compatibility very important. > > I can see, that your approach does not care too much... Otherwise, it looks > good to me. It is closer to the Linux drivers infra, so it seems to be clearer > then the current one. I did this on purpose. >From my point of view, we will have an API/ABI breakage in this code at one point. So I sent this mail to show where I'd like us to go, because this looks saner on the mid/long term. >> First some context on how dpdk is initialized at the moment : >> >> Let's imagine a system with one ixgbe pci device and take some >> part of ixgbe driver as an example. >> >> static struct eth_driver rte_ixgbe_pmd = { >> .pci_drv = { >> .name = "rte_ixgbe_pmd", >> .id_table = pci_id_ixgbe_map, >> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | >> RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_DETACHABLE, >> }, >> .eth_dev_init = eth_ixgbe_dev_init, >> .eth_dev_uninit = eth_ixgbe_dev_uninit, >> .dev_private_size = sizeof(struct ixgbe_adapter), >> }; > > Note, that the biggest issue here is that the eth_driver has no way to > distinguish among PCI or other subsystem. There is no field that helps > the generic ethdev code (librte_ether) to decide what bus we are on > (and it needs to know it in the current DPDK). > > Another point is that the ethdev infra should not know about the > underlying bus infra. The question is whether we do a big clean > up (extract PCI-bus code out) or a small clean up (give the ethdev > infra a hint which bus system it deals with). Yes, and I think these two choices are reflected by our two respective proposals :-) >> So now, what I had in mind is something like this. >> It is far from perfect and I certainly did some shortcuts in my >> reasoning. >> >> >> Generic device/driver >> >> - introduce a rte_device structure, >> - a rte_device has a name, that identifies it in a unique way across >> all buses, maybe something like pci:0000:00:01.0, and for vdev, >> vdev:name > > Having a prefix is good but would this break the user API? Is the > name exposed to users? Maybe define new apis, and wrap the old one in it ? Not too sure, I need to experiment. >> - current rte_driver does not need to know about the pmd_type >> (pdev/vdev), this is only a way to order drivers init in eal, we could >> use the rte_driver names to order them or maybe remove this ordering > > What is the main reason to have pdev/vdev distinction here? After I > register the driver, does eal really need to know the pmd_type? Already did some cleanup in the past because of pmd_type : http://dpdk.org/browse/dpdk/commit/?id=78aecefed955917753bfb6f44ae970dde4c652d0 http://dpdk.org/browse/dpdk/commit/?id=6bc2415c3387ae72f2ce3677f0e3540e734583d5 For me, there is no reason but the init ordering (vdevs come before pdevs), and I am pretty sure we don't need this for ethdev. > Moreover, there is no way how to pass arguments to pdevs, only to > vdevs. This was shortly disscussed in [2] for the szedata2 PMD. > > [2] http://dpdk.org/ml/archives/dev/2015-October/026285.html Shorty discussed with Thomas, yes. But after some experiments, it appears that you can pass devargs after a whitelist option at init (--pci-whitelist xxxx:xx:xx.x,whataniceoptionwehavehere=1). This does not work for hotplug. This is undocumented and this looks like a workaround, so I would prefer we come up with a better api for 2.3. >> Impact on PCI device/driver >> >> - rte_pci_device is modified to embed a rte_device (embedding makes it >> possible later to cast the rte_device and get the rte_pci_device in pci >> specific functions) >> - no need for a rte_pci_driver reference in rte_pci_device, since we >> have the rte_device driver > > I've noticed that DPDK does not use any kind of the container_of macro > like the Linux Kernel does. Instead, some considerations like the > rte_pci_driver MUST be placed at the beginning of the eth_driver (if I > am not mistaken). Here, it should be considered to do it another way. Ah, yes, we could use this. Just did not think of using it :-) Regards, -- David Marchand