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
