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

[Ori Kam <orika@mellanox.com>]

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.
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]