From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Subject: Re: [PATCH v2 15/30] net/mlx5: add Hash Rx queue object Date: Mon, 9 Oct 2017 10:05:21 +0200 Message-ID: <20171009080521.GB30412@autoinstall.dev.6wind.com> References: <20171006045956.GF19330@yongseok-MBP.local> <20171006070325.GI15330@autoinstall.dev.6wind.com> <20171006225005.GA20117@yongseok-MBP.local> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, adrien.mazarguil@6wind.com, ferruh.yigit@intel.com To: Yongseok Koh Return-path: Received: from mail-wm0-f45.google.com (mail-wm0-f45.google.com [74.125.82.45]) by dpdk.org (Postfix) with ESMTP id CF9C61AEF4 for ; Mon, 9 Oct 2017 10:05:32 +0200 (CEST) Received: by mail-wm0-f45.google.com with SMTP id m72so20065286wmc.1 for ; Mon, 09 Oct 2017 01:05:32 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20171006225005.GA20117@yongseok-MBP.local> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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