[dpdk-dev] [PATCH v2 02/20] net/mlx5: handle drop queues are regular queues

[Nélio Laranjeiro]

On Mon, Jul 02, 2018 at 06:07:03PM -0700, Yongseok Koh wrote:
> On Wed, Jun 27, 2018 at 05:07:34PM +0200, Nelio Laranjeiro wrote:
> > Drop queues are essentially used in flows due to Verbs API, the
> > information if the fate of the flow is a drop or not is already present
> > in the flow.  Due to this, drop queues can be fully mapped on regular
> > queues.
> 
> The title doesn't look good. Maybe typo?
> 	net/mlx5: handle drop queues are regular queues
> 'are' -> 'as'?
[...]

Right,

> >  struct priv {
> >  	LIST_ENTRY(priv) mem_event_cb; /* Called by memory event callback. */
> >  	struct rte_eth_dev_data *dev_data;  /* Pointer to device data. */
> > @@ -175,7 +178,7 @@ struct priv {
> >  	struct rte_intr_handle intr_handle; /* Interrupt handler. */
> >  	unsigned int (*reta_idx)[]; /* RETA index table. */
> >  	unsigned int reta_idx_n; /* RETA index size. */
> > -	struct mlx5_hrxq_drop *flow_drop_queue; /* Flow drop queue. */
> > +	struct mlx5_drop drop; /* Flow drop queues. */
> 
> priv->drop sounds strange. Why did you change the name?
> How about priv->flow_drop if you didn't like the full name?
>[...]

flow_drop_queue is also confusing as it is a drop hash rx queue, it can
be used without a flow as a regular queue.
Renaming it to drop_hrxq.

> > +/**
> > + * Release a drop Rx queue Verbs object.
> > + *
> > + * @param dev
> > + *   Pointer to Ethernet device.
> > + * @param rxq
> > + *   Pointer to the drop Verbs Rx queue.
> > + *
> > + * @return
> > + *   The Verbs object initialised, NULL otherwise and rte_errno is set.
> > + */
> > +void
> > +mlx5_rxq_ibv_drop_release(struct rte_eth_dev *dev, struct mlx5_rxq_ibv *rxq)
> 
> If rxq for drop is saved in priv->drop.rxq, then why does it have to get rxq
> pointer as an argument? Looks redundant.
>[...]

Like for all hrxqs, indirection tables, rxqs,  which are stored in priv
inside a list or an array.  Priv is used as a storage place which is
only access through *_{new,get,release} functions.
This is also to keep a consistency between regular hrxqs, and drop hrxq also.

> > +void
> > +mlx5_ind_table_ibv_drop_release(struct rte_eth_dev *dev,
> > +				struct mlx5_ind_table_ibv *ind_tbl)
> 
> ind_tbl is a redundant argument. Can be referenced by
> priv->drop.hrxq->ind_table.
>[...]

Ditto.

> > +/**
> > + * Release a drop hash Rx queue.
> > + *
> > + * @param dev
> > + *   Pointer to Ethernet device.
> > + * @param hrxq
> > + *   Pointer to Hash Rx queue to release.
> > + */
> > +void
> > +mlx5_hrxq_drop_release(struct rte_eth_dev *dev, struct mlx5_hrxq *hrxq)
>[...]
> 
> hrxq is a redundant argument. Can be referenced by priv->drop.hrxq.
>[...]
 
Ditto.

Thanks,

-- 
Nélio Laranjeiro
6WIND