From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id 540582BBF for ; Fri, 18 May 2018 11:49:15 +0200 (CEST) Received: by mail-wr0-f193.google.com with SMTP id p4-v6so8546405wrh.3 for ; Fri, 18 May 2018 02:49:15 -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:in-reply-to; bh=L16lpufTv1aU8cjz3XvQV2u8w5rNey/LPchHq/SetUI=; b=KzdIMSP5IrVTHs1IcMSiNw7NsV78zT1ASEvyDCgqnajN9m557vaYrU+AzreTMlEcCP ApIdXnez0FYwM5vQhnI2/mRyboTYZTGo8LtpNoNfJZB8WHYwivXbRO5It+BAX/wpa96E 0qiCRPT69yk2EeFA6PQHXuNwE68re8opV4uNdoOKmOmzVWAYGvePvwmxdeerNiKHifM+ VJkZzF7ZbkQE/ZEO5BQTb9w5VibJDfh9K1swWnR5406P/RBhgxg0seTFD0TMYMA4xEon KwxAgHlFhbi5os4+5dii1+XDJw+HfwUpjIPoUFUUlgralZaehPyl+Re9tfmIpi87/nk2 cQZA== 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:in-reply-to; bh=L16lpufTv1aU8cjz3XvQV2u8w5rNey/LPchHq/SetUI=; b=Sm0a2GoqQ+JoYPpu2842qvTe7j3xBJnjXsqPLHjyaPXD2ge8HO4ECMX5OALsgqecYm Hm3ZslIzcuRf3zBubjHq+fRXtiR+Liw47Hdd6IKZ4UmlbCHarR56jehd4zozaCnCRpyz Wscmp+n9Azh1Q6T7B2FNQA3OoQV/sRrP7UzHRxMUZLbHYyDOy36se13+T0+J8/19CDu0 IoMoKQKi3oIEIztF7L17mEEAzsNj/vrvf5htP+ZxWOddOVQcGBQk3qjSwQcULVmyrU7P 2jCROmSDEZgIG0Dw9NwKhvSMaTY6XRws4t+eSAhLM0BW9si2RxPdasgopPYexoWzUNU3 dg7Q== X-Gm-Message-State: ALKqPwdhts5bxoPr0BuoOQIfeP8spc3iXjT3GegGb/eYGOvl2MA4ob/T 10JUZ6KIjdEuGNM8OpQzEhUMmw== X-Google-Smtp-Source: AB8JxZoMPSq/7MnoYQxYGPTyv1IpiiXr67UMzErsVBPbZJS6QV6RmN5SMhVCrGHaK4U6v+/6/BYfYQ== X-Received: by 2002:adf:e447:: with SMTP id t7-v6mr7033554wrm.143.1526636955041; Fri, 18 May 2018 02:49:15 -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 b105-v6sm8736738wrd.64.2018.05.18.02.49.14 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 18 May 2018 02:49:14 -0700 (PDT) Date: Fri, 18 May 2018 11:49:00 +0200 From: Adrien Mazarguil To: Ophir Munk Cc: Shahaf Shuler , "dev@dpdk.org" Message-ID: <20180518094900.GM6497@6wind.com> References: <20180515154853.6361-1-adrien.mazarguil@6wind.com> <20180515154853.6361-2-adrien.mazarguil@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH 2/2] net/mlx4: refactor RSS conversion functions 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: Fri, 18 May 2018 09:49:15 -0000 Hi Ophir, On Thu, May 17, 2018 at 05:46:45PM +0000, Ophir Munk wrote: > Hi Adrien, > I like the idea of having one conversion function from dpdk to verbs and vice versa. > I have some comments inline regarding the implementation. > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Tuesday, May 15, 2018 6:51 PM > > To: Shahaf Shuler > > Cc: dev@dpdk.org; Ophir Munk > > Subject: [PATCH 2/2] net/mlx4: refactor RSS conversion functions > > > > Since commit 97b2217ae5bc ("net/mlx4: advertise supported RSS hash > > functions"), this PMD includes two similar-looking functions that convert RSS > > hash fields between Verbs and DPDK formats. > > > > This patch refactors them as a single two-way function and gets rid of > > redundant helper macros. > > > > Note the loss of the "static" keyword on the out[] (now verbs[]) array > > introduced by commit cbd737416c34 ("net/mlx4: avoid constant recreations > > in > > function") is what prevents the reliance on macro definitions for static > > initializers at the expense of a few extra instructions. An acceptable trade-off > > given this function is not involved in data plane operations. > > > > Signed-off-by: Adrien Mazarguil > > Cc: Ophir Munk > > --- > > drivers/net/mlx4/mlx4_ethdev.c | 3 +- > > drivers/net/mlx4/mlx4_flow.c | 147 ++++++++++-------------------------- > > drivers/net/mlx4/mlx4_flow.h | 4 +- > > 3 files changed, 44 insertions(+), 110 deletions(-) > > > > diff --git a/drivers/net/mlx4/mlx4_ethdev.c > > b/drivers/net/mlx4/mlx4_ethdev.c index 0a9c2e2f5..30deb3ef0 100644 > > --- a/drivers/net/mlx4/mlx4_ethdev.c > > +++ b/drivers/net/mlx4/mlx4_ethdev.c > > @@ -587,8 +587,7 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct > > rte_eth_dev_info *info) > > ETH_LINK_SPEED_20G | > > ETH_LINK_SPEED_40G | > > ETH_LINK_SPEED_56G; > > - info->flow_type_rss_offloads = > > - mlx4_ibv_to_rss_types(priv->hw_rss_sup); > > + info->flow_type_rss_offloads = mlx4_conv_rss_types(priv, 0, 1); > > Calling with parameters (priv, 0, 1) parameters in not as intuitive as calling with (priv, DEFAULT_RSS, DPDK_TO_VERBS) > Would you consider using definitions for the 0, 1 parameters to increase readability? Doing so would require either new macro or enum definitions, which I would like to avoid for such a minor internal function. Keep in mind the 0 value standing for "defaults" RSS types is both publicly documented [1] and documented again in mlx4_conv_rss_types(). As for DPDK_TO_VERBS, this is already the name of the parameter (int dpdk_to_verbs), currently a boolean value. I would have defined a type if there were more than two possible states. [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#action-rss > > > } > > > > /** > > diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c > > index ebc9eeb8b..90d206b55 100644 > > --- a/drivers/net/mlx4/mlx4_flow.c > > +++ b/drivers/net/mlx4/mlx4_flow.c > > @@ -42,41 +42,6 @@ > > #include "mlx4_rxtx.h" > > #include "mlx4_utils.h" > > > > -/** IBV supported RSS hash functions combinations */ -#define > > MLX4_IBV_IPV4_HF ( \ > > - IBV_RX_HASH_SRC_IPV4 | \ > > - IBV_RX_HASH_DST_IPV4) > > -#define MLX4_IBV_IPV6_HF ( \ > > - IBV_RX_HASH_SRC_IPV6 | \ > > - IBV_RX_HASH_DST_IPV6) > > -#define MLX4_IBV_TCP_HF ( \ > > - IBV_RX_HASH_SRC_PORT_TCP | \ > > - IBV_RX_HASH_DST_PORT_TCP) > > -#define MLX4_IBV_UDP_HF ( \ > > - IBV_RX_HASH_SRC_PORT_UDP | \ > > - IBV_RX_HASH_DST_PORT_UDP) > > - > > -/** Supported RSS hash functions combinations */ -#define > > MLX4_RSS_IPV4_HF ( \ > > - ETH_RSS_IPV4 | \ > > - ETH_RSS_FRAG_IPV4 | \ > > - ETH_RSS_NONFRAG_IPV4_OTHER) > > -#define MLX4_RSS_IPV6_HF ( \ > > - ETH_RSS_IPV6 | \ > > - ETH_RSS_FRAG_IPV6 | \ > > - ETH_RSS_NONFRAG_IPV6_OTHER | \ > > - ETH_RSS_IPV6_EX) > > -#define MLX4_RSS_IPV4_TCP_HF ( \ > > - ETH_RSS_NONFRAG_IPV4_TCP) > > -#define MLX4_RSS_IPV6_TCP_HF ( \ > > - ETH_RSS_NONFRAG_IPV6_TCP | \ > > - ETH_RSS_IPV6_TCP_EX) > > -#define MLX4_RSS_IPV4_UDP_HF ( \ > > - ETH_RSS_NONFRAG_IPV4_UDP) > > -#define MLX4_RSS_IPV6_UDP_HF ( \ > > - ETH_RSS_NONFRAG_IPV6_UDP | \ > > - ETH_RSS_IPV6_UDP_EX) > > - > > /** Static initializer for a list of subsequent item types. */ #define > > NEXT_ITEM(...) \ > > (const enum rte_flow_item_type []){ \ > > @@ -111,7 +76,7 @@ struct mlx4_drop { > > }; > > > > /** > > - * Convert DPDK RSS hash types to their Verbs equivalent. > > + * Convert supported RSS hash field types between DPDK and Verbs > > formats. > > * > > * This function returns the supported (default) set when @p types has > > * special value 0. > > @@ -119,106 +84,76 @@ struct mlx4_drop { > > * @param priv > > * Pointer to private structure. > > * @param types > > - * Hash types in DPDK format (see struct rte_eth_rss_conf). > > + * Depending on @p verbs_to_dpdk, hash types in either DPDK (see struct > > + * rte_eth_rss_conf) or Verbs format. > > + * @param verbs_to_dpdk > > + * A zero value converts @p types from DPDK to Verbs, a nonzero value > > + * performs the reverse operation. > > * > > * @return > > - * A valid Verbs RSS hash fields mask for mlx4 on success, (uint64_t)-1 > > - * otherwise and rte_errno is set. > > + * Converted RSS hash fields on success, (uint64_t)-1 otherwise and > > + * rte_errno is set. > > */ > > uint64_t > > -mlx4_conv_rss_types(struct priv *priv, uint64_t types) > > +mlx4_conv_rss_types(struct priv *priv, uint64_t types, int > > +verbs_to_dpdk) > > { > > - enum { IPV4, IPV6, TCP, UDP, }; > > - static const uint64_t in[] = { > > + enum { > > + INNER, IPV4, IPV6, TCP, UDP, > > + IPV4_TCP, IPV4_UDP, IPV6_TCP, IPV6_UDP, > > + }; > > + static const uint64_t dpdk[] = { > > + [INNER] = 0, > > [IPV4] = (ETH_RSS_IPV4 | > > ETH_RSS_FRAG_IPV4 | > > - ETH_RSS_NONFRAG_IPV4_TCP | > > - ETH_RSS_NONFRAG_IPV4_UDP | > > ETH_RSS_NONFRAG_IPV4_OTHER), > > [IPV6] = (ETH_RSS_IPV6 | > > ETH_RSS_FRAG_IPV6 | > > - ETH_RSS_NONFRAG_IPV6_TCP | > > - ETH_RSS_NONFRAG_IPV6_UDP | > > ETH_RSS_NONFRAG_IPV6_OTHER | > > - ETH_RSS_IPV6_EX | > > - ETH_RSS_IPV6_TCP_EX | > > - ETH_RSS_IPV6_UDP_EX), > > - [TCP] = (ETH_RSS_NONFRAG_IPV4_TCP | > > - ETH_RSS_NONFRAG_IPV6_TCP | > > - ETH_RSS_IPV6_TCP_EX), > > - [UDP] = (ETH_RSS_NONFRAG_IPV4_UDP | > > - ETH_RSS_NONFRAG_IPV6_UDP | > > - ETH_RSS_IPV6_UDP_EX), > > + ETH_RSS_IPV6_EX), > > + [TCP] = 0, > > + [UDP] = 0, > > + [IPV4_TCP] = ETH_RSS_NONFRAG_IPV4_TCP, > > + [IPV4_UDP] = ETH_RSS_NONFRAG_IPV4_UDP, > > + [IPV6_TCP] = (ETH_RSS_NONFRAG_IPV6_TCP | > > + ETH_RSS_IPV6_TCP_EX), > > + [IPV6_UDP] = (ETH_RSS_NONFRAG_IPV6_UDP | > > + ETH_RSS_IPV6_UDP_EX), > > }; > > - static const uint64_t out[RTE_DIM(in)] = { > > + const uint64_t verbs[RTE_DIM(dpdk)] = { > > + [INNER] = IBV_RX_HASH_INNER, > > [IPV4] = IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4, > > [IPV6] = IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6, > > [TCP] = IBV_RX_HASH_SRC_PORT_TCP | > > IBV_RX_HASH_DST_PORT_TCP, > > [UDP] = IBV_RX_HASH_SRC_PORT_UDP | > > IBV_RX_HASH_DST_PORT_UDP, > > + [IPV4_TCP] = verbs[IPV4] | verbs[TCP], > > + [IPV4_UDP] = verbs[IPV4] | verbs[UDP], > > + [IPV6_TCP] = verbs[IPV6] | verbs[TCP], > > + [IPV6_UDP] = verbs[IPV6] | verbs[UDP], > > }; > > + const uint64_t *in = verbs_to_dpdk ? verbs : dpdk; > > + const uint64_t *out = verbs_to_dpdk ? dpdk : verbs; > > uint64_t seen = 0; > > uint64_t conv = 0; > > unsigned int i; > > > > - if (!types) > > - return priv->hw_rss_sup; > > - for (i = 0; i != RTE_DIM(in); ++i) > > + if (!types) { > > + if (!verbs_to_dpdk) > > + return priv->hw_rss_sup; > > + types = priv->hw_rss_sup; > > + } > > + for (i = 0; i != RTE_DIM(dpdk); ++i) > > if (types & in[i]) { > > seen |= types & in[i]; > > conv |= out[i]; > > } > > - if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen)) > > + if ((verbs_to_dpdk || (conv & priv->hw_rss_sup) == conv) && > > + !(types & ~seen)) > > return conv; > > rte_errno = ENOTSUP; > > return (uint64_t)-1; > > } > > > > 1. I wonder if [INNER] case is correct. Assuming we are given IBV_RX_HASH_INNER I think we should return with error ENOTSUP. > However in the implementation above we return 0 as a valid DPDK RSS value. > Returning a 0 value is also confusing since it is used in dpdk as "Best effort" RSS HF. Well, this function performs exactly one job: convert recognized DPDK or Verbs RSS hash types to the other format. Verbs's INNER is supported but has no ETH_RSS equivalent type, hence is discarded. > 2. Assuming we are given = IBV_RX_HASH_SRC_IPV4 alone (without IBV_RX_HASH_DST_IPV4) I think we should return with error ENOSUP since mlx4 does not support a hash based on SRC IPV4 alone. > However in the implementation above we return a valid answer of all the flags from [IPV4], [IPV4_TCP] and [IPV4_UDP]. Can you please explain this logic? > The same question repeats for all other "_DST_" only or "_SRC_" only alone constants. This is done like that for the following reasons: - Original code didn't do it either. - Doing so would be overkill. Consider that the PMD has a list of known supported IBV fields but doesn't choose them on its own, the application does so through a different API without the ability to tell SRC and DST apart. Hence it's safe to toggle them together when dealing with DPDK applications. > 3. Given "IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP" alone we are returning a DPDK reply which includes TCP flags + all IPV4 and IPV6 flags. Can you please explain this logic? Same as above basically. The DPDK API does not have a concept of TCP alone, it's always on top of something (v4, v6, with and without fragments or extended headers, see ETH_RSS_TCP). Therefore a pure TCP input on the Verbs side issues all of them on the DPDK side. > As mentioned I like the idea of one function to transform either direction between verbs and dpdk but please explain the current implementation following the questions above. > > > /** > > - * Convert verbs RSS types to their DPDK equivalents. > > - * > > - * This function returns a group of RSS DPDK types given their equivalent > > group > > - * of verbs types. > > - * For example both source IPv4 and destination IPv4 verbs types are > > converted > > - * into their equivalent RSS group types. If each of these verbs types existed > > - * exclusively - no conversion would take place. > > - * > > - * @param types > > - * RSS hash types in verbs format. > > - * > > - * @return > > - * DPDK RSS hash fields supported by mlx4. > > - */ > > -uint64_t > > -mlx4_ibv_to_rss_types(uint64_t types) > > -{ > > - enum { IPV4, IPV6, IPV4_TCP, IPV6_TCP, IPV4_UDP, IPV6_UDP}; > > - > > - static const uint64_t in[] = { > > - [IPV4] = MLX4_IBV_IPV4_HF, > > - [IPV6] = MLX4_IBV_IPV6_HF, > > - [IPV4_TCP] = MLX4_IBV_IPV4_HF | MLX4_IBV_TCP_HF, > > - [IPV6_TCP] = MLX4_IBV_IPV6_HF | MLX4_IBV_TCP_HF, > > - [IPV4_UDP] = MLX4_IBV_IPV4_HF | MLX4_IBV_UDP_HF, > > - [IPV6_UDP] = MLX4_IBV_IPV6_HF | MLX4_IBV_UDP_HF, > > - }; > > - static const uint64_t out[RTE_DIM(in)] = { > > - [IPV4] = MLX4_RSS_IPV4_HF, > > - [IPV6] = MLX4_RSS_IPV6_HF, > > - [IPV4_TCP] = MLX4_RSS_IPV4_HF | > > MLX4_RSS_IPV4_TCP_HF, > > - [IPV6_TCP] = MLX4_RSS_IPV6_HF | > > MLX4_RSS_IPV6_TCP_HF, > > - [IPV4_UDP] = MLX4_RSS_IPV4_HF | > > MLX4_RSS_IPV4_UDP_HF, > > - [IPV6_UDP] = MLX4_RSS_IPV6_HF | > > MLX4_RSS_IPV6_UDP_HF, > > - }; > > - uint64_t conv = 0; > > - unsigned int i; > > - > > - for (i = 0; i != RTE_DIM(in); ++i) > > - if ((types & in[i]) == in[i]) > > - conv |= out[i]; > > - return conv; > > -} > > - > > -/** > > * Merge Ethernet pattern item into flow rule handle. > > * > > * Additional mlx4-specific constraints on supported fields: > > @@ -890,7 +825,7 @@ mlx4_flow_prepare(struct priv *priv, > > goto exit_action_not_supported; > > } > > rte_errno = 0; > > - fields = mlx4_conv_rss_types(priv, rss->types); > > + fields = mlx4_conv_rss_types(priv, rss->types, 0); > > if (fields == (uint64_t)-1 && rte_errno) { > > msg = "unsupported RSS hash type > > requested"; > > goto exit_action_not_supported; > > diff --git a/drivers/net/mlx4/mlx4_flow.h b/drivers/net/mlx4/mlx4_flow.h > > index 2e82903bd..2917ebe95 100644 > > --- a/drivers/net/mlx4/mlx4_flow.h > > +++ b/drivers/net/mlx4/mlx4_flow.h > > @@ -48,8 +48,8 @@ struct rte_flow { > > > > /* mlx4_flow.c */ > > > > -uint64_t mlx4_conv_rss_types(struct priv *priv, uint64_t types); -uint64_t > > mlx4_ibv_to_rss_types(uint64_t ibv_rss_types); > > +uint64_t mlx4_conv_rss_types(struct priv *priv, uint64_t types, > > + int verbs_to_dpdk); > > int mlx4_flow_sync(struct priv *priv, struct rte_flow_error *error); void > > mlx4_flow_clean(struct priv *priv); int mlx4_filter_ctrl(struct rte_eth_dev > > *dev, > > -- > > 2.11.0 -- Adrien Mazarguil 6WIND