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

[Yongseok Koh]

On Tue, Jul 03, 2018 at 09:17:56AM +0200, Nélio Laranjeiro wrote:
> 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:
[...]
> 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.

Not so much critical to me but the entry has a field having repeating name.
	priv->drop_hrxq.hrxq and priv->drop_hrxq.rxq
Sounds still confusing...

> > > +/**
> > > + * 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.

However, the assumption is there's only one drop queue while the regular ones
have multiple instances.

> Priv is used as a storage place which is only access through
> *_{new,get,release} functions.

Yes, that's what I'm telling you. *_{new,get,release}() accesses priv, then why
the pointer (which is saved in priv) is needed as an argument?

> This is also to keep a consistency between regular hrxqs, and drop hrxq also.
Not sure why that consistency has to be kept.

int mlx5_rxq_release(struct rte_eth_dev *dev, uint16_t idx);
int mlx5_hrxq_release(struct rte_eth_dev *dev, struct mlx5_hrxq *hxrq);

mlx5_rxq_release() takes index as the instances are stored in an array and
mlx5_hrxq_release() takes pointer as the instances are stored in a list.

Then, what if there's only one instance and no need to search?
Not taking such an argument sounds more consistent...

Thanks,
Yongseok

> 
> > > +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