Re: [dpdk-dev] [PATCH 03/11] net/bnxt: implement vector mode driver

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 5/21/19 11:39 PM, Ajit Khaparde wrote:
> From: Lance Richardson <lance.richardson@broadcom.com>
> 
> Introduce vector mode support for the bnxt pmd.
> 
> Signed-off-by: Lance Richardson <lance.richardson@broadcom.com>
> Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>   config/common_base                   |   1 +
>   drivers/net/bnxt/Makefile            |   1 +
>   drivers/net/bnxt/bnxt_ethdev.c       |  92 ++++-
>   drivers/net/bnxt/bnxt_ring.h         |   3 +-
>   drivers/net/bnxt/bnxt_rxq.c          |   5 +
>   drivers/net/bnxt/bnxt_rxq.h          |   4 +
>   drivers/net/bnxt/bnxt_rxr.h          |   9 +-
>   drivers/net/bnxt/bnxt_rxtx_vec_sse.c | 481 +++++++++++++++++++++++++++
>   drivers/net/bnxt/bnxt_txr.h          |  11 +-
>   9 files changed, 598 insertions(+), 9 deletions(-)
>   create mode 100644 drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> 
> diff --git a/config/common_base b/config/common_base
> index 6b96e0e80..1bbb7c10b 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -212,6 +212,7 @@ CONFIG_RTE_LIBRTE_BNX2X_DEBUG_PERIODIC=n
>   # Compile burst-oriented Broadcom BNXT PMD driver
>   #
>   CONFIG_RTE_LIBRTE_BNXT_PMD=y
> +CONFIG_RTE_LIBRTE_BNXT_INC_VECTOR=n
>   
>   #
>   # Compile burst-oriented Chelsio Terminator (CXGBE) PMD
> diff --git a/drivers/net/bnxt/Makefile b/drivers/net/bnxt/Makefile
> index 8be3cb0e4..9e006b5d1 100644
> --- a/drivers/net/bnxt/Makefile
> +++ b/drivers/net/bnxt/Makefile
> @@ -41,6 +41,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_vnic.c
>   SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_irq.c
>   SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_util.c
>   SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += rte_pmd_bnxt.c
> +SRCS-$(CONFIG_RTE_LIBRTE_BNXT_INC_VECTOR) += bnxt_rxtx_vec_sse.c
>   
>   #
>   # Export include files

How do we enable it with Meson build? It seems to be missing.

> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index e0e0b72c6..064a153ec 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -645,6 +645,67 @@ static int bnxt_scattered_rx(struct rte_eth_dev *eth_dev)
>   	return 0;
>   }
>   
> +static eth_rx_burst_t
> +bnxt_receive_function(__rte_unused struct rte_eth_dev *eth_dev)
> +{
> +#ifdef RTE_LIBRTE_BNXT_INC_VECTOR
> +	/*
> +	 * Vector mode receive can be enabled only if scatter rx is not
> +	 * in use and rx offloads are limited to VLAN stripping and
> +	 * CRC stripping.
> +	 */
> +	if (!eth_dev->data->scattered_rx &&
> +	    !(eth_dev->data->dev_conf.rxmode.offloads &
> +	      ~(DEV_RX_OFFLOAD_VLAN_STRIP |
> +		DEV_RX_OFFLOAD_KEEP_CRC |
> +		DEV_RX_OFFLOAD_JUMBO_FRAME |
> +		DEV_RX_OFFLOAD_IPV4_CKSUM |
> +		DEV_RX_OFFLOAD_UDP_CKSUM |
> +		DEV_RX_OFFLOAD_TCP_CKSUM |
> +		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
> +		DEV_RX_OFFLOAD_VLAN_FILTER))) {
> +		PMD_DRV_LOG(INFO, "Using vector mode receive for port %d\n",
> +			    eth_dev->data->port_id);
> +		return bnxt_recv_pkts_vec;
> +	}
> +	PMD_DRV_LOG(INFO, "Vector mode receive disabled for port %d\n",
> +		    eth_dev->data->port_id);
> +	PMD_DRV_LOG(INFO,
> +		    "Port %d scatter: %d rx offload: %" PRIX64 "\n",
> +		    eth_dev->data->port_id,
> +		    eth_dev->data->scattered_rx,
> +		    eth_dev->data->dev_conf.rxmode.offloads);
> +#endif

I think we would better have a stub for bnxt_recv_pkts_vec() when
RTE_LIBRTE_BNXT_INC_VECTOR isn't set, that would avoid having all these
#ifdeferies in this file.

> +	return bnxt_recv_pkts;
> +}
> +
> +static eth_tx_burst_t
> +bnxt_transmit_function(__rte_unused struct rte_eth_dev *eth_dev)
> +{
> +#ifdef RTE_LIBRTE_BNXT_INC_VECTOR
> +	/*
> +	 * Vector mode receive can be enabled only if scatter tx is not
> +	 * in use and tx offloads other than VLAN insertion are not
> +	 * in use.
> +	 */
> +	if (!eth_dev->data->scattered_rx &&
> +	    !(eth_dev->data->dev_conf.txmode.offloads &
> +	      ~DEV_TX_OFFLOAD_VLAN_INSERT)) {
> +		PMD_DRV_LOG(INFO, "Using vector mode transmit for port %d\n",
> +			    eth_dev->data->port_id);
> +		return bnxt_xmit_pkts_vec;
> +	}
> +	PMD_DRV_LOG(INFO, "Vector mode transmit disabled for port %d\n",
> +		    eth_dev->data->port_id);
> +	PMD_DRV_LOG(INFO,
> +		    "Port %d scatter: %d tx offload: %" PRIX64 "\n",
> +		    eth_dev->data->port_id,
> +		    eth_dev->data->scattered_rx,
> +		    eth_dev->data->dev_conf.txmode.offloads);
> +#endif
> +	return bnxt_xmit_pkts;
> +}
> +
>   static int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
>   {
>   	struct bnxt *bp = (struct bnxt *)eth_dev->data->dev_private;
> @@ -675,6 +736,8 @@ static int bnxt_dev_start_op(struct rte_eth_dev *eth_dev)
>   	if (rc)
>   		goto error;
>   
> +	eth_dev->rx_pkt_burst = bnxt_receive_function(eth_dev);
> +	eth_dev->tx_pkt_burst = bnxt_transmit_function(eth_dev);
>   	bp->flags |= BNXT_FLAG_INIT_DONE;
>   	return 0;
>   
> @@ -1597,6 +1660,8 @@ bnxt_txq_info_get_op(struct rte_eth_dev *dev, uint16_t queue_id,
>   
>   static int bnxt_mtu_set_op(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
>   {
> +	uint32_t new_pkt_size = new_mtu + ETHER_HDR_LEN + ETHER_CRC_LEN +
> +				VLAN_TAG_SIZE * BNXT_NUM_VLANS;
>   	struct bnxt *bp = eth_dev->data->dev_private;
>   	struct rte_eth_dev_info dev_info;
>   	uint32_t rc = 0;
> @@ -1610,6 +1675,23 @@ static int bnxt_mtu_set_op(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
>   		return -EINVAL;
>   	}
>   
> +#ifdef RTE_LIBRTE_BNXT_INC_VECTOR
> +	/*
> +	 * If vector-mode tx/rx is active, disallow any MTU change that would
> +	 * require scattered receive support.
> +	 */
> +	if (eth_dev->data->dev_started &&
> +	    (eth_dev->rx_pkt_burst == bnxt_recv_pkts_vec ||
> +	     eth_dev->tx_pkt_burst == bnxt_xmit_pkts_vec) &&
> +	    (new_pkt_size >
> +	     eth_dev->data->min_rx_buf_size - RTE_PKTMBUF_HEADROOM)) {
> +		PMD_DRV_LOG(ERR,
> +			    "MTU change would require scattered rx support. ");
> +		PMD_DRV_LOG(ERR, "Stop port before changing MTU.\n");
> +		return -EINVAL;
> +	}
> +#endif
> +
>   	if (new_mtu > ETHER_MTU) {
>   		bp->flags |= BNXT_FLAG_JUMBO;
>   		bp->eth_dev->data->dev_conf.rxmode.offloads |=
> @@ -1620,8 +1702,7 @@ static int bnxt_mtu_set_op(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
>   		bp->flags &= ~BNXT_FLAG_JUMBO;
>   	}
>   
> -	eth_dev->data->dev_conf.rxmode.max_rx_pkt_len =
> -		new_mtu + ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE * 2;
> +	eth_dev->data->dev_conf.rxmode.max_rx_pkt_len = new_pkt_size;
>   
>   	eth_dev->data->mtu = new_mtu;
>   	PMD_DRV_LOG(INFO, "New MTU is %d\n", eth_dev->data->mtu);
> @@ -2655,9 +2736,10 @@ bnxt_dev_supported_ptypes_get_op(struct rte_eth_dev *dev)
>   		RTE_PTYPE_UNKNOWN
>   	};
>   
> -	if (dev->rx_pkt_burst == bnxt_recv_pkts)
> -		return ptypes;
> -	return NULL;
> +	if (!dev->rx_pkt_burst)
> +		return NULL;
> +
> +	return ptypes;
>   }
>   
>   static int bnxt_map_regs(struct bnxt *bp, uint32_t *reg_arr, int count,

...

> diff --git a/drivers/net/bnxt/bnxt_txr.h b/drivers/net/bnxt/bnxt_txr.h
> index 13ca04676..c28bd9c18 100644
> --- a/drivers/net/bnxt/bnxt_txr.h
> +++ b/drivers/net/bnxt/bnxt_txr.h
> @@ -37,13 +37,15 @@ struct bnxt_sw_tx_bd {
>   	unsigned short		nr_bds;
>   };
>   
> -static inline uint32_t bnxt_tx_bds_in_hw(struct bnxt_tx_queue *txq)
> +static inline uint32_t
> +bnxt_tx_bds_in_hw(struct bnxt_tx_queue *txq)

If to be done, it should be done in patch 1.

>   {
>   	return ((txq->tx_ring->tx_prod - txq->tx_ring->tx_cons) &
>   		txq->tx_ring->tx_ring_struct->ring_mask);
>   }
>   
> -static inline uint32_t bnxt_tx_avail(struct bnxt_tx_queue *txq)
> +static inline uint32_t
> +bnxt_tx_avail(struct bnxt_tx_queue *txq)

Ditto

>   {
>   	/* Tell compiler to fetch tx indices from memory. */
>   	rte_compiler_barrier();
> @@ -57,6 +59,11 @@ int bnxt_init_one_tx_ring(struct bnxt_tx_queue *txq);
>   int bnxt_init_tx_ring_struct(struct bnxt_tx_queue *txq, unsigned int socket_id);
>   uint16_t bnxt_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>   			       uint16_t nb_pkts);
> +#ifdef RTE_LIBRTE_BNXT_INC_VECTOR
> +uint16_t bnxt_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
> +			    uint16_t nb_pkts);
> +#endif
> +
>   int bnxt_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id);
>   int bnxt_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id);
>   
>