[dpdk-dev] Clarification for eth_driver changes
On Monday 14 November 2016 11:08 PM, Ferruh Yigit wrote: [...] > What I was thinking is: > > rte_device/driver are not abstract classes. > > rte_bus device/driver is an abstract class and any bus inherited from > this class. > rte_func device/driver is and abstract class and eth/crypto inherited > from this class. > > eal layer only deal with rte_bus > pmd's only deal with functional device/driver > > but still, it is required to know device <-> driver, and functional <-> > bus, relations. rte_dev/rte_driver are to provide this links. > > But yes this add extra layer and with second thought I am not sure if it > is really possible to separate bus and functionality, this was just an > idea .. [...] I understand your point. It would really nice if we can achieve that level pluggable-ness where drivers would be able to choose a 'profile' - where 'profiles' are like net/crypto etc. In your text, profile==functionality. Maybe once the basic model is in place, we can revisit this idea. - Shreyansh
[dpdk-dev] Clarification for eth_driver changes
On 11/12/2016 5:44 PM, Shreyansh Jain wrote:
> Hello Ferruh,
>
> (Please ignore if line wrappings are not correct. Using a possibly
> unconfigured mail client).
>
>> -Original Message-
>> From: Ferruh Yigit [mailto:ferruh.yigit at intel.com]
>> Sent: Saturday, November 12, 2016 12:46 AM
>> To: Shreyansh Jain ; David Marchand
>>
>> Cc: dev at 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
>> 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?
What I was thinking is:
rte_device/driver are not abstract classes.
rte_bus device/driver is an abstract class and any bus inherited from
this class.
rte_func device/driver is and abstract class and eth/crypto inherited
from this class.
eal layer only deal with rte_bus
pmd's only deal with function
[dpdk-dev] Clarification for eth_driver changes
On Fri, Nov 11, 2016 at 8:16 PM, Ferruh Yigit wrote: > On 11/10/2016 11:05 AM, Shreyansh Jain wrote: >> On Thursday 10 November 2016 01:46 PM, David Marchand wrote: >>> 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: > > device -> virtual_bus -> ethernet_functionality > device -> pci_bus -> crypto_functionality > device -> x_bus -> y_function "a device is linked to a bus" (fine) "a bus knows what a device does" (?!) Not sure I follow you. -- David Marchand
[dpdk-dev] Clarification for eth_driver changes
Hello Ferruh, (Please ignore if line wrappings are not correct. Using a possibly unconfigured mail client). > -Original Message- > From: Ferruh Yigit [mailto:ferruh.yigit at intel.com] > Sent: Saturday, November 12, 2016 12:46 AM > To: Shreyansh Jain ; David Marchand > > Cc: dev at 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 > 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
[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
>> 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:
device -> virtual_bus -> ethernet_functionality
device -> pci_bus -> crypto_functionality
device -> x_bus -> y_function
what about:
create generic bus driver/device and all eal level deal with generic
bus. different buses inherit from generic bus logic
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 */
}
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;
}
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 */
}
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 */
}
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)
probe(drv, dev)
{
dev->func_dev = create();
func_dev->dev = dev;
func_drv = drv->func_drv;
func_drv->init(func_dev);
}
eht_init(func_dev)
{
eth_dev = (struct rte_eth_dev)func_dev;
drv = func_dev->dev->drv;
}
[dpdk-dev] Clarification for eth_driver changes
Hi Thomas, On 10 November 2016 at 16:58, Thomas Monjalon wrote: > 2016-11-10 14:12, Shreyansh Jain: >> On Thursday 10 November 2016 01:33 PM, Thomas Monjalon wrote: >> > 2016-11-10 15:51, Jianbo Liu: >> >> On 10 November 2016 at 15:26, Shreyansh Jain >> >> wrote: >> >>> This is what the current outline of eth_driver is: >> >>> >> >>> ++ >> >>> | eth_driver | >> >>> | +-+| >> >>> | | rte_pci_driver || >> >>> | | +--+|| >> >>> | | | rte_driver ||| >> >>> | | | name[] ||| >> >>> | | | ... ||| >> >>> | | +--+|| >> >>> | | .probe || >> >>> | | .remove|| >> >>> | | ...|| >> >>> | +-+| >> >>> | .eth_dev_init | >> >>> | .eth_dev_uninit | >> >>> ++ >> >>> >> >>> This is what I was thinking: >> >>> >> >>> +-++--+ >> >>> | rte_pci_driver ||eth_driver| >> >>> | +--+| _|_struct rte_driver *p | >> >>> | | rte_driver <---/ | .eth_dev_init| >> >>> | | ... ||| .eth_dev_uninit | >> >>> | | name||+--+ >> >>> | ||| >> >>> | +--+| >> >>> | | >> >>> +-+ >> >>> >> >>> ::Impact:: >> >>> Various drivers use the rte_pci_driver embedded in the eth_driver object >> >>> for >> >>> device initialization. >> >>> == They assume that rte_pci_driver is directly embedded and hence simply >> >>> dereference. >> >>> == e.g. eth_igb_dev_init() in drivers/net/e1000/igb_ethdev.c file >> >>> >> >>> With the above change, such drivers would have to access rte_driver and >> >>> then >> >>> perform container_of to obtain their respective rte_xxx_driver. >> >>> == this would be useful in case there is a non-PCI driver >> >>> >> >>> ::Problem:: >> >>> I am not sure of reason as to why eth_driver embedded rte_pci_driver in >> >>> first place - other than a convenient way to define it before PCI driver >> >>> registration. >> >>> >> >>> As all the existing PMDs are impacted - am I missing something here in >> >>> making the above change? >> >>> >> >> >> >> How do you know eth_driver->p is pointing to a rte_pci_driver or >> >> rte_soc_driver? >> >> Maybe you need to add a type/flag in rte_driver. >> > >> > Why do you need any bus information at ethdev level? >> >> AFAIK, we don't need it. Above text is not stating anything on that >> grounds either, I think. Isn't it? > > No, I was replying to Jianbo. > Anyway, David made a more interesting comment. Indeed, no need as I checked the code. It's not even a issue if using David's design. Thanks! Jianbo
[dpdk-dev] Clarification for eth_driver changes
On Thursday 10 November 2016 04:21 PM, Stephen Hemminger wrote: > I also think drv_flags should part of device not PCI. Most of the flags > there like link state support are generic. If it isn't changed for this > release will probably have to break ABI to fully support VMBUS > I didn't get your point. Currently drv_flags is in rte_pci_driver. You intend to say that it should be moved to rte_device? And, all the changes being discussed here are for 17.02. - Shreyansh
[dpdk-dev] Clarification for eth_driver changes
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 > 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. > As far as I can see, it is only a convenient wrapper for existing pci > drivers, but in the end it is just a pci_driver with ethdev context in > it that could be pushed to each existing driver. Indeed. My problem (or lack of understanding) is that all PMDs rely on it and I don't know in what all pattern they have envisioned using this model of ethdev->pci_dev, the initialization sequences. rte_device->init/uninit would settle most of those worries, I think. > > In my initial description > http://dpdk.org/ml/archives/dev/2016-January/031390.html, what I had > in mind was only having a rte_eth_device pointing to a generic > rte_device. Though I had read it (during the rte_device/driver series) but didn't remember it while posting this. I agree with your point of doing away with eth_driver. > If we need to invoke some generic driver ops from ethdev (I can only > see the ethdev hotplug api, maybe I missed something), then we would > go through rte_eth_device -> rte_device -> rte_driver. Agree with you. > The rte_driver keeps its own bus/private logic in its code, and no > need to expose a type. Agree. > > >> 3. Similarly, rte_eth_dev has rte_pci_device which should be replaced with >> rte_device. > > Yes, that's the main change for me. Indeed. This is a big change. It impacts a lot of EAL code. > > > Thanks. > Intent of this email was to know if I am missing something in assuming that eth_driver is actually not being used much. I will keep the comments from your email in mind while making changes. Thanks. _ Shreyansh
[dpdk-dev] Clarification for eth_driver changes
On 10 November 2016 at 15:26, Shreyansh Jain wrote: > Hello David, list, > > 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. > 3. Similarly, rte_eth_dev has rte_pci_device which should be replaced with > rte_device. > > This is what the current outline of eth_driver is: > > ++ > | eth_driver | > | +-+| > | | rte_pci_driver || > | | +--+|| > | | | rte_driver ||| > | | | name[] ||| > | | | ... ||| > | | +--+|| > | | .probe || > | | .remove|| > | | ...|| > | +-+| > | .eth_dev_init | > | .eth_dev_uninit | > ++ > > This is what I was thinking: > > +-++--+ > | rte_pci_driver ||eth_driver| > | +--+| _|_struct rte_driver *p | > | | rte_driver <---/ | .eth_dev_init| > | | ... ||| .eth_dev_uninit | > | | name||+--+ > | ||| > | +--+| > | | > +-+ > > ::Impact:: > Various drivers use the rte_pci_driver embedded in the eth_driver object for > device initialization. > == They assume that rte_pci_driver is directly embedded and hence simply > dereference. > == e.g. eth_igb_dev_init() in drivers/net/e1000/igb_ethdev.c file > > With the above change, such drivers would have to access rte_driver and then > perform container_of to obtain their respective rte_xxx_driver. > == this would be useful in case there is a non-PCI driver > > ::Problem:: > I am not sure of reason as to why eth_driver embedded rte_pci_driver in > first place - other than a convenient way to define it before PCI driver > registration. > > As all the existing PMDs are impacted - am I missing something here in > making the above change? > How do you know eth_driver->p is pointing to a rte_pci_driver or rte_soc_driver? Maybe you need to add a type/flag in rte_driver. > Probably, similar is the case for rte_eth_dev. > > - > Shreyansh
[dpdk-dev] Clarification for eth_driver changes
On Thursday 10 November 2016 01:33 PM, Thomas Monjalon wrote: > 2016-11-10 15:51, Jianbo Liu: >> On 10 November 2016 at 15:26, Shreyansh Jain >> wrote: >>> This is what the current outline of eth_driver is: >>> >>> ++ >>> | eth_driver | >>> | +-+| >>> | | rte_pci_driver || >>> | | +--+|| >>> | | | rte_driver ||| >>> | | | name[] ||| >>> | | | ... ||| >>> | | +--+|| >>> | | .probe || >>> | | .remove|| >>> | | ...|| >>> | +-+| >>> | .eth_dev_init | >>> | .eth_dev_uninit | >>> ++ >>> >>> This is what I was thinking: >>> >>> +-++--+ >>> | rte_pci_driver ||eth_driver| >>> | +--+| _|_struct rte_driver *p | >>> | | rte_driver <---/ | .eth_dev_init| >>> | | ... ||| .eth_dev_uninit | >>> | | name||+--+ >>> | ||| >>> | +--+| >>> | | >>> +-+ >>> >>> ::Impact:: >>> Various drivers use the rte_pci_driver embedded in the eth_driver object for >>> device initialization. >>> == They assume that rte_pci_driver is directly embedded and hence simply >>> dereference. >>> == e.g. eth_igb_dev_init() in drivers/net/e1000/igb_ethdev.c file >>> >>> With the above change, such drivers would have to access rte_driver and then >>> perform container_of to obtain their respective rte_xxx_driver. >>> == this would be useful in case there is a non-PCI driver >>> >>> ::Problem:: >>> I am not sure of reason as to why eth_driver embedded rte_pci_driver in >>> first place - other than a convenient way to define it before PCI driver >>> registration. >>> >>> As all the existing PMDs are impacted - am I missing something here in >>> making the above change? >>> >> >> How do you know eth_driver->p is pointing to a rte_pci_driver or >> rte_soc_driver? >> Maybe you need to add a type/flag in rte_driver. > > Why do you need any bus information at ethdev level? > AFAIK, we don't need it. Above text is not stating anything on that grounds either, I think. Isn't it? - Shreyansh
[dpdk-dev] Clarification for eth_driver changes
On Thursday 10 November 2016 01:21 PM, Jianbo Liu wrote: > On 10 November 2016 at 15:26, Shreyansh Jain > wrote: >> Hello David, list, >> >> 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. >> 3. Similarly, rte_eth_dev has rte_pci_device which should be replaced with >> rte_device. >> >> This is what the current outline of eth_driver is: >> >> ++ >> | eth_driver | >> | +-+| >> | | rte_pci_driver || >> | | +--+|| >> | | | rte_driver ||| >> | | | name[] ||| >> | | | ... ||| >> | | +--+|| >> | | .probe || >> | | .remove|| >> | | ...|| >> | +-+| >> | .eth_dev_init | >> | .eth_dev_uninit | >> ++ >> >> This is what I was thinking: >> >> +-++--+ >> | rte_pci_driver ||eth_driver| >> | +--+| _|_struct rte_driver *p | >> | | rte_driver <---/ | .eth_dev_init| >> | | ... ||| .eth_dev_uninit | >> | | name||+--+ >> | ||| >> | +--+| >> | | >> +-+ >> >> ::Impact:: >> Various drivers use the rte_pci_driver embedded in the eth_driver object for >> device initialization. >> == They assume that rte_pci_driver is directly embedded and hence simply >> dereference. >> == e.g. eth_igb_dev_init() in drivers/net/e1000/igb_ethdev.c file >> >> With the above change, such drivers would have to access rte_driver and then >> perform container_of to obtain their respective rte_xxx_driver. >> == this would be useful in case there is a non-PCI driver >> >> ::Problem:: >> I am not sure of reason as to why eth_driver embedded rte_pci_driver in >> first place - other than a convenient way to define it before PCI driver >> registration. >> >> As all the existing PMDs are impacted - am I missing something here in >> making the above change? >> > > How do you know eth_driver->p is pointing to a rte_pci_driver or > rte_soc_driver? > Maybe you need to add a type/flag in rte_driver. My take: PMD implementation would specify this - similar to how it is done now. A PCI PMD would perform a container_of(rte_pci_driver,...). I don't think we need a differentiation here - primarily because generic doesn't handle the eth_driver. > >> Probably, similar is the case for rte_eth_dev. >> >> - >> Shreyansh > -- - Shreyansh
[dpdk-dev] Clarification for eth_driver changes
Hello David, list, 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. 3. Similarly, rte_eth_dev has rte_pci_device which should be replaced with rte_device. This is what the current outline of eth_driver is: ++ | eth_driver | | +-+| | | rte_pci_driver || | | +--+|| | | | rte_driver ||| | | | name[] ||| | | | ... ||| | | +--+|| | | .probe || | | .remove|| | | ...|| | +-+| | .eth_dev_init | | .eth_dev_uninit | ++ This is what I was thinking: +-++--+ | rte_pci_driver ||eth_driver| | +--+| _|_struct rte_driver *p | | | rte_driver <---/ | .eth_dev_init| | | ... ||| .eth_dev_uninit | | | name||+--+ | ||| | +--+| | | +-+ ::Impact:: Various drivers use the rte_pci_driver embedded in the eth_driver object for device initialization. == They assume that rte_pci_driver is directly embedded and hence simply dereference. == e.g. eth_igb_dev_init() in drivers/net/e1000/igb_ethdev.c file With the above change, such drivers would have to access rte_driver and then perform container_of to obtain their respective rte_xxx_driver. == this would be useful in case there is a non-PCI driver ::Problem:: I am not sure of reason as to why eth_driver embedded rte_pci_driver in first place - other than a convenient way to define it before PCI driver registration. As all the existing PMDs are impacted - am I missing something here in making the above change? Probably, similar is the case for rte_eth_dev. - Shreyansh
[dpdk-dev] Clarification for eth_driver changes
Hi Stephen, 2016-11-10 02:51, Stephen Hemminger: > I also think drv_flags should part of device not PCI. Most of the flags > there like link state support are generic. If it isn't changed for this > release will probably have to break ABI to fully support VMBUS When do you plan to send VMBUS patches? Could you send a deprecation notice for this change? Are you aware of the work started by Shreyansh to have a generic bus model? Could you help in 17.02 timeframe to have a solid bus model? Thanks
[dpdk-dev] Clarification for eth_driver changes
Hello Shreyansh, On Thu, Nov 10, 2016 at 8:26 AM, Shreyansh Jain 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 ? As far as I can see, it is only a convenient wrapper for existing pci drivers, but in the end it is just a pci_driver with ethdev context in it that could be pushed to each existing driver. In my initial description http://dpdk.org/ml/archives/dev/2016-January/031390.html, what I had in mind was only having a rte_eth_device pointing to a generic rte_device. If we need to invoke some generic driver ops from ethdev (I can only see the ethdev hotplug api, maybe I missed something), then we would go through rte_eth_device -> rte_device -> rte_driver. The rte_driver keeps its own bus/private logic in its code, and no need to expose a type. > 3. Similarly, rte_eth_dev has rte_pci_device which should be replaced with > rte_device. Yes, that's the main change for me. Thanks. -- David Marchand
[dpdk-dev] Clarification for eth_driver changes
2016-11-10 15:51, Jianbo Liu: > On 10 November 2016 at 15:26, Shreyansh Jain > wrote: > > This is what the current outline of eth_driver is: > > > > ++ > > | eth_driver | > > | +-+| > > | | rte_pci_driver || > > | | +--+|| > > | | | rte_driver ||| > > | | | name[] ||| > > | | | ... ||| > > | | +--+|| > > | | .probe || > > | | .remove|| > > | | ...|| > > | +-+| > > | .eth_dev_init | > > | .eth_dev_uninit | > > ++ > > > > This is what I was thinking: > > > > +-++--+ > > | rte_pci_driver ||eth_driver| > > | +--+| _|_struct rte_driver *p | > > | | rte_driver <---/ | .eth_dev_init| > > | | ... ||| .eth_dev_uninit | > > | | name||+--+ > > | ||| > > | +--+| > > | | > > +-+ > > > > ::Impact:: > > Various drivers use the rte_pci_driver embedded in the eth_driver object for > > device initialization. > > == They assume that rte_pci_driver is directly embedded and hence simply > > dereference. > > == e.g. eth_igb_dev_init() in drivers/net/e1000/igb_ethdev.c file > > > > With the above change, such drivers would have to access rte_driver and then > > perform container_of to obtain their respective rte_xxx_driver. > > == this would be useful in case there is a non-PCI driver > > > > ::Problem:: > > I am not sure of reason as to why eth_driver embedded rte_pci_driver in > > first place - other than a convenient way to define it before PCI driver > > registration. > > > > As all the existing PMDs are impacted - am I missing something here in > > making the above change? > > > > How do you know eth_driver->p is pointing to a rte_pci_driver or > rte_soc_driver? > Maybe you need to add a type/flag in rte_driver. Why do you need any bus information at ethdev level?
[dpdk-dev] Clarification for eth_driver changes
I also think drv_flags should part of device not PCI. Most of the flags there like link state support are generic. If it isn't changed for this release will probably have to break ABI to fully support VMBUS
[dpdk-dev] Clarification for eth_driver changes
2016-11-10 14:12, Shreyansh Jain: > On Thursday 10 November 2016 01:33 PM, Thomas Monjalon wrote: > > 2016-11-10 15:51, Jianbo Liu: > >> On 10 November 2016 at 15:26, Shreyansh Jain > >> wrote: > >>> This is what the current outline of eth_driver is: > >>> > >>> ++ > >>> | eth_driver | > >>> | +-+| > >>> | | rte_pci_driver || > >>> | | +--+|| > >>> | | | rte_driver ||| > >>> | | | name[] ||| > >>> | | | ... ||| > >>> | | +--+|| > >>> | | .probe || > >>> | | .remove|| > >>> | | ...|| > >>> | +-+| > >>> | .eth_dev_init | > >>> | .eth_dev_uninit | > >>> ++ > >>> > >>> This is what I was thinking: > >>> > >>> +-++--+ > >>> | rte_pci_driver ||eth_driver| > >>> | +--+| _|_struct rte_driver *p | > >>> | | rte_driver <---/ | .eth_dev_init| > >>> | | ... ||| .eth_dev_uninit | > >>> | | name||+--+ > >>> | ||| > >>> | +--+| > >>> | | > >>> +-+ > >>> > >>> ::Impact:: > >>> Various drivers use the rte_pci_driver embedded in the eth_driver object > >>> for > >>> device initialization. > >>> == They assume that rte_pci_driver is directly embedded and hence simply > >>> dereference. > >>> == e.g. eth_igb_dev_init() in drivers/net/e1000/igb_ethdev.c file > >>> > >>> With the above change, such drivers would have to access rte_driver and > >>> then > >>> perform container_of to obtain their respective rte_xxx_driver. > >>> == this would be useful in case there is a non-PCI driver > >>> > >>> ::Problem:: > >>> I am not sure of reason as to why eth_driver embedded rte_pci_driver in > >>> first place - other than a convenient way to define it before PCI driver > >>> registration. > >>> > >>> As all the existing PMDs are impacted - am I missing something here in > >>> making the above change? > >>> > >> > >> How do you know eth_driver->p is pointing to a rte_pci_driver or > >> rte_soc_driver? > >> Maybe you need to add a type/flag in rte_driver. > > > > Why do you need any bus information at ethdev level? > > AFAIK, we don't need it. Above text is not stating anything on that > grounds either, I think. Isn't it? No, I was replying to Jianbo. Anyway, David made a more interesting comment.
