[dpdk-dev] [PATCH 02/20] librte_ether: add fields from rte_pci_driver to rte_eth_dev_data

[Iremonger, Bernard]

Hi Neil

<snip>
> > > > > +++ b/lib/librte_ether/rte_ethdev.h
> > > > > @@ -1635,8 +1635,23 @@ struct rte_eth_dev_data {
> > > > >  		all_multicast : 1, /**< RX all multicast mode ON(1) /
> OFF(0). */
> > > > >  		dev_started : 1,   /**< Device state: STARTED(1) /
> STOPPED(0). */
> > > > >  		lro         : 1;   /**< RX LRO is ON(1) / OFF(0) */
> > > > > +	uint32_t dev_flags; /**< Flags controlling handling of device.
> */
> > > > > +	enum rte_kernel_driver kdrv;	/**< Kernel driver
> passthrough */
> > > > Why add this here? The ennumerated driver types are all variants
> > > > on PCI bus types.  Not sure why the ethernet interface needs to
> > > > know this info
> > > >
> > > > > +	int numa_node;
> > > > Ditto, this seems like information that is only relevant if the
> > > > device is on a physical bus (i.e. virual devices are likely to not
> > > > have a numa node)
> > > >
> > > Actually, I disagree. For some virtual devices they will have a numa
> > > node. For ring or other virtual PMDs the numa node will be the node
> > > on which the ring / mempool etc. memory is allocated on, and can be of
> relevance.
> > >
> > > /Bruce
> > >
> >
> > I think its fairly clear that some devices (including virtual ones)
> > have some relevant relation to a numa_node (There are even some that
> > have no numa_node, for which a -1 value makes some sense).  That said,
> > there are just as many that don't have a relevant numa_node.
> >
> > 1) There are some drivers for which numa_node make no sense
> > (regardless of
> > value):
> >  * af_packet - The numa node is at best determined at run time by the
> > interface the socket is bound to
> >
> >  * pcap - same as af_packet
> >
> >  * bonding - multiple interfaces mean multiple numa_nodes, any value
> > set here is just as likely to be wrong as right
> >
> >  * mpipe - no real large memory area to associate with a numa node
> >
> >  * virtio - uses iopl for communication, and cannot know its numa_node
> >
> >  * vmxnet3 - same concept as virtio
> >
> >  * xenvirt - same as vmxnet3
> >
> > I think its better that you store numa locality information in a pmd's
> > private bus data, and export it to applications via a device method.
> > that provides the flexibility to tell the application that there is no
> > numa locality for a device (by not implementing the method), without
> > having to expose an unset data field to the application.
> >
> > Neil
> >
> 
> Sure, that could work.
> However, is it really worthwhile asking drivers to implement a new ethdev
> API function, rather than just having them set the numa node field correctly
> in the init function?
> 
> /Bruce

The four fields below have been added  to  struct rte_eth_dev_data

	uint32_t dev_flags; /**< Flags controlling handling of device. */
	enum rte_kernel_driver kdrv;	/**< Kernel driver passthrough */
	int numa_node;
	const char *drv_name;

The data for these fields is available in the struct rte_pci_device.
In order to remove the pci_device  from the vdev PMD's this data needs to be available in the eth_dev.
A new function rte_eth_copy_dev_info() has been added to the eth_dev for use by the pdevs to copy this data from the pci_device to the ethdev.
In the vdevs the pci_device has been removed and the new fields are set up directly in the rte_driver.init function.

The numa_node is already initialised  in  the following vdev PMD's:

af_packet - initialized from socket_id
bonding - initialized from socket_id
mpipe - initialized from instance
null   - initialized from socket_id
pcap - initialized from socket_id
ring  - initialized form socket_id
xenvirt - initialized from socket_id

Regards,

Bernard.