Re: [dpdk-dev] [PATCH v2 1/2] pmd: Modified dev_info structure to include default RX/TX configuration

[David Marchand <david.marchand@6wind.com>]

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.

-- 
David Marchand


On Wed, Oct 1, 2014 at 10:00 AM, Pablo de Lara <
pablo.de.lara.guarch@intel.com> 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;
> diff --git a/lib/librte_pmd_e1000/igb_ethdev.c
> b/lib/librte_pmd_e1000/igb_ethdev.c
> index 3187d92..4bf75f5 100644
> --- a/lib/librte_pmd_e1000/igb_ethdev.c
> +++ b/lib/librte_pmd_e1000/igb_ethdev.c
> @@ -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 */
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 9ed31b5..72faf95 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -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
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> index 59122a1..3b1c156 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> @@ -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 */
> --
> 1.7.7.6
>
>