RE: [PATCH] ethdev: introduce generic dummy packet burst function

["Loftus, Ciara" <ciara.loftus@intel.com>]

> Subject: [PATCH] ethdev: introduce generic dummy packet burst function
> 
> Multiple PMDs have dummy/noop Rx/Tx packet burst functions.
> 
> These dummy functions are very simple, introduce a common function in
> the ethdev and update drivers to use it instead of each driver having
> its own functions.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Ciara Loftus <ciara.loftus@intel.com>
> ---

[snip]

> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index 3f3c4a7c7214..910b76a92c42 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -350,8 +350,8 @@ mlx4_dev_stop(struct rte_eth_dev *dev)
>  		return 0;
>  	DEBUG("%p: detaching flows from all RX queues", (void *)dev);
>  	priv->started = 0;
> -	dev->tx_pkt_burst = mlx4_tx_burst_removed;
> -	dev->rx_pkt_burst = mlx4_rx_burst_removed;
> +	dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
>  	rte_wmb();
>  	/* Disable datapath on secondary process. */
>  	mlx4_mp_req_stop_rxtx(dev);
> @@ -383,8 +383,8 @@ mlx4_dev_close(struct rte_eth_dev *dev)
>  	DEBUG("%p: closing device \"%s\"",
>  	      (void *)dev,
>  	      ((priv->ctx != NULL) ? priv->ctx->device->name : ""));
> -	dev->rx_pkt_burst = mlx4_rx_burst_removed;
> -	dev->tx_pkt_burst = mlx4_tx_burst_removed;
> +	dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
>  	rte_wmb();
>  	/* Disable datapath on secondary process. */
>  	mlx4_mp_req_stop_rxtx(dev);
> diff --git a/drivers/net/mlx4/mlx4_mp.c b/drivers/net/mlx4/mlx4_mp.c
> index 8fcfb5490ee9..1da64910aadd 100644
> --- a/drivers/net/mlx4/mlx4_mp.c
> +++ b/drivers/net/mlx4/mlx4_mp.c
> @@ -150,8 +150,8 @@ mp_secondary_handle(const struct rte_mp_msg
> *mp_msg, const void *peer)
>  		break;
>  	case MLX4_MP_REQ_STOP_RXTX:
>  		INFO("port %u stopping datapath", dev->data->port_id);
> -		dev->tx_pkt_burst = mlx4_tx_burst_removed;
> -		dev->rx_pkt_burst = mlx4_rx_burst_removed;
> +		dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
> +		dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
>  		rte_mb();
>  		mp_init_msg(dev, &mp_res, param->type);
>  		res->result = 0;
> diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c
> index ed9e41fcdea9..059e432a63fc 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.c
> +++ b/drivers/net/mlx4/mlx4_rxtx.c
> @@ -1338,55 +1338,3 @@ mlx4_rx_burst(void *dpdk_rxq, struct rte_mbuf
> **pkts, uint16_t pkts_n)
>  	rxq->stats.ipackets += i;
>  	return i;
>  }
> -
> -/**
> - * Dummy DPDK callback for Tx.
> - *
> - * This function is used to temporarily replace the real callback during
> - * unsafe control operations on the queue, or in case of error.
> - *
> - * @param dpdk_txq
> - *   Generic pointer to Tx queue structure.
> - * @param[in] pkts
> - *   Packets to transmit.
> - * @param pkts_n
> - *   Number of packets in array.
> - *
> - * @return
> - *   Number of packets successfully transmitted (<= pkts_n).
> - */
> -uint16_t
> -mlx4_tx_burst_removed(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t
> pkts_n)
> -{
> -	(void)dpdk_txq;
> -	(void)pkts;
> -	(void)pkts_n;
> -	rte_mb();

The mlx4 and mlx5 PMDs lose a call to rte_mb() when switching over to the new dummy functions. Maybe the maintainer can comment on whether that's an issue or not? Other than that LGTM.

Ciara

> -	return 0;
> -}
> -
> -/**
> - * Dummy DPDK callback for Rx.
> - *
> - * This function is used to temporarily replace the real callback during
> - * unsafe control operations on the queue, or in case of error.
> - *
> - * @param dpdk_rxq
> - *   Generic pointer to Rx queue structure.
> - * @param[out] pkts
> - *   Array to store received packets.
> - * @param pkts_n
> - *   Maximum number of packets in array.
> - *
> - * @return
> - *   Number of packets successfully received (<= pkts_n).
> - */
> -uint16_t
> -mlx4_rx_burst_removed(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t
> pkts_n)
> -{
> -	(void)dpdk_rxq;
> -	(void)pkts;
> -	(void)pkts_n;
> -	rte_mb();
> -	return 0;
> -}
> diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h
> index 83e9534cd0a7..70f3cd868058 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.h
> +++ b/drivers/net/mlx4/mlx4_rxtx.h
> @@ -149,10 +149,6 @@ uint16_t mlx4_tx_burst(void *dpdk_txq, struct
> rte_mbuf **pkts,
>  		       uint16_t pkts_n);
>  uint16_t mlx4_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts,
>  		       uint16_t pkts_n);
> -uint16_t mlx4_tx_burst_removed(void *dpdk_txq, struct rte_mbuf **pkts,
> -			       uint16_t pkts_n);
> -uint16_t mlx4_rx_burst_removed(void *dpdk_rxq, struct rte_mbuf **pkts,
> -			       uint16_t pkts_n);
> 
>  /* mlx4_txq.c */
> 
> diff --git a/drivers/net/mlx5/linux/mlx5_mp_os.c
> b/drivers/net/mlx5/linux/mlx5_mp_os.c
> index c448a3e9eb87..e607089e0e20 100644
> --- a/drivers/net/mlx5/linux/mlx5_mp_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_mp_os.c
> @@ -192,8 +192,8 @@ struct rte_mp_msg mp_res;
>  		break;
>  	case MLX5_MP_REQ_STOP_RXTX:
>  		DRV_LOG(INFO, "port %u stopping datapath", dev->data-
> >port_id);
> -		dev->rx_pkt_burst = removed_rx_burst;
> -		dev->tx_pkt_burst = removed_tx_burst;
> +		dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +		dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
>  		rte_mb();
>  		mp_init_msg(&priv->mp_id, &mp_res, param->type);
>  		res->result = 0;
> diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> b/drivers/net/mlx5/linux/mlx5_os.c
> index aecdc5a68abb..bbe05bb837e0 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -1623,8 +1623,8 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	DRV_LOG(DEBUG, "port %u MTU is %u", eth_dev->data->port_id,
>  		priv->mtu);
>  	/* Initialize burst functions to prevent crashes before link-up. */
> -	eth_dev->rx_pkt_burst = removed_rx_burst;
> -	eth_dev->tx_pkt_burst = removed_tx_burst;
> +	eth_dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	eth_dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
>  	eth_dev->dev_ops = &mlx5_dev_ops;
>  	eth_dev->rx_descriptor_status = mlx5_rx_descriptor_status;
>  	eth_dev->tx_descriptor_status = mlx5_tx_descriptor_status;
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 67eda41a60a5..5571e9067787 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1559,8 +1559,8 @@ mlx5_dev_close(struct rte_eth_dev *dev)
>  	mlx5_action_handle_flush(dev);
>  	mlx5_flow_meter_flush(dev, NULL);
>  	/* Prevent crashes when queues are still in use. */
> -	dev->rx_pkt_burst = removed_rx_burst;
> -	dev->tx_pkt_burst = removed_tx_burst;
> +	dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
>  	rte_wmb();
>  	/* Disable datapath on secondary process. */
>  	mlx5_mp_os_req_stop_rxtx(dev);
> diff --git a/drivers/net/mlx5/mlx5_rx.c b/drivers/net/mlx5/mlx5_rx.c
> index f388fcc31395..11ea935d72f0 100644
> --- a/drivers/net/mlx5/mlx5_rx.c
> +++ b/drivers/net/mlx5/mlx5_rx.c
> @@ -252,7 +252,7 @@ mlx5_rx_queue_count(void *rx_queue)
>  	dev = &rte_eth_devices[rxq->port_id];
> 
>  	if (dev->rx_pkt_burst == NULL ||
> -	    dev->rx_pkt_burst == removed_rx_burst) {
> +	    dev->rx_pkt_burst == rte_eth_pkt_burst_dummy) {
>  		rte_errno = ENOTSUP;
>  		return -rte_errno;
>  	}
> @@ -1153,31 +1153,6 @@ mlx5_rx_burst_mprq(void *dpdk_rxq, struct
> rte_mbuf **pkts, uint16_t pkts_n)
>  	return i;
>  }
> 
> -/**
> - * Dummy DPDK callback for RX.
> - *
> - * This function is used to temporarily replace the real callback during
> - * unsafe control operations on the queue, or in case of error.
> - *
> - * @param dpdk_rxq
> - *   Generic pointer to RX queue structure.
> - * @param[out] pkts
> - *   Array to store received packets.
> - * @param pkts_n
> - *   Maximum number of packets in array.
> - *
> - * @return
> - *   Number of packets successfully received (<= pkts_n).
> - */
> -uint16_t
> -removed_rx_burst(void *dpdk_rxq __rte_unused,
> -		 struct rte_mbuf **pkts __rte_unused,
> -		 uint16_t pkts_n __rte_unused)
> -{
> -	rte_mb();
> -	return 0;
> -}
> -
>  /*
>   * Vectorized Rx routines are not compiled in when required vector
> instructions
>   * are not supported on a target architecture.
> diff --git a/drivers/net/mlx5/mlx5_rx.h b/drivers/net/mlx5/mlx5_rx.h
> index cb5d51340db7..7e417819f7e8 100644
> --- a/drivers/net/mlx5/mlx5_rx.h
> +++ b/drivers/net/mlx5/mlx5_rx.h
> @@ -275,8 +275,6 @@ __rte_noinline int mlx5_rx_err_handle(struct
> mlx5_rxq_data *rxq, uint8_t vec);
>  void mlx5_mprq_buf_free(struct mlx5_mprq_buf *buf);
>  uint16_t mlx5_rx_burst_mprq(void *dpdk_rxq, struct rte_mbuf **pkts,
>  			    uint16_t pkts_n);
> -uint16_t removed_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts,
> -			  uint16_t pkts_n);
>  int mlx5_rx_descriptor_status(void *rx_queue, uint16_t offset);
>  uint32_t mlx5_rx_queue_count(void *rx_queue);
>  void mlx5_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
> diff --git a/drivers/net/mlx5/mlx5_trigger.c
> b/drivers/net/mlx5/mlx5_trigger.c
> index 74c9c0a4fff8..3a59237b1a7a 100644
> --- a/drivers/net/mlx5/mlx5_trigger.c
> +++ b/drivers/net/mlx5/mlx5_trigger.c
> @@ -1244,8 +1244,8 @@ mlx5_dev_stop(struct rte_eth_dev *dev)
> 
>  	dev->data->dev_started = 0;
>  	/* Prevent crashes when queues are still in use. */
> -	dev->rx_pkt_burst = removed_rx_burst;
> -	dev->tx_pkt_burst = removed_tx_burst;
> +	dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
>  	rte_wmb();
>  	/* Disable datapath on secondary process. */
>  	mlx5_mp_os_req_stop_rxtx(dev);
> diff --git a/drivers/net/mlx5/mlx5_tx.c b/drivers/net/mlx5/mlx5_tx.c
> index fd2cf2096753..8453b2701a9f 100644
> --- a/drivers/net/mlx5/mlx5_tx.c
> +++ b/drivers/net/mlx5/mlx5_tx.c
> @@ -135,31 +135,6 @@ mlx5_tx_error_cqe_handle(struct mlx5_txq_data
> *__rte_restrict txq,
>  	return 0;
>  }
> 
> -/**
> - * Dummy DPDK callback for TX.
> - *
> - * This function is used to temporarily replace the real callback during
> - * unsafe control operations on the queue, or in case of error.
> - *
> - * @param dpdk_txq
> - *   Generic pointer to TX queue structure.
> - * @param[in] pkts
> - *   Packets to transmit.
> - * @param pkts_n
> - *   Number of packets in array.
> - *
> - * @return
> - *   Number of packets successfully transmitted (<= pkts_n).
> - */
> -uint16_t
> -removed_tx_burst(void *dpdk_txq __rte_unused,
> -		 struct rte_mbuf **pkts __rte_unused,
> -		 uint16_t pkts_n __rte_unused)
> -{
> -	rte_mb();
> -	return 0;
> -}
> -
>  /**
>   * Update completion queue consuming index via doorbell
>   * and flush the completed data buffers.
> diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
> index 099e72935a3a..31eb0a1ce28e 100644
> --- a/drivers/net/mlx5/mlx5_tx.h
> +++ b/drivers/net/mlx5/mlx5_tx.h
> @@ -221,8 +221,6 @@ void mlx5_txq_dynf_timestamp_set(struct
> rte_eth_dev *dev);
> 
>  /* mlx5_tx.c */
> 
> -uint16_t removed_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts,
> -			  uint16_t pkts_n);
>  void mlx5_tx_handle_completion(struct mlx5_txq_data *__rte_restrict txq,
>  			       unsigned int olx __rte_unused);
>  int mlx5_tx_descriptor_status(void *tx_queue, uint16_t offset);
> diff --git a/drivers/net/mlx5/windows/mlx5_os.c
> b/drivers/net/mlx5/windows/mlx5_os.c
> index ac0af0ff7d43..7f3532426f1f 100644
> --- a/drivers/net/mlx5/windows/mlx5_os.c
> +++ b/drivers/net/mlx5/windows/mlx5_os.c
> @@ -574,8 +574,8 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	DRV_LOG(DEBUG, "port %u MTU is %u.", eth_dev->data->port_id,
>  		priv->mtu);
>  	/* Initialize burst functions to prevent crashes before link-up. */
> -	eth_dev->rx_pkt_burst = removed_rx_burst;
> -	eth_dev->tx_pkt_burst = removed_tx_burst;
> +	eth_dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	eth_dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
>  	eth_dev->dev_ops = &mlx5_dev_ops;
>  	eth_dev->rx_descriptor_status = mlx5_rx_descriptor_status;
>  	eth_dev->tx_descriptor_status = mlx5_tx_descriptor_status;
> diff --git a/drivers/net/pfe/pfe_ethdev.c b/drivers/net/pfe/pfe_ethdev.c
> index edf32aa70da6..c2991ab1ccaa 100644
> --- a/drivers/net/pfe/pfe_ethdev.c
> +++ b/drivers/net/pfe/pfe_ethdev.c
> @@ -235,22 +235,6 @@ pfe_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pkts)
>  	return nb_pkts;
>  }
> 
> -static uint16_t
> -pfe_dummy_xmit_pkts(__rte_unused void *tx_queue,
> -		__rte_unused struct rte_mbuf **tx_pkts,
> -		__rte_unused uint16_t nb_pkts)
> -{
> -	return 0;
> -}
> -
> -static uint16_t
> -pfe_dummy_recv_pkts(__rte_unused void *rxq,
> -		__rte_unused struct rte_mbuf **rx_pkts,
> -		__rte_unused uint16_t nb_pkts)
> -{
> -	return 0;
> -}
> -
>  static int
>  pfe_eth_open(struct rte_eth_dev *dev)
>  {
> @@ -383,8 +367,8 @@ pfe_eth_stop(struct rte_eth_dev *dev/*, int
> wake*/)
>  	gemac_disable(priv->EMAC_baseaddr);
>  	gpi_disable(priv->GPI_baseaddr);
> 
> -	dev->rx_pkt_burst = &pfe_dummy_recv_pkts;
> -	dev->tx_pkt_burst = &pfe_dummy_xmit_pkts;
> +	dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
> 
>  	return 0;
>  }
> diff --git a/drivers/net/qede/qede_ethdev.c
> b/drivers/net/qede/qede_ethdev.c
> index a1122a297e6b..ea6b71f09355 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -322,8 +322,8 @@ qede_assign_rxtx_handlers(struct rte_eth_dev *dev,
> bool is_dummy)
>  	bool use_tx_offload = false;
> 
>  	if (is_dummy) {
> -		dev->rx_pkt_burst = qede_rxtx_pkts_dummy;
> -		dev->tx_pkt_burst = qede_rxtx_pkts_dummy;
> +		dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +		dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
>  		return;
>  	}
> 
> diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
> index 7088c57b501d..85784f4a82a6 100644
> --- a/drivers/net/qede/qede_rxtx.c
> +++ b/drivers/net/qede/qede_rxtx.c
> @@ -2734,15 +2734,6 @@ qede_xmit_pkts_cmt(void *p_fp_cmt, struct
> rte_mbuf **tx_pkts, uint16_t nb_pkts)
>  	return eng0_pkts + eng1_pkts;
>  }
> 
> -uint16_t
> -qede_rxtx_pkts_dummy(__rte_unused void *p_rxq,
> -		     __rte_unused struct rte_mbuf **pkts,
> -		     __rte_unused uint16_t nb_pkts)
> -{
> -	return 0;
> -}
> -
> -
>  /* this function does a fake walk through over completion queue
>   * to calculate number of BDs used by HW.
>   * At the end, it restores the state of completion queue.
> diff --git a/drivers/net/qede/qede_rxtx.h b/drivers/net/qede/qede_rxtx.h
> index 11ed1d9b9c50..013a4a07c716 100644
> --- a/drivers/net/qede/qede_rxtx.h
> +++ b/drivers/net/qede/qede_rxtx.h
> @@ -272,9 +272,6 @@ uint16_t qede_recv_pkts_cmt(void *p_rxq, struct
> rte_mbuf **rx_pkts,
>  uint16_t
>  qede_recv_pkts_regular(void *p_rxq, struct rte_mbuf **rx_pkts,
>  		       uint16_t nb_pkts);
> -uint16_t qede_rxtx_pkts_dummy(void *p_rxq,
> -			      struct rte_mbuf **pkts,
> -			      uint16_t nb_pkts);
> 
>  int qede_start_queues(struct rte_eth_dev *eth_dev);
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 8f0ac0adf0ae..075f97a4b37a 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1432,6 +1432,25 @@ rte_eth_linkstatus_get(const struct rte_eth_dev
> *dev,
>  	*dst = __atomic_load_n(src, __ATOMIC_SEQ_CST);
>  }
> 
> +/**
> + * @internal
> + * Dummy DPDK callback for Rx/Tx packet burst.
> + *
> + * @param queue
> + *  Pointer to Rx/Tx queue
> + * @param pkts
> + *  Packet array
> + * @param nb_pkts
> + *  Number of packets in packet array
> + */
> +static inline uint16_t
> +rte_eth_pkt_burst_dummy(void *queue __rte_unused,
> +		struct rte_mbuf **pkts __rte_unused,
> +		uint16_t nb_pkts __rte_unused)
> +{
> +	return 0;
> +}
> +
>  /**
>   * Allocate an unique switch domain identifier.
>   *
> --
> 2.34.1