From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f42.google.com (mail-oi0-f42.google.com [209.85.218.42]) by dpdk.org (Postfix) with ESMTP id DCF596A9B for ; Wed, 1 Oct 2014 10:38:46 +0200 (CEST) Received: by mail-oi0-f42.google.com with SMTP id a141so249286oig.15 for ; Wed, 01 Oct 2014 01:45:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=JgER72T4aZZZeNLpf3bPWrGhx67plVGUHvaDCLGX37k=; b=mHiNhqXJQiyksvmNSeHEkT33ape6aGivXT2H/C0QS+c9kGg3+PyJQICEksz8xNEFlG kW2ZkEBgDFZKdrQpcPKBsCkO74QOf4fsxQlmAhz8X/chHvk4rweKDhR45yg/atiVopZy y5nVunNTKpQh4giETqxEzTCfp9/SMGuC2F3ZNccpZyDpOA4allgM2upEPgfdNK6nq9by JtGHOZeTSNCrKkpgRBXUCnhgEZWhb9k/k0vOHpHhhOYamFFR8RKgpSAoDlnlpZNraVNT je4KSHlTBB6Xuq8unG/Zy/3qabD9l+wIJ21ocAG6AaRcxUeHCsQ6ZTXH3Cm6lkTrtlXe fFiA== X-Gm-Message-State: ALoCoQnh0JObp+HUrJSFEOCBxsw/0QqVz6o/PdguqH57v/u0GQbbAIyzIxR16xilBQj7GLJIqHKi MIME-Version: 1.0 X-Received: by 10.182.40.234 with SMTP id a10mr25420798obl.8.1412153130284; Wed, 01 Oct 2014 01:45:30 -0700 (PDT) Received: by 10.202.57.139 with HTTP; Wed, 1 Oct 2014 01:45:30 -0700 (PDT) In-Reply-To: <1412150458-26213-2-git-send-email-pablo.de.lara.guarch@intel.com> References: <1411741159-6671-1-git-send-email-pablo.de.lara.guarch@intel.com> <1412150458-26213-1-git-send-email-pablo.de.lara.guarch@intel.com> <1412150458-26213-2-git-send-email-pablo.de.lara.guarch@intel.com> Date: Wed, 1 Oct 2014 10:45:30 +0200 Message-ID: From: David Marchand To: Pablo de Lara Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 1/2] pmd: Modified dev_info structure to include default RX/TX configuration X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Oct 2014 08:38:47 -0000 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 > --- > 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 > >