From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH v2 15/30] net/mlx5: add Hash Rx queue object Date: Fri, 6 Oct 2017 15:50:06 -0700 Message-ID: <20171006225005.GA20117@yongseok-MBP.local> References: <20171006045956.GF19330@yongseok-MBP.local> <20171006070325.GI15330@autoinstall.dev.6wind.com> 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: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Return-path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0069.outbound.protection.outlook.com [104.47.1.69]) by dpdk.org (Postfix) with ESMTP id 29A141AEE9 for ; Sat, 7 Oct 2017 00:50:22 +0200 (CEST) Content-Disposition: inline In-Reply-To: <20171006070325.GI15330@autoinstall.dev.6wind.com> 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 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