Re: [dpdk-dev] [PATCH v6 02/14] ethdev: add support for hairpin queue

[Ori Kam <orika@mellanox.com>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Tuesday, October 29, 2019 9:39 AM
> To: Ori Kam <orika@mellanox.com>; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.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: [dpdk-dev] [PATCH v6 02/14] ethdev: add support for hairpin queue
> 
> On 10/28/19 9:44 PM, Ori Kam wrote:
> > Hi Andrew,
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >> Sent: Monday, October 28, 2019 5:16 PM
> >> To: Ori Kam <orika@mellanox.com>; John McNamara
> >> <john.mcnamara@intel.com>; Marko Kovacevic
> >> <marko.kovacevic@intel.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: [dpdk-dev] [PATCH v6 02/14] ethdev: add support for hairpin
> queue
> >>
> >> Hi Ori,
> >>
> >> On 10/27/19 3:24 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>
> >>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >> LGTM, nothing critical may be except maximum number check
> >> which I lost from my view before.
> >> Plus few style suggestions which may be dropped, but I'd be
> >> happier if applied.
> >>
> >> Thanks.
> >>
> > I really apricate your time and comments,
> > This patch is the base of a number of other series (Meta/Metering)
> > So if it is nothing critical I prefer to get this set merged and then change what
> is needed,
> > if it is O.K by you.
> 
> OK for me
> 

Thanks, I will send a new patch as soon as this get merged.

> > Detail  comments please see below.
> >
> >> [snip]
> >>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >>> index 7743205..68aca1f 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>> @@ -923,6 +923,13 @@ struct rte_eth_dev *
> >>>
> >>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -
> >> ENOTSUP);
> >>> +	if (rte_eth_dev_is_rx_hairpin_queue(dev, rx_queue_id)) {
> >>> +		RTE_ETHDEV_LOG(INFO,
> >>> +			"Can't start Rx queue %"PRIu16" of device with
> >> port_id=%"PRIu16" is hairpin queue\n",
> >>
> >> Log message looks a bit strange:
> >>       Can't start Rx queue 5 of device with port_id=0 is hairpin queue
> >> may be to put key information first:
> >>       Can't start hairpin Rx queue 5 of device with port_id=0
> >>
> > I'm not a native English speaker but I think the meaning is different.
> 
> Obviously me too
> 
> > In my original log it means that you try to start a queue but fail due to
> > the fact that the queue is hairpin queue.
> >
> > In your version it means that you can't start an hairpin queue but there is no
> > reason why not.
> >
> > What do you think?
> 
> Let's keep your version if there is no better suggestions from native
> speakers.
> 

Thanks,

> >>> +			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",
> >>> @@ -950,6 +957,13 @@ struct rte_eth_dev *
> >>>
> >>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -
> >> ENOTSUP);
> >>> +	if (rte_eth_dev_is_rx_hairpin_queue(dev, rx_queue_id)) {
> >>> +		RTE_ETHDEV_LOG(INFO,
> >>> +			"Can't stop Rx queue %"PRIu16" of device with
> >> port_id=%"PRIu16" is hairpin queue\n",
> >>
> >> Same
> >>
> > Please see comment above.
> >
> >>> +			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",
> >>> @@ -983,6 +997,13 @@ struct rte_eth_dev *
> >>>
> >>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -
> >> ENOTSUP);
> >>> +	if (rte_eth_dev_is_tx_hairpin_queue(dev, tx_queue_id)) {
> >>> +		RTE_ETHDEV_LOG(INFO,
> >>> +			"Can't start Tx queue %"PRIu16" of device with
> >> port_id=%"PRIu16" is hairpin queue\n",
> >>
> >> Same
> >>
> > Please see comment above.
> >
> >>> +			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",
> >>> @@ -1008,6 +1029,13 @@ struct rte_eth_dev *
> >>>
> >>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -
> >> ENOTSUP);
> >>> +	if (rte_eth_dev_is_tx_hairpin_queue(dev, tx_queue_id)) {
> >>> +		RTE_ETHDEV_LOG(INFO,
> >>> +			"Can't stop Tx queue %"PRIu16" of device with
> >> port_id=%"PRIu16" is hairpin queue\n",
> >>
> >> Same
> >>
> > Please see comment above.
> >
> >>> +			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",
> >>> @@ -1780,6 +1808,79 @@ 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);
> >> Most likely unsupported hairpin is caught by capability get above.
> >> So, may be it is better to move the check just before usage far below.
> >> Also, if line length is sufficient I think it would better to put -ENOTSUP
> >> to the previous line just to follow port_id check style.
> >>
> > I think that in most function we are starting with the check.
> > personally I like to have basic checks in the beginning of the code.
> > But I will do what you think is best. If I remember correctly the line
> > length is to short, but I will test again.
> 
> Up to you. Thanks.
> 

I will keep my version, but will test again if I can merge it to one line.

> >>> +	/* Use default specified by driver, if nb_rx_desc is zero */
> >>> +	if (nb_rx_desc == 0)
> >>> +		nb_rx_desc = cap.max_nb_desc;
> >> Function description and comment above mentions PMD default, but
> >> there is no default. It uses just maximum. I have no strong opinion
> >> if default is really required or it is OK to say that maximum is used.
> >> The only concern is: why maximum?
> >>
> > Most likely the best value is the max, but I can add a new field to the cap
> > that say default value. What do you think?
> 
> I'm not 100% sure since default requires 0 value handling and
> I think fallback to maximum could be the right handling here.
> May be it is better to document that maximum is used and
> introduce default if it is really required in the future.
> It should be reconsidered when API is promoted to stable
> from experimental.
> 
> Basically both options are OK for me.
> 

I like your idea, I will change the documentation to say that the max will be used.

> >>> +	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);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +	if (conf->peer_count > cap.max_rx_2_tx) {
> >>> +		RTE_ETHDEV_LOG(ERR,
> >>> +			"Invalid value for number of peers for Rx queue(=%hu),
> >> should be: <= %hu",
> >>> +			conf->peer_count, cap.max_rx_2_tx);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +	if (conf->peer_count == 0) {
> >>> +		RTE_ETHDEV_LOG(ERR,
> >>> +			"Invalid value for number of peers for Rx queue(=%hu),
> >> should be: > 0",
> >>> +			conf->peer_count);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +	if (cap.max_nb_queues != UINT16_MAX) {
> >> I'm not sure that we need to handle it separately. Code below
> >> should handle it and it and there is no point to optimize it.
> >>
> > This is done to save time if the user set uint16_max there is no point to the
> > loop, I can add the check as condition to the loop but then it looks incorrect
> > since we are checking some think that can’t be changed.
> > What do you think?
> 
> Frankly speaking I see no value in the optimization. It is control
> path and I'd prefer simpler code here.
> 

O.K. will add the check in the for command.

> >>> +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> >> May I suggest to assign count = 0 to make it a bit easier to read and
> >> more robust against future changes.
> >>
> > You mean add count = 0 to the first part of the loop?
> 
> Yes, right now count initialization is done too far from the line.
> 

Will fix.

> >>> +			if (rte_eth_dev_is_rx_hairpin_queue(dev, i))
> >> The condition should be more tricky if we resetup hairpin queue.
> >> I.e. we should check if i is rx_queue_id and count it anyway.
> >>
> >>> +				count++;
> >>> +		}
> >>> +		if (count > cap.max_nb_queues) {
> >>> +			RTE_ETHDEV_LOG(ERR, "To many Rx hairpin queues
> >> %d",
> >>
> >> I think it would be useful to log max here as well to catch
> >> unset max cases easier.
> >>
> > I'm not sure I understand.
> 
> If the question is about logging, the answer is simple:
> if the user forget to initialize maximum number of hairpin queues
> properly, it will be zero and setup will fail here. So, it would be
> good to log maximum value here just to make it clear which
> limit is exceeded.
> 

Maybe I'm missing something but the PMD sets the max number of hairpin queues.
But in any case I agree we should log what the user requested and what is the max 
that the PMD reports.

> If the question is about above check, let's consider the case when
> maximum is one and one hairpin queue is already setup, but
> user tries to setup one more. Above loop will count only one since
> hairpin state for current queue is set below. So, the condition will
> allow to setup the second hairpin queue.
> In theory, we could initialize cound=1 to count this one, but
> it would break the case when we call setup once again for the
> queue which is already hairpin. API allows and handles it.
> 

Nice catch. I think the best solution is to compare the count to cap.max_nb_queues - 1.
and even before this comparison check if the current queue is already hairpin queue if so
we can skip this check.
What do you think?

> >>> +			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)
> >>> @@ -1878,6 +1979,78 @@ 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)
> >> Same notes as for Rx queue above.
> >>
> > O.K. same comments.
> >
> >>> +{
> >>> +	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);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +	if (conf->peer_count > cap.max_tx_2_rx) {
> >>> +		RTE_ETHDEV_LOG(ERR,
> >>> +			"Invalid value for number of peers for Tx queue(=%hu),
> >> should be: <= %hu",
> >>> +			conf->peer_count, cap.max_tx_2_rx);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +	if (conf->peer_count == 0) {
> >>> +		RTE_ETHDEV_LOG(ERR,
> >>> +			"Invalid value for number of peers for Tx queue(=%hu),
> >> should be: > 0",
> >>> +			conf->peer_count);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +	if (cap.max_nb_queues != UINT16_MAX) {
> >>> +		for (i = 0; i < dev->data->nb_tx_queues; i++) {
> >>> +			if (rte_eth_dev_is_tx_hairpin_queue(dev, i))
> >>> +				count++;
> >>> +		}
> >>> +		if (count > cap.max_nb_queues) {
> >>> +			RTE_ETHDEV_LOG(ERR,
> >>> +				       "To many Tx 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)
> >>> @@ -4007,12 +4180,19 @@ 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 (rte_eth_dev_is_rx_hairpin_queue(dev, queue_id)) {
> >>> +		rte_errno = EINVAL;
> >>> +		return NULL;
> >>> +	}
> >>>    	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> >>>
> >>>    	if (cb == NULL) {
> >>> @@ -4084,6 +4264,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) {
> >>> @@ -4091,6 +4273,12 @@ int rte_eth_set_queue_rate_limit(uint16_t
> >> port_id, uint16_t queue_idx,
> >>>    		return NULL;
> >>>    	}
> >>>
> >>> +	dev = &rte_eth_devices[port_id];
> >>> +	if (rte_eth_dev_is_tx_hairpin_queue(dev, queue_id)) {
> >>> +		rte_errno = EINVAL;
> >>> +		return NULL;
> >>> +	}
> >>> +
> >>>    	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> >>>
> >>>    	if (cb == NULL) {
> >>> @@ -4204,6 +4392,13 @@ int rte_eth_set_queue_rate_limit(uint16_t
> >> port_id, uint16_t queue_idx,
> >>>    		return -EINVAL;
> >>>    	}
> >>>
> >>> +	if (rte_eth_dev_is_rx_hairpin_queue(dev, queue_id)) {
> >>> +		RTE_ETHDEV_LOG(INFO,
> >>> +			"Can't get queue info for Rx queue %"PRIu16" of device
> >> with port_id=%"PRIu16" is hairpin queue\n",
> >>
> >> "queue" is repeated 3 times above ;) I'm afraid it is too much, may be:
> >> "Can't get hairpin Rx queue %" PRIu16 " port %" PRIu16 " info\n"
> >> or
> >> "Can't get hairpin Rx queue %" PRIu16 " info of device with port_id=%"
> >> PRIu16 "\n"
> >> Anyway up to you.
> >>
> > O.K.  I will update.
> >
> >>> +			queue_id, port_id);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rxq_info_get, -
> >> ENOTSUP);
> >>>    	memset(qinfo, 0, sizeof(*qinfo));
> >>> @@ -4228,6 +4423,13 @@ int rte_eth_set_queue_rate_limit(uint16_t
> >> port_id, uint16_t queue_idx,
> >>>    		return -EINVAL;
> >>>    	}
> >>>
> >>> +	if (rte_eth_dev_is_tx_hairpin_queue(dev, queue_id)) {
> >>> +		RTE_ETHDEV_LOG(INFO,
> >>> +			"Can't get queue info for Tx queue %"PRIu16" of device
> >> with port_id=%"PRIu16" is hairpin queue\n",
> >>
> >> Same
> >>
> > Same.
> >
> >>> +			queue_id, port_id);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->txq_info_get, -
> >> ENOTSUP);
> >>>    	memset(qinfo, 0, sizeof(*qinfo));
> >>> @@ -4600,6 +4802,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);
> >> Please, move -ENOTSUP to the previous line since line length is sufficient
> >> and make it similar to port_id check above.
> >>
> > Last time I check it didn't have room, I will check again.
> >
> >>> +	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 9e1f9ae..9b69255 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -839,6 +839,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 {
> >>> +	/** The max number of hairpin queues (different bindings). */
> >>> +	uint16_t max_nb_queues;
> >>> +	/**< Max number of Rx queues to be connected to one Tx queue. */
> >> Should be /**
> >>
> > Will fix.
> >
> >>> +	uint16_t max_rx_2_tx;
> >>> +	/**< Max number of Tx queues to be connected to one Rx queue. */
> >> Should be /**
> >>
> > Will fix.
> >
> >> [snip]
> >
> > Thanks,
> > Ori

Thanks,
Ori