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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/24/19 11:29 AM, Ori Kam wrote:
> Hi Andrew,
>
> When writing the new function I thought about using bool, but
> I decided against it for the following reasons:
> 1. There is no use of bool any where in the code, and there is not special reason to add it now.

rte_ethdev.c includes stdbool.h and uses bool

> 2. Other functions of this kind already returns int. for example (rte_eth_dev_is_valid_port / rte_eth_is_valid_owner_id)
>
> Thanks,
> Ori
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Thursday, October 24, 2019 10:55 AM
>> 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: [dpdk-dev] [PATCH v5 02/15] ethdev: add support for hairpin queue
>>
>> 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]