[dpdk-dev] [RFC PATCH 1/6] librte_ether: add fields from rte_pci_driver to rte_eth_dev and rte_eth_dev_data.

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Tuesday, September 1, 2015 1:37 PM
> To: Iremonger, Bernard; Thomas Monjalon
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [RFC PATCH 1/6] librte_ether: add fields from
> rte_pci_driver to rte_eth_dev and rte_eth_dev_data.
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Iremonger,
> > Bernard
> > Sent: Tuesday, September 01, 2015 12:39 PM
> > To: Thomas Monjalon
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [RFC PATCH 1/6] librte_ether: add fields from
> rte_pci_driver to rte_eth_dev and rte_eth_dev_data.
> >
> > Hi THomas,
> >
> > <snip>
> >
> > > > @@ -424,7 +425,10 @@ rte_eth_dev_socket_id(uint8_t port_id)  {
> > > >  	if (!rte_eth_dev_is_valid_port(port_id))
> > > >  		return -1;
> > > > -	return rte_eth_devices[port_id].pci_dev->numa_node;
> > > > +	if (rte_eth_devices[port_id].dev_type == RTE_ETH_DEV_PCI)
> > > > +		return rte_eth_devices[port_id].pci_dev->numa_node;
> > > > +	else
> > > > +		return rte_eth_devices[port_id].data->numa_node;
> > >
> > > Clearly not the way to go.
> > > We should remove the special cases (PCI, PDEV, VDEV) instead of
> > > adding more checks.
> >
> > The dev_type field is not new, just using it now to distinguish between
> PCI and non PCI devices.
> >
> > I have moved some of the RTE_PCI_DRV flags to a new dev_flags field in
> > struct rte_eth_dev{}, These flags are independent of the driver type
> (PCI or non PCI).
> > Do you have view on this new dev_flags field and macros?
> 
> What looks strange here to me, I that we now we duplicate some fields
> here?
> Let say for PCI devices numa_node would be precent in pci_dev and in data,
> right?
> If there are some fields that are common for all device types (PCI, VDEV,
> etc) why not to create some unified structure for them that would be used
> by all device types and remove subsequent fields from pci_dev?
> Or alternatively create a union of structs (one struct per device type)?
> Then, as was pointed before, we wouldn't these branches by device_type at
> all.
> Konstantin
> 

I wouldn't worry so much about the duplicated data, so long as the ethdev APIs only
have to look for the data in a single place when necessary. [Some data may be naturally
present in the pci_dev structure, for instance, and then be copied by the driver into
the common ethdev structure. If however, that copy and duplication can be avoided,
great!]

/Bruce