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
