[dpdk-dev,RFC,1/6] librte_ether: add fields from rte_pci_driver to rte_eth_dev and rte_eth_dev_data.
Commit Message
add dev_flags to rte_eth_dev, add macros for dev_flags.
add numa_node to rte_eth_dev_data.
use dev_type to distinguish between vdev's and pdev's.
remove unused RTE_ETH_DEV_MAX.
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
lib/librte_ether/rte_ethdev.c | 19 +++++++++++++++----
lib/librte_ether/rte_ethdev.h | 8 +++++++-
2 files changed, 22 insertions(+), 5 deletions(-)
Comments
2015-08-27 16:40, Bernard Iremonger:
> add dev_flags to rte_eth_dev, add macros for dev_flags.
> add numa_node to rte_eth_dev_data.
> use dev_type to distinguish between vdev's and pdev's.
> remove unused RTE_ETH_DEV_MAX.
>
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
[...]
> @@ -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.
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?
Regards,
Bernard.
On Tue, Sep 01, 2015 at 11:38:31AM +0000, Iremonger, Bernard wrote:
> 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?
>
> Regards,
>
> Bernard.
>
Just to give my 2c.
The branch in the snippet above should not exist. Each PMD should set the data
numa_node and the flags fields appropriately at initialization, either directly
or by copying in the relevant data from the interface specific structure e.g. pci.
The ethdev should never need to check the device type here, it should always just
read data->numa_node.
/Bruce
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger, Bernard
> Sent: Tuesday, September 01, 2015 12:39 PM
> To: Thomas Monjalon
> Cc: dev@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
>
> Regards,
>
> Bernard.
>
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Tuesday, September 1, 2015 1:37 PM
> To: Iremonger, Bernard; Thomas Monjalon
> Cc: dev@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@dpdk.org] On Behalf Of Iremonger,
> > Bernard
> > Sent: Tuesday, September 01, 2015 12:39 PM
> > To: Thomas Monjalon
> > Cc: dev@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
@@ -260,6 +260,7 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
eth_dev->data->port_id = port_id;
eth_dev->attached = DEV_ATTACHED;
eth_dev->dev_type = type;
+ eth_dev->dev_flags = 0;
nb_ports++;
return eth_dev;
}
@@ -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;
}
uint8_t
@@ -504,6 +508,7 @@ static int
rte_eth_dev_is_detachable(uint8_t port_id)
{
uint32_t drv_flags;
+ uint32_t dev_flags;
if (!rte_eth_dev_is_valid_port(port_id)) {
PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
@@ -520,10 +525,14 @@ rte_eth_dev_is_detachable(uint8_t port_id)
default:
return -ENOTSUP;
}
+ drv_flags = rte_eth_devices[port_id].driver->pci_drv.drv_flags;
+ return !(drv_flags & RTE_PCI_DRV_DETACHABLE);
+ } else if (rte_eth_devices[port_id].dev_type == RTE_ETH_DEV_VIRTUAL) {
+ dev_flags = rte_eth_devices[port_id].dev_flags;
+ return !(dev_flags & RTE_ETH_DEV_DETACHABLE);
}
- drv_flags = rte_eth_devices[port_id].driver->pci_drv.drv_flags;
- return !(drv_flags & RTE_PCI_DRV_DETACHABLE);
+ return -ENOTSUP;
}
/* attach the new physical device, then store port_id of the device */
@@ -1795,8 +1804,10 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
(*dev->dev_ops->dev_infos_get)(dev, dev_info);
dev_info->pci_dev = dev->pci_dev;
- if (dev->driver)
+ if (dev->dev_type == RTE_ETH_DEV_PCI)
dev_info->driver_name = dev->driver->pci_drv.name;
+ else
+ dev_info->driver_name = dev->data->name;
}
void
@@ -1553,7 +1553,6 @@ enum rte_eth_dev_type {
RTE_ETH_DEV_PCI,
/**< Physical function and Virtual function of PCI devices */
RTE_ETH_DEV_VIRTUAL, /**< non hardware device */
- RTE_ETH_DEV_MAX /**< max value of this enum */
};
/**
@@ -1587,8 +1586,14 @@ struct rte_eth_dev {
struct rte_eth_rxtx_callback *pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
uint8_t attached; /**< Flag indicating the port is attached */
enum rte_eth_dev_type dev_type; /**< Flag indicating the device type */
+ uint32_t dev_flags; /**< Flags controlling handling of device. */
};
+/** Device supports link state interrupt */
+#define RTE_ETH_DEV_INTR_LSC 0x0008
+/** Device supports detaching capability */
+#define RTE_ETH_DEV_DETACHABLE 0x0010
+
struct rte_eth_dev_sriov {
uint8_t active; /**< SRIOV is active with 16, 32 or 64 pools */
uint8_t nb_q_per_pool; /**< rx queue number per pool */
@@ -1639,6 +1644,7 @@ 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) */
+ int numa_node;
};
/**