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

[Yongseok Koh]

On Fri, Oct 06, 2017 at 09:03:25AM +0200, Nélio Laranjeiro wrote:
> 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.

Whenever a hash Rx queue is created, it gets to have a ind_tbl either by
mlx5_priv_ind_table_ibv_get() or by mlx5_priv_ind_table_ibv_new(). So, the
refcnt of the ind_tbl is already increased. So, even if other hash RxQ which
have had the ind_tbl releases it, it is safe. That's why I don't think
ind_tbl->refcnt needs to get increased on calling mlx5_priv_hrxq_get(). Makes
sense?

Thanks,
Yongseok