Re: [dpdk-dev] [PATCH v2] ixgbe: add check for tx queue number

["Fischetti, Antonio" <antonio.fischetti@intel.com>]

Hi, I tested this patch with OVS+DPDK on a 72 lcores board with an 
Intel 82599ES 10 Gb NIC. It works fine.

Now when I call 'rte_eth_dev_info_get()' it returns correctly the number
of available Tx queues for the default mode, i.e. 64 when no VT and no
DCB is set.
Also, if I attempt to request more than the available queues via
'rte_eth_dev_configure()' it correctly returns -EINVAL error.

I checked it works with both DPDK v2.2.0 and the latest master branch.

Without this patch there was a misbehavior: when OVS queried for the
number of available Tx queues, 128 was returned. As a consequence
OVS was requesting 73 Tx queues, even though just 64 were really 
available. No error was returned. Of course any transmission was
failing when OVS attempted to use queues with ID >= 64.

Regards,
Antonio

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> Sent: Wednesday, March 23, 2016 3:29 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: [dpdk-dev] [PATCH v2] ixgbe: add check for tx queue number
> 
> IXGBE supports 128 TX queues. However, the full 128 queues
> are only available in VT and DCB mode.
> In normal default "none" mode (VT/DCB off) the maximum number
> of available queues is only 64.
> IXGBE doesn't check the mode when reporting the available
> number of queues. If a queue larger than 64 is used in default mode,
> the TX packets will be dropped silently.
> 
> This change adds a check to forbid using a queue number larger than 64
> during device configuration (in default mode), so that the problem is
> reported as early as possible.
> It also changes the order of where the dev_conf parameters are copied
> into the dev structure so that the correct maximum number of queues
> is reported for the correct mode.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
> 
> Changes in v2:
> 
> - Reorder memcpy of device configuration in rte_eth_dev_configure(),
>   so function gets the correct maximum number of queues
>   (depending on the operation mode), before checking the
>   requested number of queues.
> - Renamed new macro
> - Reworded/wrapped commit message
> 
>  drivers/net/ixgbe/ixgbe_ethdev.c | 17 ++++++++++++++++-
>  drivers/net/ixgbe/ixgbe_ethdev.h |  1 +
>  lib/librte_ether/rte_ethdev.c    |  6 +++---
>  3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index d4d883a..c799b47 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -1862,7 +1862,7 @@ ixgbe_check_mq_mode(struct rte_eth_dev *dev)
>  {
>  	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
>  	uint16_t nb_rx_q = dev->data->nb_rx_queues;
> -	uint16_t nb_tx_q = dev->data->nb_rx_queues;
> +	uint16_t nb_tx_q = dev->data->nb_tx_queues;
> 
>  	if (RTE_ETH_DEV_SRIOV(dev).active != 0) {
>  		/* check multi-queue mode */
> @@ -2002,6 +2002,16 @@ ixgbe_check_mq_mode(struct rte_eth_dev *dev)
>  				return -EINVAL;
>  			}
>  		}
> +
> +		if (dev_conf->txmode.mq_mode == ETH_MQ_TX_NONE) {
> +			if (nb_tx_q > IXGBE_NONE_MODE_TX_NB_QUEUES) {
> +				PMD_INIT_LOG(ERR,
> +					     "Neither VT nor DCB are enabled, "
> +					     "nb_tx_q > %d.",
> +
> IXGBE_NONE_MODE_TX_NB_QUEUES);
> +				return -EINVAL;
> +			}
> +		}
>  	}
>  	return 0;
>  }
> @@ -2856,9 +2866,14 @@ static void
>  ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *dev_info)
>  {
>  	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
> 
>  	dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues;
>  	dev_info->max_tx_queues = (uint16_t)hw->mac.max_tx_queues;
> +	if (RTE_ETH_DEV_SRIOV(dev).active == 0) {
> +		if (dev_conf->txmode.mq_mode == ETH_MQ_TX_NONE)
> +			dev_info->max_tx_queues =
> IXGBE_NONE_MODE_TX_NB_QUEUES;
> +	}
>  	dev_info->min_rx_bufsize = 1024; /* cf BSIZEPACKET in SRRCTL register
> */
>  	dev_info->max_rx_pktlen = 15872; /* includes CRC, cf MAXFRS register
> */
>  	dev_info->max_mac_addrs = hw->mac.num_rar_entries;
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> b/drivers/net/ixgbe/ixgbe_ethdev.h
> index 5c3aa16..691c62f 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -61,6 +61,7 @@
>  #define IXGBE_MAX_RX_QUEUE_NUM	128
>  #define IXGBE_VMDQ_DCB_NB_QUEUES     IXGBE_MAX_RX_QUEUE_NUM
>  #define IXGBE_DCB_NB_QUEUES          IXGBE_MAX_RX_QUEUE_NUM
> +#define IXGBE_NONE_MODE_TX_NB_QUEUES 64
> 
>  #ifndef NBBY
>  #define NBBY	8	/* number of bits in a byte */
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 8721a6b..b941b0d 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -901,6 +901,9 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t
> nb_rx_q, uint16_t nb_tx_q,
>  		return -EBUSY;
>  	}
> 
> +	/* Copy the dev_conf parameter into the dev structure */
> +	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data-
> >dev_conf));
> +
>  	/*
>  	 * Check that the numbers of RX and TX queues are not greater
>  	 * than the maximum number of RX and TX queues supported by the
> @@ -925,9 +928,6 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t
> nb_rx_q, uint16_t nb_tx_q,
>  		return -EINVAL;
>  	}
> 
> -	/* Copy the dev_conf parameter into the dev structure */
> -	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data-
> >dev_conf));
> -
>  	/*
>  	 * If link state interrupt is enabled, check that the
>  	 * device supports it.
> --
> 2.5.5