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
