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

Ori Kam orika at mellanox.com
Thu Oct 24 10:29:51 CEST 2019


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 at solarflare.com>
> Sent: Thursday, October 24, 2019 10:55 AM
> To: Ori Kam <orika at mellanox.com>; Thomas Monjalon
> <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>
> Cc: dev at dpdk.org; jingjing.wu at intel.com; stephen at 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 at mellanox.com>
> 
> Just a bit below
> Reviewed-by: Andrew Rybchenko <arybchenko at 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]



More information about the dev mailing list