From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id E4A6A388F for ; Thu, 17 Nov 2016 17:54:24 +0100 (CET) Received: by mail-wm0-f67.google.com with SMTP id m203so22957254wma.3 for ; Thu, 17 Nov 2016 08:54:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=VaEgUzNP6r1fV7eYgV3FSYTzUMUbWS3napq1J96SaGc=; b=yoUP0mya1AD/t1F7NmA4R6c9UT84nuRRh8JEQ69ff19XVTGrPNuCwTQuJiFr1klqGx AjzsCGu0Z/ALi62P9CQpK+92uS9n1rST442YYn+numEy+KRnQZI4Vnw9u1KQGGKIbsVR tRRysDTpHZ19VHJQf4fS/4PdxASAJTBIOCiJX9kEkB5dN97VTdUzHoCrOKI7bCPd0fnI RXOh2MCe64QtVwcOUnGiggpFIunpBlLIlUVUDj8WKb2I8ehUOk56FxQ3/HZLcndr2f7c YOzFpr0aim8sHO1oAsPh0YdxdSBapXNUe/baxM3r1T4KVTqRiKc3hg55ldnbpaJOjlLH WZ/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=VaEgUzNP6r1fV7eYgV3FSYTzUMUbWS3napq1J96SaGc=; b=HeBqtsnCpSB0LgXj29uzbJP8/57GFX7yuFjMcs1fsQ+IBy/61sd+th8Q/IogJtVHbH Tyue9UswTXD/CCnZqAcdMmZHXJ+9hVAIlr8IJZYpl0XOyGjLQUfNF7yPwLyB4Df8vE0C ow8DMckhcPgA2ZMkmdalIJYPmi0WX7JlWZ1NxXby4ulIGGgfBb7aR5KFBRF99SNedHIV xxqQVTmRndb3JdJgaI8lVGNHWBAHCxGb5CYhbcmbXLHDMeAc6EJsKoiWP/EAe9wkKGkc 1i7jGGuR89mUhlTjmLqfuv5v1stkOf72yPDTp8F1uqCmucRA481Qe2/Zs2T1BWxHg0/E 9bdg== X-Gm-Message-State: AKaTC00Oy0Zq6BmYBsvCbmZukmDHJzhqof4d76E+emEKl7POgs1guADR9FK6ehLRKF3Qfn6AY4qKjx2lqJlnIA== X-Received: by 10.194.127.104 with SMTP id nf8mr2761356wjb.39.1479401664273; Thu, 17 Nov 2016 08:54:24 -0800 (PST) MIME-Version: 1.0 Sender: jblunck@gmail.com Received: by 10.28.191.8 with HTTP; Thu, 17 Nov 2016 08:54:23 -0800 (PST) In-Reply-To: <944308a3-8fb8-2c4d-8f0f-04ae48bf334c@nxp.com> References: <1479360605-20558-1-git-send-email-shreyansh.jain@nxp.com> <944308a3-8fb8-2c4d-8f0f-04ae48bf334c@nxp.com> From: Jan Blunck Date: Thu, 17 Nov 2016 17:54:23 +0100 X-Google-Sender-Auth: l9iAcKcWE6w62_w7BMLOAaehvo0 Message-ID: To: Shreyansh Jain Cc: David Marchand , dev@dpdk.org Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [RFC PATCH 0/6] Restructure EAL device model for bus support 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: Thu, 17 Nov 2016 16:54:25 -0000 On Thu, Nov 17, 2016 at 2:08 PM, Shreyansh Jain wrote: > On Thursday 17 November 2016 05:25 PM, Jan Blunck wrote: >> >> On Thu, Nov 17, 2016 at 6:29 AM, Shreyansh Jain >> 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, | | >> | 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. > Yes, either have everything PCI related stuff in drivers or in librte_eal (or librte_eal_pci). I don't believe it is required to have two different locations for the rte_pci_bus and the rte_pci_* functions that are now in librte_eal. >> >> >>> 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. > Yes, that is fine too. > >> >> >>> >>> === 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