[dpdk-dev,v2,1/2] pmd: Modified dev_info structure to include default RX/TX configuration
Commit Message
Many sample apps use duplicated code to set rte_eth_txconf and rte_eth_rxconf
structures. This patch allows the user to get a default optimal RX/TX configuration
through rte_eth_dev_info get, and still any parameters may be tweaked as wished,
before setting up queues.
Besides, if a NULL pointer is passed to rte_eth_rx_queue_setup or
rte_eth_tx_queue_setup, these functions get internally the default RX/TX
configuration for the user.
Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
lib/librte_ether/rte_ethdev.c | 28 +++++++++++++++++++++-----
lib/librte_ether/rte_ethdev.h | 2 +
lib/librte_pmd_e1000/igb_ethdev.c | 32 ++++++++++++++++++++++++++++++-
lib/librte_pmd_i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++++++++++++
lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 36 +++++++++++++++++++++++++++++++++++
5 files changed, 124 insertions(+), 7 deletions(-)
Comments
On Wed, Oct 01, 2014 at 09:00:57AM +0100, Pablo de Lara wrote:
> Many sample apps use duplicated code to set rte_eth_txconf and rte_eth_rxconf
> structures. This patch allows the user to get a default optimal RX/TX configuration
> through rte_eth_dev_info get, and still any parameters may be tweaked as wished,
> before setting up queues.
>
> Besides, if a NULL pointer is passed to rte_eth_rx_queue_setup or
> rte_eth_tx_queue_setup, these functions get internally the default RX/TX
> configuration for the user.
>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
> lib/librte_ether/rte_ethdev.c | 28 +++++++++++++++++++++-----
> lib/librte_ether/rte_ethdev.h | 2 +
> lib/librte_pmd_e1000/igb_ethdev.c | 32 ++++++++++++++++++++++++++++++-
> lib/librte_pmd_i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++++++++++++
> lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 36 +++++++++++++++++++++++++++++++++++
> 5 files changed, 124 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index fd1010a..2f9eea2 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -928,6 +928,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> struct rte_eth_dev *dev;
> struct rte_pktmbuf_pool_private *mbp_priv;
> struct rte_eth_dev_info dev_info;
> + const struct rte_eth_rxconf *conf;
>
> /* This function is only safe when called from the primary process
> * in a multi-process setup*/
> @@ -937,6 +938,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> return (-EINVAL);
> }
> +
> dev = &rte_eth_devices[port_id];
> if (rx_queue_id >= dev->data->nb_rx_queues) {
> PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n", rx_queue_id);
> @@ -980,8 +982,12 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> return (-EINVAL);
> }
>
> + conf = rx_conf;
> + if (conf == NULL)
> + conf = &dev_info.default_rxconf;
> +
> ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
> - socket_id, rx_conf, mp);
> + socket_id, conf, mp);
> if (!ret) {
> if (!dev->data->min_rx_buf_size ||
> dev->data->min_rx_buf_size > mbp_buf_size)
One very minor nit - I don't think you need the new local variable conf.
You can just check "if (rx_conf == NULL) rx_conf = &dev...". The same for
the tx_conf too.
/Bruce
On Wed, Oct 01, 2014 at 09:00:57AM +0100, Pablo de Lara wrote:
> Many sample apps use duplicated code to set rte_eth_txconf and rte_eth_rxconf
> structures. This patch allows the user to get a default optimal RX/TX configuration
> through rte_eth_dev_info get, and still any parameters may be tweaked as wished,
> before setting up queues.
>
> Besides, if a NULL pointer is passed to rte_eth_rx_queue_setup or
> rte_eth_tx_queue_setup, these functions get internally the default RX/TX
> configuration for the user.
>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
> lib/librte_ether/rte_ethdev.c | 28 +++++++++++++++++++++-----
> lib/librte_ether/rte_ethdev.h | 2 +
> lib/librte_pmd_e1000/igb_ethdev.c | 32 ++++++++++++++++++++++++++++++-
> lib/librte_pmd_i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++++++++++++
> lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 36 +++++++++++++++++++++++++++++++++++
> 5 files changed, 124 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index fd1010a..2f9eea2 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -928,6 +928,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> struct rte_eth_dev *dev;
> struct rte_pktmbuf_pool_private *mbp_priv;
> struct rte_eth_dev_info dev_info;
> + const struct rte_eth_rxconf *conf;
>
> /* This function is only safe when called from the primary process
> * in a multi-process setup*/
> @@ -937,6 +938,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> return (-EINVAL);
> }
> +
> dev = &rte_eth_devices[port_id];
> if (rx_queue_id >= dev->data->nb_rx_queues) {
> PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n", rx_queue_id);
> @@ -980,8 +982,12 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> return (-EINVAL);
> }
>
> + conf = rx_conf;
> + if (conf == NULL)
> + conf = &dev_info.default_rxconf;
> +
> ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
> - socket_id, rx_conf, mp);
> + socket_id, conf, mp);
> if (!ret) {
> if (!dev->data->min_rx_buf_size ||
> dev->data->min_rx_buf_size > mbp_buf_size)
> @@ -997,6 +1003,8 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> const struct rte_eth_txconf *tx_conf)
> {
> struct rte_eth_dev *dev;
> + struct rte_eth_dev_info dev_info;
> + const struct rte_eth_txconf *conf;
>
> /* This function is only safe when called from the primary process
> * in a multi-process setup*/
> @@ -1006,6 +1014,7 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> return (-EINVAL);
> }
> +
> dev = &rte_eth_devices[port_id];
> if (tx_queue_id >= dev->data->nb_tx_queues) {
> PMD_DEBUG_TRACE("Invalid TX queue_id=%d\n", tx_queue_id);
> @@ -1018,9 +1027,17 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> return -EBUSY;
> }
>
> + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
> FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_setup, -ENOTSUP);
> +
> + (*dev->dev_ops->dev_infos_get)(dev, &dev_info);
> +
> + conf = tx_conf;
> + if (conf == NULL)
> + conf = &dev_info.default_txconf;
> +
> return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id, nb_tx_desc,
> - socket_id, tx_conf);
> + socket_id, conf);
> }
>
> void
> @@ -1249,10 +1266,9 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
> }
> dev = &rte_eth_devices[port_id];
>
> - /* Default device offload capabilities to zero */
> - dev_info->rx_offload_capa = 0;
> - dev_info->tx_offload_capa = 0;
> - dev_info->if_index = 0;
> + /* Reset dev info structure */
> + memset(dev_info, 0, sizeof(struct rte_eth_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;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 50df654..f63d38a 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -906,6 +906,8 @@ struct rte_eth_dev_info {
> uint16_t max_vmdq_pools; /**< Maximum number of VMDq pools. */
> uint32_t rx_offload_capa; /**< Device RX offload capabilities. */
> uint32_t tx_offload_capa; /**< Device TX offload capabilities. */
> + struct rte_eth_rxconf default_rxconf; /**< Default RX configuration */
> + struct rte_eth_txconf default_txconf; /**< Default TX configuration */
> };
>
> struct rte_eth_dev;
In this file (rte_ethdev.h), you also need to update the API documentation
for the rte_eth_rx_queue_setup and rte_eth_tx_queue_setup functions to state
that passing in a NULL value for the conf parameter is allowed, and that it
just means to use the defaults.
/Bruce
Hello Pablo,
I agree with Bruce comments.
I have a single comment: I would have preferred to have a separate patch
for the memset() on dev_info (could be the first patch of this patchset).
All the more so as it is not detailed in the commit log.
Thanks.
@@ -928,6 +928,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
struct rte_eth_dev *dev;
struct rte_pktmbuf_pool_private *mbp_priv;
struct rte_eth_dev_info dev_info;
+ const struct rte_eth_rxconf *conf;
/* This function is only safe when called from the primary process
* in a multi-process setup*/
@@ -937,6 +938,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
return (-EINVAL);
}
+
dev = &rte_eth_devices[port_id];
if (rx_queue_id >= dev->data->nb_rx_queues) {
PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n", rx_queue_id);
@@ -980,8 +982,12 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
return (-EINVAL);
}
+ conf = rx_conf;
+ if (conf == NULL)
+ conf = &dev_info.default_rxconf;
+
ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
- socket_id, rx_conf, mp);
+ socket_id, conf, mp);
if (!ret) {
if (!dev->data->min_rx_buf_size ||
dev->data->min_rx_buf_size > mbp_buf_size)
@@ -997,6 +1003,8 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
const struct rte_eth_txconf *tx_conf)
{
struct rte_eth_dev *dev;
+ struct rte_eth_dev_info dev_info;
+ const struct rte_eth_txconf *conf;
/* This function is only safe when called from the primary process
* in a multi-process setup*/
@@ -1006,6 +1014,7 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
return (-EINVAL);
}
+
dev = &rte_eth_devices[port_id];
if (tx_queue_id >= dev->data->nb_tx_queues) {
PMD_DEBUG_TRACE("Invalid TX queue_id=%d\n", tx_queue_id);
@@ -1018,9 +1027,17 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
return -EBUSY;
}
+ FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_setup, -ENOTSUP);
+
+ (*dev->dev_ops->dev_infos_get)(dev, &dev_info);
+
+ conf = tx_conf;
+ if (conf == NULL)
+ conf = &dev_info.default_txconf;
+
return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id, nb_tx_desc,
- socket_id, tx_conf);
+ socket_id, conf);
}
void
@@ -1249,10 +1266,9 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
}
dev = &rte_eth_devices[port_id];
- /* Default device offload capabilities to zero */
- dev_info->rx_offload_capa = 0;
- dev_info->tx_offload_capa = 0;
- dev_info->if_index = 0;
+ /* Reset dev info structure */
+ memset(dev_info, 0, sizeof(struct rte_eth_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;
@@ -906,6 +906,8 @@ struct rte_eth_dev_info {
uint16_t max_vmdq_pools; /**< Maximum number of VMDq pools. */
uint32_t rx_offload_capa; /**< Device RX offload capabilities. */
uint32_t tx_offload_capa; /**< Device TX offload capabilities. */
+ struct rte_eth_rxconf default_rxconf; /**< Default RX configuration */
+ struct rte_eth_txconf default_txconf; /**< Default TX configuration */
};
struct rte_eth_dev;
@@ -57,6 +57,18 @@
#include "e1000/e1000_api.h"
#include "e1000_ethdev.h"
+/**
+ * Default values for port configuration
+ */
+#define IGB_DEFAULT_RX_FREE_THRESH 32
+#define IGB_DEFAULT_RX_PTHRESH 8
+#define IGB_DEFAULT_RX_HTHRESH 8
+#define IGB_DEFAULT_RX_WTHRESH 0
+
+#define IGB_DEFAULT_TX_PTHRESH 32
+#define IGB_DEFAULT_TX_HTHRESH 0
+#define IGB_DEFAULT_TX_WTHRESH 0
+
static int eth_igb_configure(struct rte_eth_dev *dev);
static int eth_igb_start(struct rte_eth_dev *dev);
static void eth_igb_stop(struct rte_eth_dev *dev);
@@ -165,7 +177,6 @@ static int eth_igb_remove_5tuple_filter(struct rte_eth_dev *dev,
static int eth_igb_get_5tuple_filter(struct rte_eth_dev *dev,
uint16_t index,
struct rte_5tuple_filter *filter, uint16_t *rx_queue);
-
/*
* Define VF Stats MACRO for Non "cleared on read" register
*/
@@ -1331,6 +1342,25 @@ eth_igb_infos_get(struct rte_eth_dev *dev,
dev_info->max_tx_queues = 0;
dev_info->max_vmdq_pools = 0;
}
+
+ dev_info->default_rxconf = (struct rte_eth_rxconf) {
+ .rx_thresh = {
+ .pthresh = IGB_DEFAULT_RX_PTHRESH,
+ .hthresh = IGB_DEFAULT_RX_HTHRESH,
+ .wthresh = IGB_DEFAULT_RX_WTHRESH,
+ },
+ .rx_free_thresh = IGB_DEFAULT_RX_FREE_THRESH,
+ .rx_drop_en = 0,
+ };
+
+ dev_info->default_txconf = (struct rte_eth_txconf) {
+ .tx_thresh = {
+ .pthresh = IGB_DEFAULT_TX_PTHRESH,
+ .hthresh = IGB_DEFAULT_TX_HTHRESH,
+ .wthresh = IGB_DEFAULT_TX_WTHRESH,
+ },
+ .txq_flags = 0,
+ };
}
/* return 0 means link status changed, -1 means not changed */
@@ -58,6 +58,17 @@
#include "i40e_rxtx.h"
#include "i40e_pf.h"
+#define I40E_DEFAULT_RX_FREE_THRESH 32
+#define I40E_DEFAULT_RX_PTHRESH 8
+#define I40E_DEFAULT_RX_HTHRESH 8
+#define I40E_DEFAULT_RX_WTHRESH 0
+
+#define I40E_DEFAULT_TX_FREE_THRESH 32
+#define I40E_DEFAULT_TX_PTHRESH 32
+#define I40E_DEFAULT_TX_HTHRESH 0
+#define I40E_DEFAULT_TX_WTHRESH 0
+#define I40E_DEFAULT_TX_RSBIT_THRESH 32
+
/* Maximun number of MAC addresses */
#define I40E_NUM_MACADDR_MAX 64
#define I40E_CLEAR_PXE_WAIT_MS 200
@@ -1231,6 +1242,28 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
DEV_TX_OFFLOAD_UDP_CKSUM |
DEV_TX_OFFLOAD_TCP_CKSUM |
DEV_TX_OFFLOAD_SCTP_CKSUM;
+
+ dev_info->default_rxconf = (struct rte_eth_rxconf) {
+ .rx_thresh = {
+ .pthresh = I40E_DEFAULT_RX_PTHRESH,
+ .hthresh = I40E_DEFAULT_RX_HTHRESH,
+ .wthresh = I40E_DEFAULT_RX_WTHRESH,
+ },
+ .rx_free_thresh = I40E_DEFAULT_RX_FREE_THRESH,
+ .rx_drop_en = 0,
+ };
+
+ dev_info->default_txconf = (struct rte_eth_txconf) {
+ .tx_thresh = {
+ .pthresh = I40E_DEFAULT_TX_PTHRESH,
+ .hthresh = I40E_DEFAULT_TX_HTHRESH,
+ .wthresh = I40E_DEFAULT_TX_WTHRESH,
+ },
+ .tx_free_thresh = I40E_DEFAULT_TX_FREE_THRESH,
+ .tx_rs_thresh = I40E_DEFAULT_TX_RSBIT_THRESH,
+ .txq_flags = ETH_TXQ_FLAGS_NOMULTSEGS | ETH_TXQ_FLAGS_NOOFFLOADS,
+ };
+
}
static int
@@ -92,6 +92,20 @@
#define IXGBE_MMW_SIZE_DEFAULT 0x4
#define IXGBE_MMW_SIZE_JUMBO_FRAME 0x14
+/**
+ * Default values for RX/TX configuration
+ */
+#define IXGBE_DEFAULT_RX_FREE_THRESH 32
+#define IXGBE_DEFAULT_RX_PTHRESH 8
+#define IXGBE_DEFAULT_RX_HTHRESH 8
+#define IXGBE_DEFAULT_RX_WTHRESH 0
+
+#define IXGBE_DEFAULT_TX_FREE_THRESH 32
+#define IXGBE_DEFAULT_TX_PTHRESH 32
+#define IXGBE_DEFAULT_TX_HTHRESH 0
+#define IXGBE_DEFAULT_TX_WTHRESH 0
+#define IXGBE_DEFAULT_TX_RSBIT_THRESH 32
+
#define IXGBEVF_PMD_NAME "rte_ixgbevf_pmd" /* PMD name */
#define IXGBE_QUEUE_STAT_COUNTERS (sizeof(hw_stats->qprc) / sizeof(hw_stats->qprc[0]))
@@ -1948,6 +1962,28 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
DEV_TX_OFFLOAD_UDP_CKSUM |
DEV_TX_OFFLOAD_TCP_CKSUM |
DEV_TX_OFFLOAD_SCTP_CKSUM;
+
+ dev_info->default_rxconf = (struct rte_eth_rxconf) {
+ .rx_thresh = {
+ .pthresh = IXGBE_DEFAULT_RX_PTHRESH,
+ .hthresh = IXGBE_DEFAULT_RX_HTHRESH,
+ .wthresh = IXGBE_DEFAULT_RX_WTHRESH,
+ },
+ .rx_free_thresh = IXGBE_DEFAULT_RX_FREE_THRESH,
+ .rx_drop_en = 0,
+ };
+
+
+ dev_info->default_txconf = (struct rte_eth_txconf) {
+ .tx_thresh = {
+ .pthresh = IXGBE_DEFAULT_TX_PTHRESH,
+ .hthresh = IXGBE_DEFAULT_TX_HTHRESH,
+ .wthresh = IXGBE_DEFAULT_TX_WTHRESH,
+ },
+ .tx_free_thresh = IXGBE_DEFAULT_TX_FREE_THRESH,
+ .tx_rs_thresh = IXGBE_DEFAULT_TX_RSBIT_THRESH,
+ .txq_flags = ETH_TXQ_FLAGS_NOMULTSEGS | ETH_TXQ_FLAGS_NOOFFLOADS,
+ };
}
/* return 0 means link status changed, -1 means not changed */