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: Fri, 6 Oct 2017 09:03:25 +0200 Message-ID: <20171006070325.GI15330@autoinstall.dev.6wind.com> References: <20171006045956.GF19330@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-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id 483A71B23B for ; Fri, 6 Oct 2017 09:03:37 +0200 (CEST) Received: by mail-wm0-f51.google.com with SMTP id b189so5704676wmd.4 for ; Fri, 06 Oct 2017 00:03:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20171006045956.GF19330@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 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. > However, it is logically flawless, so > Acked-by: Yongseok Koh Thanks, -- Nélio Laranjeiro 6WIND