[dpdk-dev] [PATCH v2 15/30] net/mlx5: add Hash Rx queue object

[Nélio Laranjeiro]

On Thu, Oct 05, 2017 at 09:59:58PM -0700, Yongseok Koh wrote:
> On Thu, Oct 05, 2017 at 02:49:47PM +0200, Nelio Laranjeiro wrote:
> [...]
> > +struct mlx5_hrxq*
> > +mlx5_priv_hrxq_get(struct priv *priv, uint8_t *rss_key, uint8_t rss_key_len,
> > +		   uint64_t hash_fields, uint16_t queues[], uint16_t queues_n)
> > +{
> > +	struct mlx5_hrxq *hrxq;
> > +
> > +	LIST_FOREACH(hrxq, &priv->hrxqs, next) {
> > +		struct mlx5_ind_table_ibv *ind_tbl;
> > +
> > +		if (hrxq->rss_key_len != rss_key_len)
> > +			continue;
> > +		if (memcmp(hrxq->rss_key, rss_key, rss_key_len))
> > +			continue;
> > +		if (hrxq->hash_fields != hash_fields)
> > +			continue;
> > +		ind_tbl = mlx5_priv_ind_table_ibv_get(priv, queues, queues_n);
> > +		if (!ind_tbl)
> > +			continue;
> > +		if (ind_tbl != hrxq->ind_table) {
> > +			mlx5_priv_ind_table_ibv_release(priv, ind_tbl);
> 
> As one hrxq can have only one ind_tbl, it looks unnecessary to increment refcnt
> of ind_tbl. As long as a hrxq exist, its ind_tbl can't be destroyed. So, it's
> safe. How about moving up this _release() outside of this if-clause and remove
> _release() in _hrxq_release()?

This is right, but in the other side, an indirection table can be used
by several hash rx queues, that is the main reason why they have their
own reference counter.


  +-------+  +-------+
  | Hrxq  |  | Hrxq  |
  | r = 1 |  | r = 1 |
  +-------+  +-------+
      |          |
      v          v
 +-------------------+
 | indirection table |
 | r = 2             |
 +-------------------+

Seems logical to make the Indirection table counter evolve the same way
as the hash rx queue, otherwise a second hash rx queue using this
indirection may release it whereas it is still in use by another hash rx
queue.

> However, it is logically flawless, so
> Acked-by: Yongseok Koh <yskoh at mellanox.com>

Thanks,

-- 
Nélio Laranjeiro
6WIND