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

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi Ori,

thanks for review notes applied. Please, see below.

On 10/23/19 4:37 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>

Just a bit below
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

[snip]

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 78da293..199e96e 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) == 1) {

I think the function should return bool and it results should be
used as is without == 1 or something like this.
Everyrwhere in the patch.

[snip]

> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index c404f17..98023d7 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -26,6 +26,50 @@
>    */
>   #define RTE_ETH_QUEUE_STATE_STOPPED 0
>   #define RTE_ETH_QUEUE_STATE_STARTED 1
> +#define RTE_ETH_QUEUE_STATE_HAIRPIN 2
> +
> +/**
> + * @internal
> + * Check if the selected Rx queue is hairpin queue.
> + *
> + * @param dev
> + *  Pointer to the selected device.
> + * @param queue_id
> + *  The selected queue.
> + *
> + * @return
> + *   - (1) if the queue is hairpin queue, 0 otherwise.
> + */
> +static inline int

I think the function should return bool (and stdbool.h should be included).
Return value description should be updated.

> +rte_eth_dev_is_rx_hairpin_queue(struct rte_eth_dev *dev, uint16_t queue_id)
> +{
> +	if (dev->data->rx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN)
> +		return 1;
> +	return 0;
> +}
> +
> +
> +/**
> + * @internal
> + * Check if the selected Tx queue is hairpin queue.
> + *
> + * @param dev
> + *  Pointer to the selected device.
> + * @param queue_id
> + *  The selected queue.
> + *
> + * @return
> + *   - (1) if the queue is hairpin queue, 0 otherwise.
> + */
> +static inline int

Same here.

> +rte_eth_dev_is_tx_hairpin_queue(struct rte_eth_dev *dev, uint16_t queue_id)
> +{
> +	if (dev->data->tx_queue_state[queue_id] ==
> +	    RTE_ETH_QUEUE_STATE_HAIRPIN)
> +		return 1;
> +	return 0;
> +}
>   
>   /**
>    * @internal

[snip]