From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f51.google.com (mail-pa0-f51.google.com [209.85.220.51]) by dpdk.org (Postfix) with ESMTP id CDFAA91F8 for ; Wed, 7 Sep 2016 20:39:55 +0200 (CEST) Received: by mail-pa0-f51.google.com with SMTP id cm16so8822449pac.0 for ; Wed, 07 Sep 2016 11:39:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=Cd7SG3nrIR1bA5ltjzxDanFIfSUELRQG+Yj7WSuC0XY=; b=kgfkRz71lCpHA7pNVSCzGsss4Zzqmpu8BpNuzARLsvZgs1Cow8W53fcd6pHdcAMVMH lPfnkS05XyyrGQb3LtUqUYJgmDH5qR9ihQVpN6FuUjtq1ekbmqMWJvlxhazGjTHj8z+v P0uHqJ9oK7vRkF0SirEKhRnsRKiFPpsgrx6Stbc+VVfY6EYZYadxyNdC89/fYMHdPOJa Md+B/wRprNTyiclwvwgHshyTzN/E4G4hj702U7MgQotxvlqTmqbNn/L3tzekqOkw3zSF qFYG/0vLJasSuprHIve9k/9RQb/RVtkCEi3cSbExh0PxaQY1upVQxZac96lI9Sx50JM7 FDwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=Cd7SG3nrIR1bA5ltjzxDanFIfSUELRQG+Yj7WSuC0XY=; b=G2bOgtZHHKn5GrnHJrir3nJ+9UW1sVqnsg6NbrAaqnHMAtJZfaej3BaX1pipme4oLL HIqn8CBY+y5xwfiXj2QzrfyC3WINchhnI9uK5tQP+8MUNvJ/5EG+xGEIqUJhXve6xjrx zMfDqcYbjNVRt5S5VbC3pNIqx8vQnWYj1qS4wHMirGaLkXKlRT1pL9GpzHN+2lQQzCJV y6eAetj9HTcX9tw0Y/7/E2p9xkv3oM7hpuiTzcJbAPlfBhZXFAqmRIvWGb/RWu/Ir6/Z pXM2z+yL98LNLVoWNrRRHHJ3qAe50VmNW5Ua/2IQGQwpb6quBbv8Sw7RLC6ywR7ZQqXZ /s3w== X-Gm-Message-State: AE9vXwO7g6APxcWC1Ai8wN8+sd+w+T0F9ue/zDgdZqZPhH3H8hvys5SP+I6D03RzOPmfFw== X-Received: by 10.66.75.4 with SMTP id y4mr3579540pav.139.1473273594854; Wed, 07 Sep 2016 11:39:54 -0700 (PDT) Received: from xeon-e3 (static-50-53-69-251.bvtn.or.frontiernet.net. [50.53.69.251]) by smtp.gmail.com with ESMTPSA id m5sm50731657paw.40.2016.09.07.11.39.54 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 07 Sep 2016 11:39:54 -0700 (PDT) Date: Wed, 7 Sep 2016 11:40:04 -0700 From: Stephen Hemminger To: Shreyansh Jain Cc: , Message-ID: <20160907114004.4a218155@xeon-e3> In-Reply-To: <1473257297-7221-1-git-send-email-shreyansh.jain@nxp.com> References: <1466510566-9240-1-git-send-email-shreyansh.jain@nxp.com> <1473257297-7221-1-git-send-email-shreyansh.jain@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v9 00/25] Introducing rte_driver/rte_device generalization X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Sep 2016 18:39:56 -0000 On Wed, 7 Sep 2016 19:37:52 +0530 Shreyansh Jain wrote: > Based on master (e22856313) > > Background: > =========== > > It includes two different patch-sets floated on ML earlier: > * Original patch series is from David Marchand [1], [2]. > `- This focused mainly on PCI (PDEV) part > `- v7 of this was posted by me [8] in August/2016 > * Patch series [4] from Jan Viktorin > `- This focused on VDEV and rte_device integration > > Introduction: > ============= > > This patch series introduces a generic device model, moving away from PCI > centric code layout. Key change is to introduce rte_driver/rte_device > structures at the top level which are inherited by > rte_XXX_driver/rte_XXX_device - where XXX belongs to {pci, vdev, soc (in > future),...}. > > Key motivation for this series is to move away from PCI centric design of > EAL to a more hierarchical device model - pivoted around a generic driver > and device. Each specific driver and device can inherit the common > properties of the generic set and build upon it through driver/device > specific functions. > > Earlier, the EAL device initialization model was: > (Refer: [3]) > > -- > Constructor: > |- PMD_DRIVER_REGISTER(rte_driver) > `- insert into dev_driver_list, rte_driver object > > rte_eal_init(): > |- rte_eal_pci_init() > | `- scan and fill pci_device_list from sysfs > | > |- rte_eal_dev_init() > | `- For each rte_driver in dev_driver_list > | `- call the rte_driver->init() function > | |- PMDs designed to call rte_eth_driver_register(eth_driver) > | |- eth_driver have rte_pci_driver embedded in them > | `- rte_eth_driver_register installs the > | rte_pci_driver->devinit/devuninit callbacks. > | > |- rte_eal_pci_probe() > | |- For each device detected, dev_driver_list is parsed and matching is > | | done. > | |- For each matching device, the rte_pci_driver->devinit() is called. > | |- Default map is to rte_eth_dev_init() which in turn creates a > | | new ethernet device (eth_dev) > | | `- eth_drv->eth_dev_init() is called which is implemented by > `--| individual PMD drivers. > > -- > > The structure of driver looks something like: > > +------------+ ._____. > | rte_driver <-----| PMD |___ > | .init | `-----` \ > +----.-------+ | \ > `-. | What PMD actually is > \ | | > +----------v----+ | > | eth_driver | | > | .eth_dev_init | | > +----.----------+ | > `-. | > \ | > +------------v---+ > | rte_pci_driver | > | .pci_devinit | > +----------------+ > > and all devices are part of a following linked lists: > - dev_driver_list for all rte_drivers > - pci_device_list for all devices, whether PCI or VDEV > > > From the above: > * a PMD initializes a rte_driver, eth_driver even though actually it is a > pci_driver > * initialization routines are passed from rte_driver->pci_driver->eth_driver > even though they should ideally be rte_eal_init()->rte_pci_driver() > * For a single driver/device type model, this is not necessarily a > functional issue - but more of a design language. > * But, when number of driver/device type increase, this would create problem > in how driver<=>device links are represented. > > Proposed Architecture: > ====================== > > A nice representation has already been created by David in [3]. Copying that > here: > > +------------------+ +-------------------------------+ > | | | | > | rte_pci_device | | rte_pci_driver | > | | | | > +-------------+ | +--------------+ | | +---------------------------+ | > | | | | | | | | | | > | rte_eth_dev +---> rte_device +-----> rte_driver | | > | | | | char name[] | | | | char name[] | | > +-------------+ | | | | | | int init(rte_device *) | | > | +--------------+ | | | int uninit(rte_device *) | | > | | | | | | > +------------------+ | +---------------------------+ | > | | > +-------------------------------+ > > - for ethdev on top of vdev devices > > +------------------+ +-------------------------------+ > | | | | > | drv specific | | rte_vdev_driver | > | | | | > +-------------+ | +--------------+ | | +---------------------------+ | > | | | | | | | | | | > | rte_eth_dev +---> rte_device +-----> rte_driver | | > | | | | char name[] | | | | char name[] | | > +-------------+ | | | | | | int init(rte_device *) | | > | +--------------+ | | | int uninit(rte_device *) | | > | | | | | | > +------------------+ | +---------------------------+ | > | | > | int priv_size | > | | > +-------------------------------+ > > Representing from above, it would be: > > +--------------+ > | rte_driver | > | name | > | | > +------^-------+ pci_driver_list > | / vdev_driver_list > `---. <> / / > |\____________/_______ / > | / \ / > +-----------/-----+ +--------/---------+ > | rte_pci_driver | | rte_vdev_driver | > | pci_devinit() | | vdev_devinit() | > | pci_devuninit()| | vdev_devuninit()| > | | | | > +-----------------+ +------------------+ > > > +--------------+ > | rte_device | > | name | > | | > +------^-------+ pci_device_list > | / xxx_device_list > `---. <> / / > |\____________/________ / > | / \ / > +-----------/-----+ +--------/---------+ > | rte_pci_device | | rte_xxx_device | > | | | | > | | | | > | | | | > +-----------------+ +------------------+ > > * Each driver type has its own structure which derives from the generic > rte_driver structure. > \- Each driver type maintains its own list, at the same time, rte_driver > list also exists - so that *all* drivers can be looped on, if required. > * Each device, associated with one or more drivers, has its own type > derived from rte_device > \- Each device _may_ maintain its own list (for example, in current > implementation, vdev is not maintaining it). > > ==Introducing a new device/driver type implies== > - creating their own rte_.h file which contains the device/driver > definitions. > - defining the DRIVER_REGISTER_XXX helpers > > ==Hotplugging Support== > - devices should be able to support attach/detach operations. > - Earlier these functions were part of ethdev. They have been moved to eal > to be more generic. > > This patch is part of larger aim to: > > --------------------+ > eth_driver (PMD) |-------------> rte_driver > crypto_driver (PMD) | ^ > | | > --------------------+ | > / > +-----------------------/+ > | rte_pci_driver | > | rte_vdev_driver | > | rte_soc_driver | > | rte_xxx_driver | > > Where PMD devices (rte_eth_dev, rte_cryptodev_dev) are connected to their > drivers rather than explicitly inheriting type specific driver (PCI, etc). > > > About the Patches: > ================== > > There are a large number of patches for this - primarily because the changes > are quite varied and keeping them logically separate yet compilable is > important. Most of the patches are small and those which are large touch the > drivers (PMDs) to accommodate the structure changes: > > - Patches 0001~0003 are for introducing the container_of function (so that > rte_device can be obtained from rte_pci_device, for example), and > removing unused code. > - Patches 0004~0007 just perform the ground work for enabling change from > rte_driver/eth_driver based PMDs to rte_xxx_driver based PMDs > - In patch 0008, all the pdev PMDs are changed to support rte_pci_driver ( > including cryptodev, which is eventually generalized with PCI) > - Patch 0009~0010 merge the crypto and pci functions for registration and > naming. > - Patches 0011~0014 deal with hotplugging - hotplug no more invokes scan of > complete bus and has been generalized into EAl from ethdev. > - Patches 0015 introduces vdev init/uninit into separate C units and > enables its compilation. Patch 0016~0017 build on it and remove the > remaining legacy support for vdev/pdev distinctions. > - Patches 0018~0022 enable the vdev drivers to register using the > DRIVER_REGISTER_* operations, and remove their rte_driver->init() > - Patch 0023 enables the rte_driver registration into a common driver > linked list. > - Patches 0024~0025 introduce the rte_device, a generalization of > rte_xxx_device, and associated operation of creating rte_device linked > list. It also enables the drivers to use rte_device.name/numa_node > members rather than rte_xxx_device specific members. > > Notes: > ====== > > * Some sign-off were already provided by Jan on the original v5; But, as a > large number of merges have been made, I am leaving those out just in case > it is not sync with initial understanding. > > * This might not be in-sync with pmdinfogen as PMD_REGISTER_DRIVER is > removed [7]. > > Future Work/Pending: > =================== > - Presently eth_driver, rte_eth_dev are not aligned to the rte_driver/ > rte_device model. eth_driver still is a PCI specific entity. This > has been highlighted by comments from Ferruh in [9]. > - cryptodev driver too still remains to be normalized over the rte_driver > model > - Some variables, like drv_name (as highlighted by Ferruh), are getting > duplicated across rte_xxx_driver/device and rte_driver/device. > > References: > =========== > > [1] http://dpdk.org/ml/archives/dev/2016-January/032387.html > [2] http://dpdk.org/ml/archives/dev/2016-April/037686.html > [3] http://dpdk.org/ml/archives/dev/2016-January/031390.html > [4] http://dpdk.org/ml/archives/dev/2016-July/043645.html > [5] http://dpdk.org/ml/archives/dev/2016-June/042439.html > [6] http://dpdk.org/ml/archives/dev/2016-June/042444.html > [7] http://dpdk.org/ml/archives/dev/2016-July/043172.html > [8] http://dpdk.org/ml/archives/dev/2016-August/044941.html > [9] http://dpdk.org/ml/archives/dev/2016-August/045947.html > > Changes since v8: > - Some review comments from Ferruh Yigit & Reshma Pattan have been fixed. > = Though changes in mlx4/mlx5/szedata2 have been done, I am still unable to > verify those in absence of a proper environment at my end. > = Comment from Ferruh for eth_driver, drv_name are not fixed yet. > - Added a 'Future work' section in Covering letter > > Changes since v7: > - Rebase over master (e22856313fff2) > - Merge the patch series by David [1][2] and Jan [4] into a single set > hereafter, PCI and VDEV, both are re-factored for rte_device/driver model > > Changes since v6: > - rebase over 16.07 (b0a1419) > - DRIVER_REGISTER_PCI macro is now passed pci_drv rather than drv > - review comments regarding missing information in log messages > - new API additions to 16.11 map objects > - review comment in [5] and [7] are not included in this series. > > Changes since v5: > - Rebase over master (11c5e45d8) > - Rename RTE_EAL_PCI_REGISTER helper macro to DRIVER_REGISTER_PCI to be in > sync > with DRIVER_REGISTER_PCI_TABLE. [Probably, in future, both can be merged] > - Modifications to bnxt and thunderx driver PMD registration files for > using the simplified PCI device registration helper macro > > Changes since v4: > - Fix compilation issue after rebase on HEAD (913154e) in previous series > - Retain rte_eth_dev_get_port_by_name and rte_eth_dev_get_name_by_port which > were removed by previous patchset. These are being used by pdump library > > Changes since v3: > - rebase over HEAD (913154e) > - Update arguments to RTE_EAL_PCI_REGISTER macro as per Jan's suggestion > - modify qede driver to use RTE_EAL_PCI_REGISTER > - Argument check in hotplug functions > > Changes since v2: > - rebase over HEAD (d76c193) > - Move SYSFS_PCI_DRIVERS macro to rte_pci.h to avoid compilation issue > > Changes since v1: > - rebased on HEAD, new drivers should be okay > - patches have been split into smaller pieces > - RTE_INIT macro has been added, but in the end, I am not sure it is useful > - device type has been removed from ethdev, as it was used only by hotplug > - getting rid of pmd type in eal patch (patch 5 of initial series) has been > dropped for now, we can do this once vdev drivers have been converted > > > Shreyansh Jain (25): > eal: define macro container_of > eal: remove duplicate function declaration > pci: no need for dynamic tailq init > crypto: no need for a crypto pmd type > drivers: align pci driver definitions > eal: introduce init macros > driver: init/uninit common wrappers for PCI drivers > drivers: convert all pdev drivers as pci drivers > driver: Remove driver register callbacks for crypto/net > eal/pci: Helpers for device name parsing/update > ethdev: do not scan all pci devices on attach > eal: add hotplug operations for pci and vdev > ethdev: convert to eal hotplug > ethdev: get rid of device type > eal: extract vdev infra > eal: Remove PDEV/VDEV unused code > drivers: convert PMD_VDEV drivers to use rte_vdev_driver > eal: move init/uninit to rte_vdev_driver > eal: remove PMD_DRIVER_REGISTER and unused pmd_types > eal: rte_pci.h includes rte_dev.h > eal: rename and move rte_pci_resource > eal/pci: inherit rte_driver by rte_pci_driver > eal: call rte_eal_driver_register > eal: introduce rte_device > eal/pci: Create rte_device list and fallback on its members > > app/test/test_pci.c | 10 +- > app/test/virtual_pmd.c | 8 +- > drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 7 +- > drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 7 +- > drivers/crypto/kasumi/rte_kasumi_pmd.c | 7 +- > drivers/crypto/null/null_crypto_pmd.c | 7 +- > drivers/crypto/qat/qat_qp.c | 2 +- > drivers/crypto/qat/rte_qat_cryptodev.c | 18 +- > drivers/crypto/snow3g/rte_snow3g_pmd.c | 7 +- > drivers/net/af_packet/rte_eth_af_packet.c | 11 +- > drivers/net/bnx2x/bnx2x_ethdev.c | 36 +-- > drivers/net/bnx2x/bnx2x_rxtx.c | 3 +- > drivers/net/bnxt/bnxt_ethdev.c | 17 +- > drivers/net/bonding/rte_eth_bond_api.c | 2 +- > drivers/net/bonding/rte_eth_bond_pmd.c | 9 +- > drivers/net/cxgbe/cxgbe_ethdev.c | 25 +-- > drivers/net/cxgbe/cxgbe_main.c | 2 +- > drivers/net/cxgbe/sge.c | 7 +- > drivers/net/e1000/em_ethdev.c | 17 +- > drivers/net/e1000/igb_ethdev.c | 41 +--- > drivers/net/ena/ena_ethdev.c | 20 +- > drivers/net/enic/enic_ethdev.c | 24 +- > drivers/net/fm10k/fm10k_ethdev.c | 30 +-- > drivers/net/i40e/i40e_ethdev.c | 31 +-- > drivers/net/i40e/i40e_ethdev_vf.c | 26 +-- > drivers/net/i40e/i40e_fdir.c | 2 +- > drivers/net/ixgbe/ixgbe_ethdev.c | 48 +--- > drivers/net/mlx4/mlx4.c | 21 +- > drivers/net/mlx5/mlx5.c | 22 +- > drivers/net/mpipe/mpipe_tilegx.c | 18 +- > drivers/net/nfp/nfp_net.c | 28 +-- > drivers/net/null/rte_eth_null.c | 11 +- > drivers/net/pcap/rte_eth_pcap.c | 11 +- > drivers/net/qede/qede_ethdev.c | 42 +--- > drivers/net/ring/rte_eth_ring.c | 11 +- > drivers/net/szedata2/rte_eth_szedata2.c | 29 +-- > drivers/net/thunderx/nicvf_ethdev.c | 21 +- > drivers/net/vhost/rte_eth_vhost.c | 11 +- > drivers/net/virtio/virtio_ethdev.c | 28 +-- > drivers/net/virtio/virtio_pci.c | 5 +- > drivers/net/virtio/virtio_user_ethdev.c | 10 +- > drivers/net/vmxnet3/vmxnet3_ethdev.c | 27 +-- > drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +- > drivers/net/xenvirt/rte_eth_xenvirt.c | 11 +- > examples/ip_pipeline/init.c | 22 -- > lib/librte_cryptodev/rte_cryptodev.c | 71 ++---- > lib/librte_cryptodev/rte_cryptodev.h | 2 - > lib/librte_cryptodev/rte_cryptodev_pmd.h | 45 ++-- > lib/librte_cryptodev/rte_cryptodev_version.map | 8 +- > lib/librte_eal/bsdapp/eal/Makefile | 1 + > lib/librte_eal/bsdapp/eal/eal_pci.c | 54 ++++- > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 7 + > lib/librte_eal/common/Makefile | 2 +- > lib/librte_eal/common/eal_common_dev.c | 95 ++++---- > lib/librte_eal/common/eal_common_pci.c | 34 ++- > lib/librte_eal/common/eal_common_vdev.c | 106 +++++++++ > lib/librte_eal/common/eal_private.h | 20 +- > lib/librte_eal/common/include/rte_common.h | 21 ++ > lib/librte_eal/common/include/rte_dev.h | 77 +++++-- > lib/librte_eal/common/include/rte_eal.h | 3 + > lib/librte_eal/common/include/rte_pci.h | 51 +++-- > lib/librte_eal/common/include/rte_tailq.h | 4 +- > lib/librte_eal/common/include/rte_vdev.h | 96 ++++++++ > lib/librte_eal/linuxapp/eal/Makefile | 1 + > lib/librte_eal/linuxapp/eal/eal.c | 1 + > lib/librte_eal/linuxapp/eal/eal_pci.c | 23 +- > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 10 + > lib/librte_ether/rte_ethdev.c | 280 +++++------------------- > lib/librte_ether/rte_ethdev.h | 40 ++-- > lib/librte_ether/rte_ether_version.map | 10 +- > 70 files changed, 791 insertions(+), 1025 deletions(-) > create mode 100644 lib/librte_eal/common/eal_common_vdev.c > create mode 100644 lib/librte_eal/common/include/rte_vdev.h > Overall I like to see the clean separation. Are you sure you removed as much as possible from PCI? I wonder of global PCI device list is needed at all if you now have list of all devices.