From: Shreyansh Jain <shreyansh.jain@nxp.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
David Marchand <david.marchand@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] Clarification for eth_driver changes
Date: Sat, 12 Nov 2016 17:44:57 +0000 [thread overview]
Message-ID: <DB5PR0401MB2054B1B4C5EC1BA8412E8B4090BA0@DB5PR0401MB2054.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <1a19615c-1121-fed4-b34f-aa0f4b654085@intel.com>
Hello Ferruh,
(Please ignore if line wrappings are not correct. Using a possibly
unconfigured mail client).
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Saturday, November 12, 2016 12:46 AM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>; David Marchand
> <david.marchand@6wind.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] Clarification for eth_driver changes
>
> On 11/10/2016 11:05 AM, Shreyansh Jain wrote:
> > Hello David,
> >
> > On Thursday 10 November 2016 01:46 PM, David Marchand wrote:
> >> Hello Shreyansh,
> >>
> >> On Thu, Nov 10, 2016 at 8:26 AM, Shreyansh Jain <shreyansh.jain@nxp.com>
> wrote:
> >>> I need some help and clarification regarding some changes I am doing to
> >>> cleanup the EAL code.
> >>>
> >>> There are some changes which should be done for eth_driver/rte_eth_device
> >>> structures:
> >>>
> >>> 1. most obvious, eth_driver should be renamed to rte_eth_driver.
> >>> 2. eth_driver currently has rte_pci_driver embedded in it
> >>> - there can be ethernet devices which are _not_ PCI
> >>> - in which case, this structure should be removed.
> >>
> >> Do we really need to keep a eth_driver ?
> >
> > No. As you have rightly mentioned below (as well as in your Jan'16
> > post), it is a mere convenience.
>
> Isn't it good to separate the logic related which bus device connected
> and what functionality it provides. Because these two can be flexible:
Indeed. The very idea of a Bus model is to make a hierarchy which allows
for pluggability/flexibility in terms of devices being used. But, until now I
have only considered placement hierarchy and not functional hierarchy. (more
below)
>
> device -> virtual_bus -> ethernet_functionality
> device -> pci_bus -> crypto_functionality
> device -> x_bus -> y_function
>
Ok.
>
> what about:
>
> create generic bus driver/device and all eal level deal with generic
> bus. different buses inherit from generic bus logic
From what I had in mind: (and very much similar to what you have already
mentioned in this email)
- 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()
-- There is a problem of making sure the constructor for bus registration is
executed before drivers. Probably by putting the bus code within lib*
- Each registered bus is part of a doubly list.
- Each device inherits rte_bus
- Each driver inherits rte_bus
- Existing Device and Drivers lists would be moved into rte_bus
(more below)
>
> create generic functionality device/driver and pmd level deal with
> these. different functionalities inherit from generic functionality logic
>
> and use rte_device/rte_driver as glue logic for all these.
>
> This makes easy to add new bus or functionality device/drivers without
> breaking existing logic.
>
>
> Something like this:
>
> struct rte_device {
> char *name;
> struct rte_driver *drv;
> struct rte_bus_device *bus_dev;
> struct rte_funcional_device *func_dev;
> *devargs
> }
>
> struct rte_bus_device {
> struct rte_device *dev;
> /* generic bus device */
> }
This is where you lost me. From what I understood from your text:
(CMIIW)
- Most abstract class is 'rte_device'.
- A bus is a device. So, it points to a rte_device
- But, a rte_device belongs to a bus, so it points to a rte_bus_device.
Isn't that contradictory?
This is how I view the physical layout of devices on which DPDK works:
+---------+ +----------+
|Driver 1A| |Driver 2B |
|servicing| |servicing | (*)
|Bus A | |Bus B |
+---------+ +----------+
\ /
+---------+ +-------+
| bus A |---| bus B |--- ...
+---------+ +-------+
/ \ \ \
/ \_ \ \
+---------+ / / \
| device 1| / +--------+ \
| on Bus A| / |Device 3| |
+---------+ / |on bus B| |
+---------+ +--------+ |
| device 2| +--------+
| on Bus A| |Device 4|
+---------+ |on bus B|
+--------+
(*) Multiple drivers servicing a Bus.
Now, if we introduce the abstraction for functionality (assuming net/crypto) as
two functionalities currently applicable:
+---------+ +----------+
|Driver 1A| |Driver 2B |
|servicing| |servicing | (*)
|Bus A | |Bus B |
+---------' +---.------+
\ /
+---------+ +-------+
| bus A |---| bus B |--- ...
+---------+ +-------+
/ \ \ \
/ \_ \ \
+---------' / / \
| device 1| / +--------' \
| on Bus A| / |Device 3| |
+---------+ / |on bus B| |
/ +---------' +-|------+ |
| | device 2| / +-------'+
| | on Bus A| / |Device 4|
\ +--|------+ _____/ |on bus B|
\ \_____ / +--------+
| .--\' /
| / \ ___/
+-'----'-+ +'------'+
|Func X | |Func Y |
|(Net) | |(Crypto)|
+--------| +--------+
So that means, a device would be a 'net' or 'crypto' device bases on the
Functionality it attaches to.
From a physical layout view, is that correct understanding of your argument?
>
> struct rte_pci_device {
> struct rte_bus_device bus_dev;
> /* generic pci bus */
> }
>
> struct rte_vdev_device {
> struct rte_bus_device bus_dev;
> /* generic vdev bus */
> }
>
> struct rte_funcional_device {
> struct rte_device *dev;
> }
I understand your point of 'pluggable' functionality. It would be helpful if
same driver would like to move between being a crypto and net. But is that a
plausible use case for DPDK right now?
To me, it seems as one more layer of redirection/abstraction.
This is what the view I have as of now:
__ rte_bus_list
/
+----------'---+
|rte_bus |
| driver_list |
| device_list |
| scan |
| match |
| remove |
| ..some refcnt|
+--|------|----+
_________/ \_________
+--------/----+ +-\--------------+
|rte_device | |rte_driver |
| rte_bus | | rte_bus |
| flags (3*) | | probe (1*) |
| init | | remove |
| uninit | | ... |
+---||--------+ | 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.
So, unlike your model, rte_bus remains the topmost class which is neither a
device, not a driver. It is just a class.
Further, as specific information exists in each specific device and driver,
that is not generalized.
>
> struct rte_eth_device {
> struct rte_funcional_device func_dev;
> /* generic eth device */
> }
>
> struct rte_crypto_device {
> struct rte_funcional_device func_dev;
> /* generic crypto device */
> }
>
I tried thinking of this breakup but I couldn't understand what common things
a rte_functional_device would contain except init/uninit (similar to what you
have also mentioned below) which can be part of rte_device itself.
>
> struct rte_driver {
> char *name;
> struct rte_device *dev;
> struct rte_bus_driver *bus_drv;
> struct rte_funcional_driver *func_drv;
> }
>
> struct rte_bus_driver {
> struct rte_driver *drv;
> rte_bus_probe_t *probe(dev, drv);
> rte_bus_remove_t *remove(dev);
> /* generic bus driver */
> }
I still think a bus is neither a device, nor a driver. Yes, if we draw
physical analogy, buses are indeed devices on PCB - but they are not anything
functional with respect to an application view. They exist only to provide way
for application to understand device layout.
>
> struct rte_pci_driver {
> struct rte_bus_driver bus_drv;
> *id_table;
> /* generic pci bus */
> }
>
> struct rte_vdev_driver {
> struct rte_bus_driver bus_drv;
> /* generic vdev bus */
> }
>
> struct rte_funcional_driver {
> struct rte_driver *drv;
> rte_funcional_init_t *init;
> rte_funcional_uninit_t *uninit;
> }
>
> struct rte_eth_driver {
> struct rte_funcional_driver func_drv;
> /* generic eth driver */
> }
>
> struct rte_crypto_driver {
> struct rte_funcional_driver func_drv;
> /* generic crypto driver */
> }
>
> pci_scan_one()
> {
> dev = create();
> pci_dev = create();
>
> dev->bus_dev = pci_dev;
> pci_dev->bus_dev.dev = dev;
>
> insert(bus_dev_list);
> }
>
> register_drv(drv)
> {
> insert(bus_drv_list)
> insert(func_drv_list)
> insert(drv_list)
> }
>
> rte_eal_bus_probe()
> for bus_dev in bus_dev_list
> bus_probe_all_drivers(bus_dev)
> for bus_drv in bus_drv_list
> bus_probe_one_driver(bus_drv, bus_dev)
> bus_dev->dev->drv = bus_drv->drv;
> bus_drv->drv->dev = bus_dev->dev;
> probe(drv, dev)
>
Agree.
> probe(drv, dev)
> {
> dev->func_dev = create();
In my case, it would be creating a rte_device; rte_pci_dev in case of
PCI probe, pointing back to rte_device, rte_bus of PCI type.
> func_dev->dev = dev;
>
> func_drv = drv->func_drv;
> func_drv->init(func_dev);
Effectively, it would be rte_device->init();
> }
>
> eht_init(func_dev)
> {
> eth_dev = (struct rte_eth_dev)func_dev;
> drv = func_dev->dev->drv;
> }
>
>
I am not against what you have stated. Creating a functional device is just
one more layer of abstraction in my view. Mostly abstraction/classification
makes life easier for applications to visualize underlying hierarchy. If this
serves a purpose, I am OK with that. At least right now, I think it would
only end up being like eth_driver which is just a holder.
__
Shreyansh
next prev parent reply other threads:[~2016-11-12 17:45 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-10 7:26 Shreyansh Jain
2016-11-10 7:51 ` Jianbo Liu
2016-11-10 8:03 ` Thomas Monjalon
2016-11-10 8:42 ` Shreyansh Jain
2016-11-10 8:58 ` Thomas Monjalon
2016-11-10 9:20 ` Jianbo Liu
2016-11-10 10:51 ` Stephen Hemminger
2016-11-10 11:07 ` Thomas Monjalon
2016-11-10 11:09 ` Shreyansh Jain
2016-11-10 8:38 ` Shreyansh Jain
2016-11-10 8:16 ` David Marchand
2016-11-10 11:05 ` Shreyansh Jain
2016-11-11 19:16 ` Ferruh Yigit
2016-11-12 17:44 ` Shreyansh Jain [this message]
2016-11-14 17:38 ` Ferruh Yigit
2016-11-16 5:09 ` Shreyansh Jain
2016-11-14 9:07 ` David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DB5PR0401MB2054B1B4C5EC1BA8412E8B4090BA0@DB5PR0401MB2054.eurprd04.prod.outlook.com \
--to=shreyansh.jain@nxp.com \
--cc=david.marchand@6wind.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).