From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id 378C51B76E for ; Mon, 9 Apr 2018 16:42:40 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id r82so17413567wme.0 for ; Mon, 09 Apr 2018 07:42:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=Plryx13J3keetUStkBll16xjcDDs4JF7yjmJpoA2oo8=; b=JwHF31ZrfqJXqG06v2pRj6p5H52CUYWon/qCRsxfEZqZPQajEO57XuhY+TuQlyMar7 MmVfg/Vit/QE89vRts7kxWdLCzqAgJKH1BDYzz/G+kUBfKw7LAvkzbN7O+dM8LY6PV1l G99Xmw07Gbds9mGnKjnvP2i1HDY8syy1zDbM6i9OAY2O1wxX2fHvq7gLOd8dQc7VN7Vc 2lmkXbj9Km/XVkMhXxrH905Fho1kObc3lL7F0pwnaYSRFhdNGKpE1ZTzdy2fvKkUQMja lj/UslDUDZOjB07WTvkcQjSB37NZ2CvSc6G3dnIw9gU0JwL63jYnSrBnv6VrgoRFW8vr vYmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=Plryx13J3keetUStkBll16xjcDDs4JF7yjmJpoA2oo8=; b=kqtlLc+paVHWNSQF4ioCNXSGnHaNcVxF/8VW+Xi7Tuy6dN3ikh/wu5M15hq/wpyOxw /UtZ2uwcmeVSasnn93yFjP7pCjYRPSwRgHShhd6cOf9uZ6SUd56RZHqB3LvmXKAfyI6f 7B92WlcOXdTYAy8BOF3aKyGkXgk7+Ww69UKnQHAB5wXQPZSABGmb0U61Q6S4HduvNdT/ 1dzNdOqOZs78wkGCITajBFohpm+guRiQy4ZU81ZJEWuN70VDMWcDXN0F+hniRO5MTfWL D+wsVY8Rlb26ytKetQ+Fy/yb8w56SMoZ5FCgB9yp42U4+PU5nGv06mDYqLH0a4x45s1D Ee4w== X-Gm-Message-State: ALQs6tBEk0RWRtQNVejRk+lL6mBPj10Cy1dXIIlRhp73MdzUl9PLi9le wcBOJbDvnJH9f/NJOzJCYIrrgQ== X-Google-Smtp-Source: AIpwx4+MAUbPKCS3+rYbd1UHkmbyZ2tCn6yNn3+A0ui/PYBTLWbWSU1Ybfrbe0rBqxD3LXD89LDUxQ== X-Received: by 10.28.106.1 with SMTP id f1mr194298wmc.59.1523284959927; Mon, 09 Apr 2018 07:42:39 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id h190sm1164799wmd.22.2018.04.09.07.42.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Apr 2018 07:42:39 -0700 (PDT) Date: Mon, 9 Apr 2018 16:42:26 +0200 From: Adrien Mazarguil To: Andrew Rybchenko Cc: Thomas Monjalon , Ferruh Yigit , dev@dpdk.org, Xueming Li , Wenzhuo Lu , Jingjing Wu , Beilei Xing , Qi Zhang , Konstantin Ananyev , Nelio Laranjeiro , Yongseok Koh , Pascal Mazon , Radu Nicolau , Akhil Goyal , Ivan Malov Message-ID: <20180409144226.GZ4957@6wind.com> References: <20180404150312.12304-1-adrien.mazarguil@6wind.com> <20180406131736.19145-1-adrien.mazarguil@6wind.com> <20180406131736.19145-8-adrien.mazarguil@6wind.com> <314cca32-b9f9-afb9-b2e5-55d9175935cb@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <314cca32-b9f9-afb9-b2e5-55d9175935cb@solarflare.com> Subject: Re: [dpdk-dev] [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Apr 2018 14:42:40 -0000 On Sat, Apr 07, 2018 at 12:05:51PM +0300, Andrew Rybchenko wrote: > On 04/06/2018 04:25 PM, Adrien Mazarguil wrote: > > Since its inception, the rte_flow RSS action has been relying in part on > > external struct rte_eth_rss_conf for compatibility with the legacy RSS API. > > This structure lacks parameters such as the hash algorithm to use, and more > > recently, a method to tell which layer RSS should be performed on [1]. > > > > Given struct rte_eth_rss_conf will never be flexible enough to represent a > > complete RSS configuration (e.g. RETA table), this patch supersedes it by > > extending the rte_flow RSS action directly. > > > > A subsequent patch will add a field to use a non-default RSS hash > > algorithm. To that end, a field named "types" replaces the field formerly > > known as "rss_hf" and standing for "RSS hash functions" as it was > > confusing. Actual RSS hash function types are defined by enum > > rte_eth_hash_function. > > This patch updates all PMDs and example applications accordingly. > > > > It breaks ABI compatibility for the following public functions: > > > > - rte_flow_copy() > > - rte_flow_create() > > - rte_flow_query() > > - rte_flow_validate() > > > > [1] commit 676b605182a5 ("doc: announce ethdev API change for RSS > > configuration") > > > > Signed-off-by: Adrien Mazarguil > > Cc: Xueming Li > > Cc: Ferruh Yigit > > Cc: Thomas Monjalon > > Cc: Wenzhuo Lu > > Cc: Jingjing Wu > > Cc: Beilei Xing > > Cc: Qi Zhang > > Cc: Konstantin Ananyev > > Cc: Nelio Laranjeiro > > Cc: Yongseok Koh > > Cc: Andrew Rybchenko > > Cc: Pascal Mazon > > Cc: Radu Nicolau > > Cc: Akhil Goyal > > --- > > app/test-pmd/cmdline_flow.c | 59 +++++----- > > app/test-pmd/config.c | 39 +++---- > > doc/guides/prog_guide/rte_flow.rst | 22 ++-- > > drivers/net/e1000/e1000_ethdev.h | 13 ++- > > drivers/net/e1000/igb_ethdev.c | 4 +- > > drivers/net/e1000/igb_flow.c | 31 ++--- > > drivers/net/e1000/igb_rxtx.c | 51 +++++++-- > > drivers/net/i40e/i40e_ethdev.c | 53 +++++++-- > > drivers/net/i40e/i40e_ethdev.h | 15 ++- > > drivers/net/i40e/i40e_flow.c | 47 ++++---- > > drivers/net/ixgbe/ixgbe_ethdev.c | 4 +- > > drivers/net/ixgbe/ixgbe_ethdev.h | 13 ++- > > drivers/net/ixgbe/ixgbe_flow.c | 30 ++--- > > drivers/net/ixgbe/ixgbe_rxtx.c | 51 +++++++-- > > drivers/net/mlx4/mlx4.c | 2 +- > > drivers/net/mlx4/mlx4_flow.c | 61 +++++----- > > drivers/net/mlx4/mlx4_flow.h | 2 +- > > drivers/net/mlx4/mlx4_rxq.c | 2 +- > > drivers/net/mlx4/mlx4_rxtx.h | 2 +- > > drivers/net/mlx5/mlx5_flow.c | 193 +++++++++++++++----------------- > > drivers/net/mlx5/mlx5_rxq.c | 22 ++-- > > drivers/net/mlx5/mlx5_rxtx.h | 26 +++-- > > drivers/net/sfc/sfc_flow.c | 21 ++-- > > drivers/net/tap/tap_flow.c | 8 +- > > examples/ipsec-secgw/ipsec.c | 10 +- > > lib/librte_ether/rte_flow.c | 39 +++---- > > lib/librte_ether/rte_flow.h | 6 +- > > 27 files changed, 473 insertions(+), 353 deletions(-) > > <...> > > > diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c > > index 056405515..1a2c0299c 100644 > > --- a/drivers/net/sfc/sfc_flow.c > > +++ b/drivers/net/sfc/sfc_flow.c > > @@ -1234,13 +1234,11 @@ sfc_flow_parse_rss(struct sfc_adapter *sa, > > struct sfc_rxq *rxq; > > unsigned int rxq_hw_index_min; > > unsigned int rxq_hw_index_max; > > - const struct rte_eth_rss_conf *rss_conf = rss->rss_conf; > > - uint64_t rss_hf; > > - uint8_t *rss_key = NULL; > > + const uint8_t *rss_key; > > struct sfc_flow_rss *sfc_rss_conf = &flow->rss_conf; > > unsigned int i; > > - if (rss->num == 0) > > + if (rss->queue_num == 0) > > return -EINVAL; > > rxq_sw_index = sa->rxq_count - 1; > > @@ -1248,7 +1246,7 @@ sfc_flow_parse_rss(struct sfc_adapter *sa, > > rxq_hw_index_min = rxq->hw_index; > > rxq_hw_index_max = 0; > > - for (i = 0; i < rss->num; ++i) { > > + for (i = 0; i < rss->queue_num; ++i) { > > rxq_sw_index = rss->queue[i]; > > if (rxq_sw_index >= sa->rxq_count) > > @@ -1263,15 +1261,14 @@ sfc_flow_parse_rss(struct sfc_adapter *sa, > > rxq_hw_index_max = rxq->hw_index; > > } > > - rss_hf = (rss_conf != NULL) ? rss_conf->rss_hf : SFC_RSS_OFFLOADS; > > Here we had a fallback to default rss_hf (now types) if rss_conf is > unspecified. Thing is, rss_action->conf was never supposed to be NULL in the first place. Crashing on a NULL configuration has always been fine, but until recently prevented validation with testpmd's broken implementation. This problem was addressed in a prior series [1][2][3]. Since a value is now always provided, no need for a fallback. [1] "app/testpmd: fix lack of flow action configuration" http://dpdk.org/ml/archives/dev/2018-April/095280.html [2] "app/testpmd: fix RSS flow action configuration" http://dpdk.org/ml/archives/dev/2018-April/095281.html [3] "app/testpmd: fix missing RSS fields in flow action" http://dpdk.org/ml/archives/dev/2018-April/095282.html > > - if ((rss_hf & ~SFC_RSS_OFFLOADS) != 0) > > + if ((rss->types & ~SFC_RSS_OFFLOADS) != 0) > > return -EINVAL; > > - if (rss_conf != NULL) { > > - if (rss_conf->rss_key_len != sizeof(sa->rss_key)) > > + if (rss->key_len) { > > + if (rss->key_len != sizeof(sa->rss_key)) > > return -EINVAL; > > - rss_key = rss_conf->rss_key; > > + rss_key = rss->key; > > } else { > > rss_key = sa->rss_key; > > } > > @@ -1280,11 +1277,11 @@ sfc_flow_parse_rss(struct sfc_adapter *sa, > > sfc_rss_conf->rxq_hw_index_min = rxq_hw_index_min; > > sfc_rss_conf->rxq_hw_index_max = rxq_hw_index_max; > > - sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss_hf); > > + sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss->types); > > Now types go directly to mapping function and unspecified types (0) > will result in 0 rss_hash_types. Of course, it is a question how to treat > types==0. It is possible to say that it no RSS, but it does not make sense. > So, real options are device defaults (regardless configured on device level) > or device config (rx_adv.conf.rss_conf.rss_hf). I would prefer the later. > Please, document the intended behaviour in rte_flow.rst. Granted the existing documentation doesn't say much on that topic, but a 0 value for rss_hf does actually mean "no RSS" [4]: "The *rss_hf* field of the *rss_conf* structure indicates the different types of IPv4/IPv6 packets to which the RSS hashing must be applied. Supplying an *rss_hf* equal to zero disables the RSS feature." Now since this action doesn't use struct rte_eth_rss_conf anymore, we could define 0 as a PMD-specific behavior, which could be no RSS. It would make the API easier to use for applications that don't care about the RSS capabilities of each underlying adapter, 0 would just work everywhere as a safe default. [4] https://dpdk.org/doc/api/structrte__eth__rss__conf.html > If the later is chosen, above we'll have a bug since fallback to fixed > default. > Just use sa->rss_hash_types as fallback. Something like: > if (rss->types) >     sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss->types); > else >     sfc_rss_conf->rss_hash_types =sa->rss_hash_types; Looks like the previous code didn't provide a fallback when rss_hf was 0, only when rss_conf itself was NULL. So this is not a new issue introduced by this patch. I will update documentation to define 0 as described above for the convenience of application writers and leave the existing code in place. PMD maintainers will be free to enhance it as they wish later. Just remember testpmd now always provides a default value for it after querying the device [2]. > > rte_memcpy(sfc_rss_conf->rss_key, rss_key, sizeof(sa->rss_key)); > > for (i = 0; i < RTE_DIM(sfc_rss_conf->rss_tbl); ++i) { > > - unsigned int rxq_sw_index = rss->queue[i % rss->num]; > > + unsigned int rxq_sw_index = rss->queue[i % rss->queue_num]; > > struct sfc_rxq *rxq = sa->rxq_info[rxq_sw_index].rxq; > > sfc_rss_conf->rss_tbl[i] = rxq->hw_index - rxq_hw_index_min; > > <...> -- Adrien Mazarguil 6WIND