On Thursday 17 November 2016 05:25 PM, Jan Blunck wrote: > On Thu, Nov 17, 2016 at 6:29 AM, Shreyansh Jain <shreyansh.jain at nxp.com> > wrote: >> DPDK has been inherently a PCI inclined framework. Because of this, the >> design of device tree (or list) within DPDK is also PCI inclined. A non-PCI >> device doesn't have a way of being expressed without using hooks started from >> EAL to PMD. >> >> With this cover letter, some patches are presented which try to break this >> strict linkage of EAL with PCI devices. Aim is to generalize the device >> hierarchy on the lines of how Linux handles it: >> >> device A1 >> | >> +==.===='==============.============+ Bus A. >> | `--> driver A11 \ >> device A2 `-> driver A12 \______ >> |CPU | >> /````` >> device A1 / >> | / >> +==.===='==============.============+ Bus A` >> | `--> driver B11 >> device A2 `-> driver B12 >> >> Simply put: >> - a bus is connect to CPU (or core) >> - devices are conneted to Bus >> - drivers are running instances which manage one or more devices >> - bus is responsible for identifying devices (and interrupt propogation) >> - driver is responsible for initializing the device >> >> (*Reusing text from email [1]) >> In context of DPDK EAL: >> - a generic bus (not a driver, not a device). I don't know how to categorize >> a bus. It is certainly not a device, and then handler for a bus (physical) >> can be considered a 'bus driver'. So, just 'rte_bus'. >> - there is a bus for each physical implementation (or virtual). So, a >> rte_bus >> Object for PCI, VDEV, ABC, DEF and so on. >> - Buses are registered just like a PMD - RTE_PMD_BUS_REGISTER() >> - Each registered bus is part of a doubly list. >> -- Each device inherits rte_bus >> -- Each driver inherits rte_bus >> -- Device and Drivers lists are part of rte_bus >> - eth_driver is no more required - it was just a holder for PMDs to register >> themselves. It can be replaced with rte_xxx_driver and corresponding init/ >> uninit moved to rte_driver >> - rte_eth_dev modified to disassociate itself from rte_pci_device and >> connect >> to generic rte_device >> >> Once again, improvising from [1]: >> >> __ rte_bus_list >> / >> +----------'---+ >> |rte_bus | >> | driver_list------> List of rte_bus specific >> | device_list---- devices >> | scan | `-> List of rte_bus associated >> | match | drivers >> | dump | >> | ..some refcnt| (#) >> +--|------|----+ >> _________/ \_________ >> +--------/----+ +-\---------------+ >> |rte_device | |rte_driver | >> | rte_bus | | rte_bus | >> | rte_driver |(#) | init | >> | | | uninit | >> | devargs | | dev_private_size| >> +---||--------+ | drv_flags |(#) >> || | intr_handle(2*) |(#) >> | \ +----------\\\----+ >> | \_____________ \\\ >> | \ ||| >> +------|---------+ +----|----------+ ||| >> |rte_pci_device | |rte_xxx_device | (4*) ||| >> | PCI specific | | xxx device | ||| >> | info (mem,) | | specific fns | / | \ >> +----------------+ +---------------+ / | \ >> _____________________/ / \ >> / ___/ \ >> +-------------'--+ +------------'---+ +--'------------+ >> |rte_pci_driver | |rte_vdev_driver | |rte_xxx_driver | >> | PCI id table, | | <probably, | | .... | >> | other driver | | nothing> | +---------------+ >> | data | +----------------+ >> +----------------+ >> >> (1*) Problem is that probe functions have different arguments. So, >> generalizing them might be some rework in the respective device >> layers >> (2*) Interrupt handling for each driver type might be different. I am not >> sure how to generalize that either. This is grey area for me. >> (3*) Probably exposing a bitmask for device capabilities. Nothing similar >> exists now to relate it. Don't know if that is useful. Allowing >> applications to question a device about what it supports and what it >> doesn't - making it more flexible at application layer (but more >> code >> as well.) >> (4*) Even vdev would be an instantiated as a device. It is not being done >> currently. >> (#) Items which are still pending >> >> With this cover letter, some patches have been posted. These are _only_ for >> discussion purpose. They are not complete and neither compilable. >> All the patches, except 0001, have sufficient context about what the changes >> are and rationale for same. Obviously, code is best answer. >> >> === Patch description: === >> >> Patch 0001: Introduce container_of macro. Originally a patch from Jan. >> >> Patch 0002: introduce changes to rte_device/rte_driver for rte_bus, and >> rte_bus definition itself. >> >> Patch 0003: Add a new layer for 'bus driver' with linuxapp PCI as an example >> >> Patch 0004: Changes with respect to rte_bus APIs and impact on eal_common_pci > > From my point of view it is beneficial to keep the PCI support in one > place. Moving the match() and scan() > to the drivers directory doesn't seem to be technically required but > it makes the code harder to read and understand.
It is not a technical requirement - just logical placement. These are bus specific logic and should exist with the bus driver - where ever that is placed. Though, I am not entirely sure about 'harder to read'. If a person is reading through PCI's bus implementation, I am assuming it would be nice to have all the hooks (or default hooks) at same place. Isn't it? Or, maybe you are suggesting move all librte_eal/*/*pci* out to some common location. If so, that is something I haven't yet given serious thought. > > >> Patch 0005: Change to rte_eal_init (of linuxapp only, for now) for supporting >> bus->scan. Probe is still being done old way, but in a new >> wrapper >> >> Patch 0006: eth_driver removal and corresponding changes in ixgbe_ethdev, as >> an example. Includes changes to rte_ethdev to remove most >> possible >> PCI references. But, work still remains. > > Making rte_ethdev independent from PCI device is not directly related > to the rest of the patches that add bus support. I think it makes > sense to handle this separately because of the impact of refactoring > rte_ethdev. Once rte_device is available, the change is independent. Only dependency on it was changes required to rte_ethdev.c file because of various PCI naming/variables/functions. And that is indeed a very large change - including changes to drivers/* which I haven't yet done. Are you suggesting breaking out of this series? If so, can be done but only when formal patches start coming out. > > >> >> === Pending Items/Questions: === >> >> - Interrupt and notification handling. How to allow drivers to be notified >> of presence/plugging of a device. >> - Placement of bus driver/handling code. librte_bus, bus/, drivers/bus? >> -- Also from a pespective of a external library and whether symbols would be >> available in that. >> -- and secondary processes >> - VDEV bus is missing from current set. >> - Locking of list for supporting hotplugging. Or, at the least safe add/ >> remove >> - PMDINFOGEN support or lack of it. >> - Is there ever a case where rte_eth_dev needs to be resolved from >> rte_pci_device? I couldn't find any such use and neither a use-case for >> it. >> - There should be a way for Bus APIs to return a generic list handle so that >> EAL doesn't need to bother about bus->driver_list like dereferencing. This >> is untidy as well as less portable (in terms of code movement, not arch). >> - Are more helper hooks required for a bus? >> -- I can think of scan, match, dump, find, plug (device), unplug (device), >> associate (driver), disassociate (driver). But, most of the work is >> already being done by lower instances (rte_device/driver etc). >> >> Further: >> - In next few days I will make all necessary changes on the lines mentioned >> in the patches. This would include changing the drivers/* and librte_eal/* >> - As an when review comments float in and agreement reached, I will keep >> changing the model >> - There are grey areas like interrupt, notification, locking of bus/list >> which require more discussion. I will try and post a rfc for those as well >> or if someone can help me on those - great > > As already hinted on IRC I'm taking a look at the interrupt usage of ethdev. Great. Thank you. Do let me know feedback for any changes that you might require along the way. > >> - Change would include PCI bus and VDEV bus handling. A new bus (NXP's >> FSLMC) >> would also be layered over this series to verify the model of 'bus >> registration'. This is also part of 17.02 roadmap. >> >> [1] http://dpdk.org/ml/archives/dev/2016-November/050186.html >> >> Jan Viktorin (1): >> eal: define container macro >> >> Shreyansh Jain (5): >> eal: introduce bus-device-driver structure >> bus: add bus driver layer >> eal/common: handle bus abstraction for device/driver objects >> eal: supporting bus model in init process >> eal: removing eth_driver >> >> bus/Makefile | 36 +++ >> bus/pci/Makefile | 37 +++ >> bus/pci/linuxapp/pci_bus.c | 418 >> +++++++++++++++++++++++++++++ >> bus/pci/linuxapp/pci_bus.h | 55 ++++ >> drivers/net/ixgbe/ixgbe_ethdev.c | 49 ++-- >> lib/librte_eal/common/eal_common_bus.c | 188 +++++++++++++ >> lib/librte_eal/common/eal_common_dev.c | 31 ++- >> lib/librte_eal/common/eal_common_pci.c | 226 +++++++++------- >> lib/librte_eal/common/include/rte_bus.h | 243 +++++++++++++++++ >> lib/librte_eal/common/include/rte_common.h | 18 ++ >> lib/librte_eal/common/include/rte_dev.h | 36 +-- >> lib/librte_eal/common/include/rte_pci.h | 11 +- >> lib/librte_eal/linuxapp/eal/Makefile | 1 + >> lib/librte_eal/linuxapp/eal/eal.c | 51 +++- >> lib/librte_eal/linuxapp/eal/eal_pci.c | 298 -------------------- >> lib/librte_ether/rte_ethdev.c | 36 ++- >> lib/librte_ether/rte_ethdev.h | 6 +- >> 17 files changed, 1262 insertions(+), 478 deletions(-) >> create mode 100644 bus/Makefile >> create mode 100644 bus/pci/Makefile >> create mode 100644 bus/pci/linuxapp/pci_bus.c >> create mode 100644 bus/pci/linuxapp/pci_bus.h >> create mode 100644 lib/librte_eal/common/eal_common_bus.c >> create mode 100644 lib/librte_eal/common/include/rte_bus.h >> >> -- >> 2.7.4 >> > - Shreyansh
