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

Nélio Laranjeiro nelio.laranjeiro at 6wind.com
Mon Oct 9 10:05:21 CEST 2017


On Fri, Oct 06, 2017 at 03:50:06PM -0700, Yongseok Koh wrote:
> 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?

It make sense, but in this situation, the whole patches needs to be
modified to follow this design, the current one being, it needs an
object it gets a reference,  it does not need it anymore, it release the
reference.  Which mean a get() in a high level object causes a get() on
underlying ones.  A release on high level objects causes a release() on
underlying ones.  In this case, a flow will handle a reference on all
objects which contains a reference counter and used by it, even the
hidden ones.

Currently it won't hurt as it is a control plane point which already
rely on a lot of system calls.

Can we agree on letting the design as is for this release and maybe
changing it in the next one?

Thanks,

-- 
Nélio Laranjeiro
6WIND


More information about the dev mailing list