[v4,02/15] ethdev: add support for hairpin queue

  This commit introduce hairpin queue type.

The hairpin queue in build from Rx queue binded to Tx queue.
It is used to offload traffic coming from the wire and redirect it back
to the wire.

There are 3 new functions:
- rte_eth_dev_hairpin_capability_get
- rte_eth_rx_hairpin_queue_setup
- rte_eth_tx_hairpin_queue_setup

In order to use the queue, there is a need to create rte_flow
with queue / RSS action that targets one or more of the Rx queues.

Signed-off-by: Ori Kam <orika@mellanox.com>

---
V4:
 - update according to ML comments.

V3:
 - update according to ML comments.

V2:
 - update according to ML comments.

---
 lib/librte_ethdev/rte_ethdev.c           | 229 +++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 143 ++++++++++++++++++-
 lib/librte_ethdev/rte_ethdev_core.h      |  91 +++++++++++-
 lib/librte_ethdev/rte_ethdev_driver.h    |   1 +
 lib/librte_ethdev/rte_ethdev_version.map |   5 +
 5 files changed, 461 insertions(+), 8 deletions(-)
  

17/10/2019 17:32, Ori Kam:
> V4:
>  - update according to ML comments.
> 
> V3:
>  - update according to ML comments.
> 
> V2:
>  - update according to ML comments.

I would prefer to see a summary of the changes in the changelog,
so we can easily track the progress.
  

Hi Ori,

see my notes below.

A generic note is that we have strict policy about Rx/Tx (not RX/TX) in
commit messages, but I'd like to follow it in comments and log messages
at least in a new code. It is already a mixture in the existing code.

On 10/17/19 6:32 PM, Ori Kam wrote:
> This commit introduce hairpin queue type.
>
> The hairpin queue in build from Rx queue binded to Tx queue.
> It is used to offload traffic coming from the wire and redirect it back
> to the wire.
>
> There are 3 new functions:
> - rte_eth_dev_hairpin_capability_get
> - rte_eth_rx_hairpin_queue_setup
> - rte_eth_tx_hairpin_queue_setup
>
> In order to use the queue, there is a need to create rte_flow
> with queue / RSS action that targets one or more of the Rx queues.
>
> Signed-off-by: Ori Kam <orika@mellanox.com>
>
> ---
> V4:
>   - update according to ML comments.
>
> V3:
>   - update according to ML comments.
>
> V2:
>   - update according to ML comments.
>
> ---
>   lib/librte_ethdev/rte_ethdev.c           | 229 +++++++++++++++++++++++++++++++
>   lib/librte_ethdev/rte_ethdev.h           | 143 ++++++++++++++++++-
>   lib/librte_ethdev/rte_ethdev_core.h      |  91 +++++++++++-
>   lib/librte_ethdev/rte_ethdev_driver.h    |   1 +
>   lib/librte_ethdev/rte_ethdev_version.map |   5 +
>   5 files changed, 461 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index af82360..10a8bf2 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -904,6 +904,14 @@ struct rte_eth_dev *
>   
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
>   
> +	if (dev->data->rx_queue_state[rx_queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {

May I suggest to add helper static function to check
if device Rx queue is hairpin. Plus similar function for Tx.
It will allow to make changes in rx_queue_state less
intrusive.
These functions should be used everywhere below in
the code.

> +		RTE_ETHDEV_LOG(INFO,
> +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is hairpin queue\n",

The message is the same for many cases and it is bad since it does
not allow to identify place where it was logged easily.
It should be mentioned here that it is an attempt to start Rx queue.

> +			rx_queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
>   	if (dev->data->rx_queue_state[rx_queue_id] != RTE_ETH_QUEUE_STATE_STOPPED) {
>   		RTE_ETHDEV_LOG(INFO,
>   			"Queue %"PRIu16" of device with port_id=%"PRIu16" already started\n",
> @@ -931,6 +939,14 @@ struct rte_eth_dev *
>   
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
>   
> +	if (dev->data->rx_queue_state[rx_queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		RTE_ETHDEV_LOG(INFO,
> +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is hairpin queue\n",
> +			rx_queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
>   	if (dev->data->rx_queue_state[rx_queue_id] == RTE_ETH_QUEUE_STATE_STOPPED) {
>   		RTE_ETHDEV_LOG(INFO,
>   			"Queue %"PRIu16" of device with port_id=%"PRIu16" already stopped\n",
> @@ -964,6 +980,14 @@ struct rte_eth_dev *
>   
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
>   
> +	if (dev->data->tx_queue_state[tx_queue_id] ==
> +	   RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		RTE_ETHDEV_LOG(INFO,
> +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is hairpin queue\n",
> +			tx_queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
>   	if (dev->data->tx_queue_state[tx_queue_id] != RTE_ETH_QUEUE_STATE_STOPPED) {
>   		RTE_ETHDEV_LOG(INFO,
>   			"Queue %"PRIu16" of device with port_id=%"PRIu16" already started\n",
> @@ -989,6 +1013,14 @@ struct rte_eth_dev *
>   
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
>   
> +	if (dev->data->tx_queue_state[tx_queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		RTE_ETHDEV_LOG(INFO,
> +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is hairpin queue\n",
> +			tx_queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
>   	if (dev->data->tx_queue_state[tx_queue_id] == RTE_ETH_QUEUE_STATE_STOPPED) {
>   		RTE_ETHDEV_LOG(INFO,
>   			"Queue %"PRIu16" of device with port_id=%"PRIu16" already stopped\n",
> @@ -1758,6 +1790,81 @@ struct rte_eth_dev *
>   }
>   
>   int
> +rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> +			       uint16_t nb_rx_desc,
> +			       const struct rte_eth_hairpin_conf *conf)
> +{
> +	int ret;
> +	struct rte_eth_dev *dev;
> +	struct rte_eth_hairpin_cap cap;
> +	void **rxq;
> +	int i;
> +	int count = 0;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> +
> +	dev = &rte_eth_devices[port_id];
> +	if (rx_queue_id >= dev->data->nb_rx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
> +		return -EINVAL;
> +	}
> +	ret = rte_eth_dev_hairpin_capability_get(port_id, &cap);
> +	if (ret != 0)
> +		return ret;
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_hairpin_queue_setup,
> +				-ENOTSUP);
> +	/* Use default specified by driver, if nb_rx_desc is zero */
> +	if (nb_rx_desc == 0)
> +		nb_rx_desc = cap.max_nb_desc;
> +	if (nb_rx_desc > cap.max_nb_desc) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Invalid value for nb_rx_desc(=%hu), should be: "
> +			       "<= %hu", nb_rx_desc, cap.max_nb_desc);

Please, don't split format string

> +		return -EINVAL;
> +	}
> +	if (conf->peer_n > cap.max_rx_2_tx) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Invalid value for number of peers(=%hu), "
> +			       "should be: <= %hu", conf->peer_n,

Please, don't split format string.
Also make the message unique. Right now it is same for Rx and Tx.

> +			       cap.max_rx_2_tx);
> +		return -EINVAL;
> +	}
> +	if (conf->peer_n == 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Invalid value for number of peers(=%hu), "
> +			       "should be: > 0", conf->peer_n);

Please, don't split format string
Also make the message unique. Right now it is same for Rx and Tx.

> +		return -EINVAL;
> +	}
> +	if (cap.max_n_queues != UINT16_MAX) {
> +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +			if (dev->data->rx_queue_state[i] ==
> +			    RTE_ETH_QUEUE_STATE_HAIRPIN)
> +				count++;
> +		}
> +		if (count > cap.max_n_queues) {
> +			RTE_ETHDEV_LOG(ERR,
> +				       "To many Rx hairpin queues %d", count);
> +			return -EINVAL;
> +		}
> +	}
> +	if (dev->data->dev_started)
> +		return -EBUSY;
> +	rxq = dev->data->rx_queues;
> +	if (rxq[rx_queue_id] != NULL) {
> +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release,
> +					-ENOTSUP);
> +		(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
> +		rxq[rx_queue_id] = NULL;
> +	}
> +	ret = (*dev->dev_ops->rx_hairpin_queue_setup)(dev, rx_queue_id,
> +						      nb_rx_desc, conf);
> +	if (ret == 0)
> +		dev->data->rx_queue_state[rx_queue_id] =
> +			RTE_ETH_QUEUE_STATE_HAIRPIN;
> +	return eth_err(port_id, ret);
> +}
> +
> +int
>   rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>   		       uint16_t nb_tx_desc, unsigned int socket_id,
>   		       const struct rte_eth_txconf *tx_conf)
> @@ -1856,6 +1963,80 @@ struct rte_eth_dev *
>   		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));
>   }
>   
> +int
> +rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
> +			       uint16_t nb_tx_desc,
> +			       const struct rte_eth_hairpin_conf *conf)
> +{
> +	struct rte_eth_dev *dev;
> +	struct rte_eth_hairpin_cap cap;
> +	void **txq;
> +	int i;
> +	int count = 0;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> +	dev = &rte_eth_devices[port_id];
> +	if (tx_queue_id >= dev->data->nb_tx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
> +		return -EINVAL;
> +	}
> +	ret = rte_eth_dev_hairpin_capability_get(port_id, &cap);
> +	if (ret != 0)
> +		return ret;
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_hairpin_queue_setup,
> +				-ENOTSUP);
> +	/* Use default specified by driver, if nb_tx_desc is zero */
> +	if (nb_tx_desc == 0)
> +		nb_tx_desc = cap.max_nb_desc;
> +	if (nb_tx_desc > cap.max_nb_desc) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Invalid value for nb_tx_desc(=%hu), should be: "
> +			       "<= %hu", nb_tx_desc, cap.max_nb_desc);

Please, don't split format string

> +		return -EINVAL;
> +	}
> +	if (conf->peer_n > cap.max_tx_2_rx) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Invalid value for number of peers(=%hu), "
> +			       "should be: <= %hu", conf->peer_n,

Please, don't split format string

> +			       cap.max_tx_2_rx);
> +		return -EINVAL;
> +	}
> +	if (conf->peer_n == 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Invalid value for number of peers(=%hu), "
> +			       "should be: > 0", conf->peer_n);

Please, don't split format string

> +		return -EINVAL;
> +	}
> +	if (cap.max_n_queues != UINT16_MAX) {
> +		for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +			if (dev->data->tx_queue_state[i] ==
> +			    RTE_ETH_QUEUE_STATE_HAIRPIN)
> +				count++;
> +		}
> +		if (count > cap.max_n_queues) {
> +			RTE_ETHDEV_LOG(ERR,
> +				       "To many Rx hairpin queues %d", count);
> +			return -EINVAL;
> +		}
> +	}
> +	if (dev->data->dev_started)
> +		return -EBUSY;
> +	txq = dev->data->tx_queues;
> +	if (txq[tx_queue_id] != NULL) {
> +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release,
> +					-ENOTSUP);
> +		(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
> +		txq[tx_queue_id] = NULL;
> +	}
> +	ret = (*dev->dev_ops->tx_hairpin_queue_setup)
> +		(dev, tx_queue_id, nb_tx_desc, conf);
> +	if (ret == 0)
> +		dev->data->tx_queue_state[tx_queue_id] =
> +			RTE_ETH_QUEUE_STATE_HAIRPIN;
> +	return eth_err(port_id, ret);
> +}
> +
>   void
>   rte_eth_tx_buffer_drop_callback(struct rte_mbuf **pkts, uint16_t unsent,
>   		void *userdata __rte_unused)
> @@ -3981,12 +4162,20 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   	rte_errno = ENOTSUP;
>   	return NULL;
>   #endif
> +	struct rte_eth_dev *dev;
> +
>   	/* check input parameters */
>   	if (!rte_eth_dev_is_valid_port(port_id) || fn == NULL ||
>   		    queue_id >= rte_eth_devices[port_id].data->nb_rx_queues) {
>   		rte_errno = EINVAL;
>   		return NULL;
>   	}
> +	dev = &rte_eth_devices[port_id];
> +	if (dev->data->rx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		rte_errno = EINVAL;
> +		return NULL;
> +	}
>   	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
>   
>   	if (cb == NULL) {
> @@ -4058,6 +4247,8 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   	rte_errno = ENOTSUP;
>   	return NULL;
>   #endif
> +	struct rte_eth_dev *dev;
> +
>   	/* check input parameters */
>   	if (!rte_eth_dev_is_valid_port(port_id) || fn == NULL ||
>   		    queue_id >= rte_eth_devices[port_id].data->nb_tx_queues) {
> @@ -4065,6 +4256,13 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   		return NULL;
>   	}
>   
> +	dev = &rte_eth_devices[port_id];
> +	if (dev->data->tx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		rte_errno = EINVAL;
> +		return NULL;
> +	}
> +
>   	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
>   
>   	if (cb == NULL) {
> @@ -4180,6 +4378,14 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rxq_info_get, -ENOTSUP);
>   
> +	if (dev->data->rx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		RTE_ETHDEV_LOG(INFO,
> +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is hairpin queue\n",

I think it would be useful if log mentions what caller tried to do.
See note about log messages uniqueness, e.g.
"Cannot get RxQ info: port %"PRIu16" queue %"PRIu16" is hairpin"
Also I think it is better to check it before rxq_info_get check
mainly to put it nearby queue range check to group all
queue ID checks together.

> +			queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
>   	memset(qinfo, 0, sizeof(*qinfo));
>   	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
>   	return 0;
> @@ -4202,6 +4408,14 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   		return -EINVAL;
>   	}
>   
> +	if (dev->data->tx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		RTE_ETHDEV_LOG(INFO,
> +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is hairpin queue\n",

Same as above.

> +			queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->txq_info_get, -ENOTSUP);
>   
>   	memset(qinfo, 0, sizeof(*qinfo));
> @@ -4510,6 +4724,21 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   }
>   
>   int
> +rte_eth_dev_hairpin_capability_get(uint16_t port_id,
> +				   struct rte_eth_hairpin_cap *cap)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> +
> +	dev = &rte_eth_devices[port_id];
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hairpin_cap_get,
> +				-ENOTSUP);
> +	memset(cap, 0, sizeof(*cap));
> +	return eth_err(port_id, (*dev->dev_ops->hairpin_cap_get)(dev, cap));
> +}
> +
> +int
>   rte_eth_dev_pool_ops_supported(uint16_t port_id, const char *pool)
>   {
>   	struct rte_eth_dev *dev;
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 187a2bb..276f55f 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -804,6 +804,46 @@ struct rte_eth_txconf {
>   };
>   
>   /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * A structure used to return the hairpin capabilities that are supported.
> + */
> +struct rte_eth_hairpin_cap {
> +	uint16_t max_n_queues;
> +	/**< The max number of hairpin queues (different bindings). */
> +	uint16_t max_rx_2_tx;
> +	/**< Max number of Rx queues to be connected to one Tx queue. */
> +	uint16_t max_tx_2_rx;
> +	/**< Max number of Tx queues to be connected to one Rx queue. */
> +	uint16_t max_nb_desc; /**< The max num of descriptors. */
> +};
> +
> +#define RTE_ETH_MAX_HAIRPIN_PEERS 32
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * A structure used to hold hairpin peer data.
> + */
> +struct rte_eth_hairpin_peer {
> +	uint16_t port; /**< Peer port. */
> +	uint16_t queue; /**< Peer queue. */
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * A structure used to configure hairpin binding.
> + */
> +struct rte_eth_hairpin_conf {
> +	uint16_t peer_n; /**< The number of peers. */
> +	struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
> +};
> +
> +/**
>    * A structure contains information about HW descriptor ring limitations.
>    */
>   struct rte_eth_desc_lim {
> @@ -1765,6 +1805,37 @@ int rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>   		struct rte_mempool *mb_pool);
>   
>   /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Allocate and set up a hairpin receive queue for an Ethernet device.
> + *
> + * The function set up the selected queue to be used in hairpin.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param rx_queue_id
> + *   The index of the receive queue to set up.
> + *   The value must be in the range [0, nb_rx_queue - 1] previously supplied
> + *   to rte_eth_dev_configure().
> + * @param nb_rx_desc
> + *   The number of receive descriptors to allocate for the receive ring.
> + *   0 means the PMD will use default value.
> + * @param conf
> + *   The pointer to the hairpin configuration.
> + *
> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-EINVAL) if bad parameter.
> + *   - (-ENOMEM) if unable to allocate the resources.
> + */
> +__rte_experimental
> +int rte_eth_rx_hairpin_queue_setup
> +	(uint16_t port_id, uint16_t rx_queue_id, uint16_t nb_rx_desc,
> +	 const struct rte_eth_hairpin_conf *conf);
> +
> +/**
>    * Allocate and set up a transmit queue for an Ethernet device.
>    *
>    * @param port_id
> @@ -1817,6 +1888,35 @@ int rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>   		const struct rte_eth_txconf *tx_conf);
>   
>   /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Allocate and set up a transmit hairpin queue for an Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param tx_queue_id
> + *   The index of the transmit queue to set up.
> + *   The value must be in the range [0, nb_tx_queue - 1] previously supplied
> + *   to rte_eth_dev_configure().
> + * @param nb_tx_desc
> + *   The number of transmit descriptors to allocate for the transmit ring.
> + *   0 to set default PMD value.
> + * @param conf
> + *   The hairpin configuration.
> + *
> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-EINVAL) if bad parameter.
> + *   - (-ENOMEM) if unable to allocate the resources.
> + */
> +__rte_experimental
> +int rte_eth_tx_hairpin_queue_setup
> +	(uint16_t port_id, uint16_t tx_queue_id, uint16_t nb_tx_desc,
> +	 const struct rte_eth_hairpin_conf *conf);
> +
> +/**
>    * Return the NUMA socket to which an Ethernet device is connected
>    *
>    * @param port_id
> @@ -1851,7 +1951,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>    *   to rte_eth_dev_configure().
>    * @return
>    *   - 0: Success, the receive queue is started.
> - *   - -EINVAL: The port_id or the queue_id out of range.
> + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
>    *   - -EIO: if device is removed.
>    *   - -ENOTSUP: The function not supported in PMD driver.
>    */
> @@ -1868,7 +1968,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>    *   to rte_eth_dev_configure().
>    * @return
>    *   - 0: Success, the receive queue is stopped.
> - *   - -EINVAL: The port_id or the queue_id out of range.
> + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
>    *   - -EIO: if device is removed.
>    *   - -ENOTSUP: The function not supported in PMD driver.
>    */
> @@ -1886,7 +1986,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>    *   to rte_eth_dev_configure().
>    * @return
>    *   - 0: Success, the transmit queue is started.
> - *   - -EINVAL: The port_id or the queue_id out of range.
> + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
>    *   - -EIO: if device is removed.
>    *   - -ENOTSUP: The function not supported in PMD driver.
>    */
> @@ -1903,7 +2003,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>    *   to rte_eth_dev_configure().
>    * @return
>    *   - 0: Success, the transmit queue is stopped.
> - *   - -EINVAL: The port_id or the queue_id out of range.
> + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
>    *   - -EIO: if device is removed.
>    *   - -ENOTSUP: The function not supported in PMD driver.
>    */
> @@ -3569,7 +3669,8 @@ int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
>    * @return
>    *   - 0: Success
>    *   - -ENOTSUP: routine is not supported by the device PMD.
> - *   - -EINVAL:  The port_id or the queue_id is out of range.
> + *   - -EINVAL:  The port_id or the queue_id is out of range, or the queue
> + *               is hairpin queue.
>    */
>   int rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>   	struct rte_eth_rxq_info *qinfo);
> @@ -3589,7 +3690,8 @@ int rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>    * @return
>    *   - 0: Success
>    *   - -ENOTSUP: routine is not supported by the device PMD.
> - *   - -EINVAL:  The port_id or the queue_id is out of range.
> + *   - -EINVAL:  The port_id or the queue_id is out of range, or the queue
> + *               is hairpin queue.
>    */
>   int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>   	struct rte_eth_txq_info *qinfo);
> @@ -4031,6 +4133,23 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t port_id,
>   void *
>   rte_eth_dev_get_sec_ctx(uint16_t port_id);
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Query the device hairpin capabilities.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param cap
> + *   Pointer to a structure that will hold the hairpin capabilities.
> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + */
> +__rte_experimental
> +int rte_eth_dev_hairpin_capability_get(uint16_t port_id,
> +				       struct rte_eth_hairpin_cap *cap);
>   
>   #include <rte_ethdev_core.h>
>   
> @@ -4131,6 +4250,12 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t port_id,
>   		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", queue_id);
>   		return 0;
>   	}
> +	if (dev->data->rx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		RTE_ETHDEV_LOG(ERR, "RX queue_id=%u is hairpin queue\n",

I see but these log messages are very similar to above, but I still think
it would be useful to mention context to make it clear in log, e.g.
"Cannot Rx from hairpin queue%"PRIu16" at port %"PRIu16

> +			       queue_id);
> +		return 0;
> +	}
>   #endif
>   	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
>   				     rx_pkts, nb_pkts);
> @@ -4397,6 +4522,12 @@ static inline int rte_eth_tx_descriptor_status(uint16_t port_id,
>   		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", queue_id);
>   		return 0;
>   	}
> +	if (dev->data->tx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> +		RTE_ETHDEV_LOG(ERR, "TX queue_id=%u is hairpin queue\n",
> +			       queue_id);

I think it would be useful to mention context to make it clear in log, e.g.
"Cannot Tx to hairpin queue%"PRIu16" at port %"PRIu16

> +		return 0;
> +	}
>   #endif
>   
>   #ifdef RTE_ETHDEV_RXTX_CALLBACKS

[snip]
  

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Tuesday, October 22, 2019 2:38 PM
> To: Ori Kam <orika@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; jingjing.wu@intel.com; stephen@networkplumber.org
> Subject: Re: [PATCH v4 02/15] ethdev: add support for hairpin queue
> 
> Hi Ori,
> 
> see my notes below.
> 
> A generic note is that we have strict policy about Rx/Tx (not RX/TX) in
> commit messages, but I'd like to follow it in comments and log messages
> at least in a new code. It is already a mixture in the existing code.
>

O.K. will make sure my code is aligned.
 
> On 10/17/19 6:32 PM, Ori Kam wrote:
> > This commit introduce hairpin queue type.
> >
> > The hairpin queue in build from Rx queue binded to Tx queue.
> > It is used to offload traffic coming from the wire and redirect it back
> > to the wire.
> >
> > There are 3 new functions:
> > - rte_eth_dev_hairpin_capability_get
> > - rte_eth_rx_hairpin_queue_setup
> > - rte_eth_tx_hairpin_queue_setup
> >
> > In order to use the queue, there is a need to create rte_flow
> > with queue / RSS action that targets one or more of the Rx queues.
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> >
> > ---
> > V4:
> >   - update according to ML comments.
> >
> > V3:
> >   - update according to ML comments.
> >
> > V2:
> >   - update according to ML comments.
> >
> > ---
> >   lib/librte_ethdev/rte_ethdev.c           | 229
> +++++++++++++++++++++++++++++++
> >   lib/librte_ethdev/rte_ethdev.h           | 143 ++++++++++++++++++-
> >   lib/librte_ethdev/rte_ethdev_core.h      |  91 +++++++++++-
> >   lib/librte_ethdev/rte_ethdev_driver.h    |   1 +
> >   lib/librte_ethdev/rte_ethdev_version.map |   5 +
> >   5 files changed, 461 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index af82360..10a8bf2 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -904,6 +904,14 @@ struct rte_eth_dev *
> >
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -
> ENOTSUP);
> >
> > +	if (dev->data->rx_queue_state[rx_queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> 
> May I suggest to add helper static function to check
> if device Rx queue is hairpin. Plus similar function for Tx.
> It will allow to make changes in rx_queue_state less
> intrusive.
> These functions should be used everywhere below in
> the code.
> 

Agree, will change.

> > +		RTE_ETHDEV_LOG(INFO,
> > +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is
> hairpin queue\n",
> 
> The message is the same for many cases and it is bad since it does
> not allow to identify place where it was logged easily.
> It should be mentioned here that it is an attempt to start Rx queue.
> 

O.K will change.

> > +			rx_queue_id, port_id);
> > +		return -EINVAL;
> > +	}
> > +
> >   	if (dev->data->rx_queue_state[rx_queue_id] !=
> RTE_ETH_QUEUE_STATE_STOPPED) {
> >   		RTE_ETHDEV_LOG(INFO,
> >   			"Queue %"PRIu16" of device with port_id=%"PRIu16"
> already started\n",
> > @@ -931,6 +939,14 @@ struct rte_eth_dev *
> >
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -
> ENOTSUP);
> >
> > +	if (dev->data->rx_queue_state[rx_queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		RTE_ETHDEV_LOG(INFO,
> > +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is
> hairpin queue\n",
> > +			rx_queue_id, port_id);
> > +		return -EINVAL;
> > +	}
> > +
> >   	if (dev->data->rx_queue_state[rx_queue_id] ==
> RTE_ETH_QUEUE_STATE_STOPPED) {
> >   		RTE_ETHDEV_LOG(INFO,
> >   			"Queue %"PRIu16" of device with port_id=%"PRIu16"
> already stopped\n",
> > @@ -964,6 +980,14 @@ struct rte_eth_dev *
> >
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -
> ENOTSUP);
> >
> > +	if (dev->data->tx_queue_state[tx_queue_id] ==
> > +	   RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		RTE_ETHDEV_LOG(INFO,
> > +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is
> hairpin queue\n",
> > +			tx_queue_id, port_id);
> > +		return -EINVAL;
> > +	}
> > +
> >   	if (dev->data->tx_queue_state[tx_queue_id] !=
> RTE_ETH_QUEUE_STATE_STOPPED) {
> >   		RTE_ETHDEV_LOG(INFO,
> >   			"Queue %"PRIu16" of device with port_id=%"PRIu16"
> already started\n",
> > @@ -989,6 +1013,14 @@ struct rte_eth_dev *
> >
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -
> ENOTSUP);
> >
> > +	if (dev->data->tx_queue_state[tx_queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		RTE_ETHDEV_LOG(INFO,
> > +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is
> hairpin queue\n",
> > +			tx_queue_id, port_id);
> > +		return -EINVAL;
> > +	}
> > +
> >   	if (dev->data->tx_queue_state[tx_queue_id] ==
> RTE_ETH_QUEUE_STATE_STOPPED) {
> >   		RTE_ETHDEV_LOG(INFO,
> >   			"Queue %"PRIu16" of device with port_id=%"PRIu16"
> already stopped\n",
> > @@ -1758,6 +1790,81 @@ struct rte_eth_dev *
> >   }
> >
> >   int
> > +rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> > +			       uint16_t nb_rx_desc,
> > +			       const struct rte_eth_hairpin_conf *conf)
> > +{
> > +	int ret;
> > +	struct rte_eth_dev *dev;
> > +	struct rte_eth_hairpin_cap cap;
> > +	void **rxq;
> > +	int i;
> > +	int count = 0;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > +
> > +	dev = &rte_eth_devices[port_id];
> > +	if (rx_queue_id >= dev->data->nb_rx_queues) {
> > +		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n",
> rx_queue_id);
> > +		return -EINVAL;
> > +	}
> > +	ret = rte_eth_dev_hairpin_capability_get(port_id, &cap);
> > +	if (ret != 0)
> > +		return ret;
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >rx_hairpin_queue_setup,
> > +				-ENOTSUP);
> > +	/* Use default specified by driver, if nb_rx_desc is zero */
> > +	if (nb_rx_desc == 0)
> > +		nb_rx_desc = cap.max_nb_desc;
> > +	if (nb_rx_desc > cap.max_nb_desc) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			       "Invalid value for nb_rx_desc(=%hu), should be: "
> > +			       "<= %hu", nb_rx_desc, cap.max_nb_desc);
> 
> Please, don't split format string
> 

O.K. will fix.

> > +		return -EINVAL;
> > +	}
> > +	if (conf->peer_n > cap.max_rx_2_tx) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			       "Invalid value for number of peers(=%hu), "
> > +			       "should be: <= %hu", conf->peer_n,
> 
> Please, don't split format string.
> Also make the message unique. Right now it is same for Rx and Tx.
>

O.K. will fix.
 
> > +			       cap.max_rx_2_tx);
> > +		return -EINVAL;
> > +	}
> > +	if (conf->peer_n == 0) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			       "Invalid value for number of peers(=%hu), "
> > +			       "should be: > 0", conf->peer_n);
> 
> Please, don't split format string
> Also make the message unique. Right now it is same for Rx and Tx.
> 

O.K. will fix.

> > +		return -EINVAL;
> > +	}
> > +	if (cap.max_n_queues != UINT16_MAX) {
> > +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +			if (dev->data->rx_queue_state[i] ==
> > +			    RTE_ETH_QUEUE_STATE_HAIRPIN)
> > +				count++;
> > +		}
> > +		if (count > cap.max_n_queues) {
> > +			RTE_ETHDEV_LOG(ERR,
> > +				       "To many Rx hairpin queues %d", count);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +	if (dev->data->dev_started)
> > +		return -EBUSY;
> > +	rxq = dev->data->rx_queues;
> > +	if (rxq[rx_queue_id] != NULL) {
> > +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >rx_queue_release,
> > +					-ENOTSUP);
> > +		(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
> > +		rxq[rx_queue_id] = NULL;
> > +	}
> > +	ret = (*dev->dev_ops->rx_hairpin_queue_setup)(dev, rx_queue_id,
> > +						      nb_rx_desc, conf);
> > +	if (ret == 0)
> > +		dev->data->rx_queue_state[rx_queue_id] =
> > +			RTE_ETH_QUEUE_STATE_HAIRPIN;
> > +	return eth_err(port_id, ret);
> > +}
> > +
> > +int
> >   rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
> >   		       uint16_t nb_tx_desc, unsigned int socket_id,
> >   		       const struct rte_eth_txconf *tx_conf)
> > @@ -1856,6 +1963,80 @@ struct rte_eth_dev *
> >   		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));
> >   }
> >
> > +int
> > +rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
> > +			       uint16_t nb_tx_desc,
> > +			       const struct rte_eth_hairpin_conf *conf)
> > +{
> > +	struct rte_eth_dev *dev;
> > +	struct rte_eth_hairpin_cap cap;
> > +	void **txq;
> > +	int i;
> > +	int count = 0;
> > +	int ret;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > +	dev = &rte_eth_devices[port_id];
> > +	if (tx_queue_id >= dev->data->nb_tx_queues) {
> > +		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> tx_queue_id);
> > +		return -EINVAL;
> > +	}
> > +	ret = rte_eth_dev_hairpin_capability_get(port_id, &cap);
> > +	if (ret != 0)
> > +		return ret;
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >tx_hairpin_queue_setup,
> > +				-ENOTSUP);
> > +	/* Use default specified by driver, if nb_tx_desc is zero */
> > +	if (nb_tx_desc == 0)
> > +		nb_tx_desc = cap.max_nb_desc;
> > +	if (nb_tx_desc > cap.max_nb_desc) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			       "Invalid value for nb_tx_desc(=%hu), should be: "
> > +			       "<= %hu", nb_tx_desc, cap.max_nb_desc);
> 
> Please, don't split format string
>

 
O.K. will fix.

> > +		return -EINVAL;
> > +	}
> > +	if (conf->peer_n > cap.max_tx_2_rx) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			       "Invalid value for number of peers(=%hu), "
> > +			       "should be: <= %hu", conf->peer_n,
> 
> Please, don't split format string
> 

O.K. will fix.

> > +			       cap.max_tx_2_rx);
> > +		return -EINVAL;
> > +	}
> > +	if (conf->peer_n == 0) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			       "Invalid value for number of peers(=%hu), "
> > +			       "should be: > 0", conf->peer_n);
> 
> Please, don't split format string
> 

O.K. will fix.

> > +		return -EINVAL;
> > +	}
> > +	if (cap.max_n_queues != UINT16_MAX) {
> > +		for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +			if (dev->data->tx_queue_state[i] ==
> > +			    RTE_ETH_QUEUE_STATE_HAIRPIN)
> > +				count++;
> > +		}
> > +		if (count > cap.max_n_queues) {
> > +			RTE_ETHDEV_LOG(ERR,
> > +				       "To many Rx hairpin queues %d", count);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +	if (dev->data->dev_started)
> > +		return -EBUSY;
> > +	txq = dev->data->tx_queues;
> > +	if (txq[tx_queue_id] != NULL) {
> > +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >tx_queue_release,
> > +					-ENOTSUP);
> > +		(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
> > +		txq[tx_queue_id] = NULL;
> > +	}
> > +	ret = (*dev->dev_ops->tx_hairpin_queue_setup)
> > +		(dev, tx_queue_id, nb_tx_desc, conf);
> > +	if (ret == 0)
> > +		dev->data->tx_queue_state[tx_queue_id] =
> > +			RTE_ETH_QUEUE_STATE_HAIRPIN;
> > +	return eth_err(port_id, ret);
> > +}
> > +
> >   void
> >   rte_eth_tx_buffer_drop_callback(struct rte_mbuf **pkts, uint16_t unsent,
> >   		void *userdata __rte_unused)
> > @@ -3981,12 +4162,20 @@ int rte_eth_set_queue_rate_limit(uint16_t
> port_id, uint16_t queue_idx,
> >   	rte_errno = ENOTSUP;
> >   	return NULL;
> >   #endif
> > +	struct rte_eth_dev *dev;
> > +
> >   	/* check input parameters */
> >   	if (!rte_eth_dev_is_valid_port(port_id) || fn == NULL ||
> >   		    queue_id >= rte_eth_devices[port_id].data->nb_rx_queues) {
> >   		rte_errno = EINVAL;
> >   		return NULL;
> >   	}
> > +	dev = &rte_eth_devices[port_id];
> > +	if (dev->data->rx_queue_state[queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		rte_errno = EINVAL;
> > +		return NULL;
> > +	}
> >   	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> >
> >   	if (cb == NULL) {
> > @@ -4058,6 +4247,8 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id,
> uint16_t queue_idx,
> >   	rte_errno = ENOTSUP;
> >   	return NULL;
> >   #endif
> > +	struct rte_eth_dev *dev;
> > +
> >   	/* check input parameters */
> >   	if (!rte_eth_dev_is_valid_port(port_id) || fn == NULL ||
> >   		    queue_id >= rte_eth_devices[port_id].data->nb_tx_queues) {
> > @@ -4065,6 +4256,13 @@ int rte_eth_set_queue_rate_limit(uint16_t
> port_id, uint16_t queue_idx,
> >   		return NULL;
> >   	}
> >
> > +	dev = &rte_eth_devices[port_id];
> > +	if (dev->data->tx_queue_state[queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		rte_errno = EINVAL;
> > +		return NULL;
> > +	}
> > +
> >   	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> >
> >   	if (cb == NULL) {
> > @@ -4180,6 +4378,14 @@ int rte_eth_set_queue_rate_limit(uint16_t
> port_id, uint16_t queue_idx,
> >
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rxq_info_get, -
> ENOTSUP);
> >
> > +	if (dev->data->rx_queue_state[queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		RTE_ETHDEV_LOG(INFO,
> > +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is
> hairpin queue\n",
> 
> I think it would be useful if log mentions what caller tried to do.
> See note about log messages uniqueness, e.g.
> "Cannot get RxQ info: port %"PRIu16" queue %"PRIu16" is hairpin"
> Also I think it is better to check it before rxq_info_get check
> mainly to put it nearby queue range check to group all
> queue ID checks together.
> 

O.K. will fix.

> > +			queue_id, port_id);
> > +		return -EINVAL;
> > +	}
> > +
> >   	memset(qinfo, 0, sizeof(*qinfo));
> >   	dev->dev_ops->rxq_info_get(dev, queue_id, qinfo);
> >   	return 0;
> > @@ -4202,6 +4408,14 @@ int rte_eth_set_queue_rate_limit(uint16_t
> port_id, uint16_t queue_idx,
> >   		return -EINVAL;
> >   	}
> >
> > +	if (dev->data->tx_queue_state[queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		RTE_ETHDEV_LOG(INFO,
> > +			"Queue %"PRIu16" of device with port_id=%"PRIu16" is
> hairpin queue\n",
> 
> Same as above.
> 

O.K. will fix.

> > +			queue_id, port_id);
> > +		return -EINVAL;
> > +	}
> > +
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->txq_info_get, -
> ENOTSUP);
> >
> >   	memset(qinfo, 0, sizeof(*qinfo));
> > @@ -4510,6 +4724,21 @@ int rte_eth_set_queue_rate_limit(uint16_t
> port_id, uint16_t queue_idx,
> >   }
> >
> >   int
> > +rte_eth_dev_hairpin_capability_get(uint16_t port_id,
> > +				   struct rte_eth_hairpin_cap *cap)
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > +
> > +	dev = &rte_eth_devices[port_id];
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hairpin_cap_get,
> > +				-ENOTSUP);
> > +	memset(cap, 0, sizeof(*cap));
> > +	return eth_err(port_id, (*dev->dev_ops->hairpin_cap_get)(dev, cap));
> > +}
> > +
> > +int
> >   rte_eth_dev_pool_ops_supported(uint16_t port_id, const char *pool)
> >   {
> >   	struct rte_eth_dev *dev;
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index 187a2bb..276f55f 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -804,6 +804,46 @@ struct rte_eth_txconf {
> >   };
> >
> >   /**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to return the hairpin capabilities that are supported.
> > + */
> > +struct rte_eth_hairpin_cap {
> > +	uint16_t max_n_queues;
> > +	/**< The max number of hairpin queues (different bindings). */
> > +	uint16_t max_rx_2_tx;
> > +	/**< Max number of Rx queues to be connected to one Tx queue. */
> > +	uint16_t max_tx_2_rx;
> > +	/**< Max number of Tx queues to be connected to one Rx queue. */
> > +	uint16_t max_nb_desc; /**< The max num of descriptors. */
> > +};
> > +
> > +#define RTE_ETH_MAX_HAIRPIN_PEERS 32
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to hold hairpin peer data.
> > + */
> > +struct rte_eth_hairpin_peer {
> > +	uint16_t port; /**< Peer port. */
> > +	uint16_t queue; /**< Peer queue. */
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to configure hairpin binding.
> > + */
> > +struct rte_eth_hairpin_conf {
> > +	uint16_t peer_n; /**< The number of peers. */
> > +	struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
> > +};
> > +
> > +/**
> >    * A structure contains information about HW descriptor ring limitations.
> >    */
> >   struct rte_eth_desc_lim {
> > @@ -1765,6 +1805,37 @@ int rte_eth_rx_queue_setup(uint16_t port_id,
> uint16_t rx_queue_id,
> >   		struct rte_mempool *mb_pool);
> >
> >   /**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * Allocate and set up a hairpin receive queue for an Ethernet device.
> > + *
> > + * The function set up the selected queue to be used in hairpin.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param rx_queue_id
> > + *   The index of the receive queue to set up.
> > + *   The value must be in the range [0, nb_rx_queue - 1] previously supplied
> > + *   to rte_eth_dev_configure().
> > + * @param nb_rx_desc
> > + *   The number of receive descriptors to allocate for the receive ring.
> > + *   0 means the PMD will use default value.
> > + * @param conf
> > + *   The pointer to the hairpin configuration.
> > + *
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + *   - (-EINVAL) if bad parameter.
> > + *   - (-ENOMEM) if unable to allocate the resources.
> > + */
> > +__rte_experimental
> > +int rte_eth_rx_hairpin_queue_setup
> > +	(uint16_t port_id, uint16_t rx_queue_id, uint16_t nb_rx_desc,
> > +	 const struct rte_eth_hairpin_conf *conf);
> > +
> > +/**
> >    * Allocate and set up a transmit queue for an Ethernet device.
> >    *
> >    * @param port_id
> > @@ -1817,6 +1888,35 @@ int rte_eth_tx_queue_setup(uint16_t port_id,
> uint16_t tx_queue_id,
> >   		const struct rte_eth_txconf *tx_conf);
> >
> >   /**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * Allocate and set up a transmit hairpin queue for an Ethernet device.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param tx_queue_id
> > + *   The index of the transmit queue to set up.
> > + *   The value must be in the range [0, nb_tx_queue - 1] previously supplied
> > + *   to rte_eth_dev_configure().
> > + * @param nb_tx_desc
> > + *   The number of transmit descriptors to allocate for the transmit ring.
> > + *   0 to set default PMD value.
> > + * @param conf
> > + *   The hairpin configuration.
> > + *
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + *   - (-EINVAL) if bad parameter.
> > + *   - (-ENOMEM) if unable to allocate the resources.
> > + */
> > +__rte_experimental
> > +int rte_eth_tx_hairpin_queue_setup
> > +	(uint16_t port_id, uint16_t tx_queue_id, uint16_t nb_tx_desc,
> > +	 const struct rte_eth_hairpin_conf *conf);
> > +
> > +/**
> >    * Return the NUMA socket to which an Ethernet device is connected
> >    *
> >    * @param port_id
> > @@ -1851,7 +1951,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id,
> uint16_t tx_queue_id,
> >    *   to rte_eth_dev_configure().
> >    * @return
> >    *   - 0: Success, the receive queue is started.
> > - *   - -EINVAL: The port_id or the queue_id out of range.
> > + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
> >    *   - -EIO: if device is removed.
> >    *   - -ENOTSUP: The function not supported in PMD driver.
> >    */
> > @@ -1868,7 +1968,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id,
> uint16_t tx_queue_id,
> >    *   to rte_eth_dev_configure().
> >    * @return
> >    *   - 0: Success, the receive queue is stopped.
> > - *   - -EINVAL: The port_id or the queue_id out of range.
> > + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
> >    *   - -EIO: if device is removed.
> >    *   - -ENOTSUP: The function not supported in PMD driver.
> >    */
> > @@ -1886,7 +1986,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id,
> uint16_t tx_queue_id,
> >    *   to rte_eth_dev_configure().
> >    * @return
> >    *   - 0: Success, the transmit queue is started.
> > - *   - -EINVAL: The port_id or the queue_id out of range.
> > + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
> >    *   - -EIO: if device is removed.
> >    *   - -ENOTSUP: The function not supported in PMD driver.
> >    */
> > @@ -1903,7 +2003,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id,
> uint16_t tx_queue_id,
> >    *   to rte_eth_dev_configure().
> >    * @return
> >    *   - 0: Success, the transmit queue is stopped.
> > - *   - -EINVAL: The port_id or the queue_id out of range.
> > + *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
> >    *   - -EIO: if device is removed.
> >    *   - -ENOTSUP: The function not supported in PMD driver.
> >    */
> > @@ -3569,7 +3669,8 @@ int rte_eth_remove_tx_callback(uint16_t port_id,
> uint16_t queue_id,
> >    * @return
> >    *   - 0: Success
> >    *   - -ENOTSUP: routine is not supported by the device PMD.
> > - *   - -EINVAL:  The port_id or the queue_id is out of range.
> > + *   - -EINVAL:  The port_id or the queue_id is out of range, or the queue
> > + *               is hairpin queue.
> >    */
> >   int rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >   	struct rte_eth_rxq_info *qinfo);
> > @@ -3589,7 +3690,8 @@ int rte_eth_rx_queue_info_get(uint16_t port_id,
> uint16_t queue_id,
> >    * @return
> >    *   - 0: Success
> >    *   - -ENOTSUP: routine is not supported by the device PMD.
> > - *   - -EINVAL:  The port_id or the queue_id is out of range.
> > + *   - -EINVAL:  The port_id or the queue_id is out of range, or the queue
> > + *               is hairpin queue.
> >    */
> >   int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >   	struct rte_eth_txq_info *qinfo);
> > @@ -4031,6 +4133,23 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t
> port_id,
> >   void *
> >   rte_eth_dev_get_sec_ctx(uint16_t port_id);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * Query the device hairpin capabilities.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param cap
> > + *   Pointer to a structure that will hold the hairpin capabilities.
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + */
> > +__rte_experimental
> > +int rte_eth_dev_hairpin_capability_get(uint16_t port_id,
> > +				       struct rte_eth_hairpin_cap *cap);
> >
> >   #include <rte_ethdev_core.h>
> >
> > @@ -4131,6 +4250,12 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t
> port_id,
> >   		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n",
> queue_id);
> >   		return 0;
> >   	}
> > +	if (dev->data->rx_queue_state[queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		RTE_ETHDEV_LOG(ERR, "RX queue_id=%u is hairpin queue\n",
> 
> I see but these log messages are very similar to above, but I still think
> it would be useful to mention context to make it clear in log, e.g.
> "Cannot Rx from hairpin queue%"PRIu16" at port %"PRIu16
> 

O.K. will fix.

> > +			       queue_id);
> > +		return 0;
> > +	}
> >   #endif
> >   	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> >   				     rx_pkts, nb_pkts);
> > @@ -4397,6 +4522,12 @@ static inline int
> rte_eth_tx_descriptor_status(uint16_t port_id,
> >   		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> queue_id);
> >   		return 0;
> >   	}
> > +	if (dev->data->tx_queue_state[queue_id] ==
> > +	    RTE_ETH_QUEUE_STATE_HAIRPIN) {
> > +		RTE_ETHDEV_LOG(ERR, "TX queue_id=%u is hairpin queue\n",
> > +			       queue_id);
> 
> I think it would be useful to mention context to make it clear in log, e.g.
> "Cannot Tx to hairpin queue%"PRIu16" at port %"PRIu16
> 

O.K. will fix.

> > +		return 0;
> > +	}
> >   #endif
> >
> >   #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> 
> [snip]
  

17/10/2019 17:32, Ori Kam:
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * A structure used to return the hairpin capabilities that are supported.
> + */
> +struct rte_eth_hairpin_cap {
> +	uint16_t max_n_queues;
> +	/**< The max number of hairpin queues (different bindings). */
> +	uint16_t max_rx_2_tx;
> +	/**< Max number of Rx queues to be connected to one Tx queue. */
> +	uint16_t max_tx_2_rx;
> +	/**< Max number of Tx queues to be connected to one Rx queue. */
> +	uint16_t max_nb_desc; /**< The max num of descriptors. */
> +};

I think you can switch to "comment-first style" for this struct.


> +#define RTE_ETH_MAX_HAIRPIN_PEERS 32

Usually I think such define is in the build config.
Any other opinion?


> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * A structure used to hold hairpin peer data.
> + */
> +struct rte_eth_hairpin_peer {
> +	uint16_t port; /**< Peer port. */
> +	uint16_t queue; /**< Peer queue. */
> +};

It may be the right place to give more words about what is a peer,
can we have multiple peers, etc.


> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * A structure used to configure hairpin binding.
> + */
> +struct rte_eth_hairpin_conf {
> +	uint16_t peer_n; /**< The number of peers. */

In general, I don't like one-letter abbreviations.
Is peer_count better?

> +	struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
> +};
  

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 23, 2019 10:05 AM
> To: Ori Kam <orika@mellanox.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; jingjing.wu@intel.com;
> stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH v4 02/15] ethdev: add support for hairpin queue
> 
> 17/10/2019 17:32, Ori Kam:
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> >  /**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to return the hairpin capabilities that are supported.
> > + */
> > +struct rte_eth_hairpin_cap {
> > +	uint16_t max_n_queues;
> > +	/**< The max number of hairpin queues (different bindings). */
> > +	uint16_t max_rx_2_tx;
> > +	/**< Max number of Rx queues to be connected to one Tx queue. */
> > +	uint16_t max_tx_2_rx;
> > +	/**< Max number of Tx queues to be connected to one Rx queue. */
> > +	uint16_t max_nb_desc; /**< The max num of descriptors. */
> > +};
> 
> I think you can switch to "comment-first style" for this struct.
> 

O.K I will change.

> 
> > +#define RTE_ETH_MAX_HAIRPIN_PEERS 32
> 
> Usually I think such define is in the build config.
> Any other opinion?
> 

I need to check. But if you don't mind let's keep it this way, and modify it 
later after we see how other manufactures will add hairpin.

> 
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to hold hairpin peer data.
> > + */
> > +struct rte_eth_hairpin_peer {
> > +	uint16_t port; /**< Peer port. */
> > +	uint16_t queue; /**< Peer queue. */
> > +};
> 
> It may be the right place to give more words about what is a peer,
> can we have multiple peers, etc.
> 


I'm not sure what I can say to make it clearer but I will try.

> 
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to configure hairpin binding.
> > + */
> > +struct rte_eth_hairpin_conf {
> > +	uint16_t peer_n; /**< The number of peers. */
> 
> In general, I don't like one-letter abbreviations.
> Is peer_count better?
> 

O.K. I will change to count.

> > +	struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
> > +};
> 
>
  

On Wed, Oct 23, 2019 at 10:09:45AM +0000, Ori Kam wrote:
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Wednesday, October 23, 2019 10:05 AM
> > To: Ori Kam <orika@mellanox.com>
> > Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; jingjing.wu@intel.com;
> > stephen@networkplumber.org
> > Subject: Re: [dpdk-dev] [PATCH v4 02/15] ethdev: add support for hairpin queue
> > 
> > 17/10/2019 17:32, Ori Kam:
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > >  /**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > > + *
> > > + * A structure used to return the hairpin capabilities that are supported.
> > > + */
> > > +struct rte_eth_hairpin_cap {
> > > +	uint16_t max_n_queues;
> > > +	/**< The max number of hairpin queues (different bindings). */
> > > +	uint16_t max_rx_2_tx;
> > > +	/**< Max number of Rx queues to be connected to one Tx queue. */
> > > +	uint16_t max_tx_2_rx;
> > > +	/**< Max number of Tx queues to be connected to one Rx queue. */
> > > +	uint16_t max_nb_desc; /**< The max num of descriptors. */
> > > +};
> > 
> > I think you can switch to "comment-first style" for this struct.
> > 
> 
> O.K I will change.
> 
> > 
> > > +#define RTE_ETH_MAX_HAIRPIN_PEERS 32
> > 
> > Usually I think such define is in the build config.
> > Any other opinion?
> > 
+1 for not moving it to the build config unless absolutely necessary.