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

[Nélio Laranjeiro]

On Tue, Jul 03, 2018 at 10:05:05AM -0700, Yongseok Koh wrote:
> 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...
>[...]

Even if I prefer to keep consistency about identical functionality (in
this case creating an hash Rx queue), I'll make the changes to please
you.

Regards,

-- 
Nélio Laranjeiro
6WIND