From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0082.outbound.protection.outlook.com [104.47.0.82]) by dpdk.org (Postfix) with ESMTP id 1B1418BA7 for ; Thu, 17 May 2018 19:46:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=2Um8McXmdGehWYHDZxepxfXMcxQrMFLdnixq+zSRlDw=; b=srVFArWfgos+a5KAvF83a96tVuiHkO4SObSLFe7VbeHBMDQrcOHTvRRXTtV37J8FDIZ1awX+Ev7rnLk9zYG2mhE6gH1nLVVkb5xpv+kUvPyNQAtz5TPvBOCnGy6z5m4ti7QXEiQ6FmBC4facRu7Pi1tjoVamRcb5J/bNcCZUwgo= Received: from HE1PR0501MB2314.eurprd05.prod.outlook.com (10.168.34.19) by HE1PR0501MB2139.eurprd05.prod.outlook.com (10.167.246.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.755.16; Thu, 17 May 2018 17:46:45 +0000 Received: from HE1PR0501MB2314.eurprd05.prod.outlook.com ([fe80::1071:70d0:eac1:5d97]) by HE1PR0501MB2314.eurprd05.prod.outlook.com ([fe80::1071:70d0:eac1:5d97%18]) with mapi id 15.20.0776.010; Thu, 17 May 2018 17:46:46 +0000 From: Ophir Munk To: Adrien Mazarguil , Shahaf Shuler CC: "dev@dpdk.org" Thread-Topic: [PATCH 2/2] net/mlx4: refactor RSS conversion functions Thread-Index: AQHT7GSU2XJWOtCnhESr6BANNYSRoaQ0HQTA Date: Thu, 17 May 2018 17:46:45 +0000 Message-ID: References: <20180515154853.6361-1-adrien.mazarguil@6wind.com> <20180515154853.6361-2-adrien.mazarguil@6wind.com> In-Reply-To: <20180515154853.6361-2-adrien.mazarguil@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=ophirmu@mellanox.com; x-originating-ip: [85.250.111.138] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; HE1PR0501MB2139; 7:Hsfg97yGec9t6sb0l63ZEDnfE/I4GB65qZhKDhI+JDH01dG/KGVbtHOBGqkilL4t5veDisoQoGcNgmY8duMCJiwi7UrbhIEf6PFMePAl3bsC4ScQWz9WmzcMsZxnKVcLUqJE74bBhIjipTUOm5m48WJvWOadr7kBlyWnrnNJ8lqs4MH/+nwEzDSg+o0gxuHNfWChxx/qVBr0t1/7XipT5HkOHn2snbE0TCRGcqfZxSyKu9wfr+OoLM7bx0X4pLRk x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(5600026)(48565401081)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603328)(7153060)(7193020); SRVR:HE1PR0501MB2139; x-ms-traffictypediagnostic: HE1PR0501MB2139: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3231254)(944501410)(52105095)(10201501046)(3002001)(93006095)(93001095)(6055026)(149027)(150027)(6041310)(20161123560045)(20161123558120)(20161123564045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(6072148)(201708071742011)(7699016); SRVR:HE1PR0501MB2139; BCL:0; PCL:0; RULEID:; SRVR:HE1PR0501MB2139; x-forefront-prvs: 067553F396 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(346002)(396003)(376002)(39380400002)(39860400002)(366004)(54094003)(199004)(189003)(13464003)(26005)(74316002)(486006)(316002)(7696005)(6506007)(86362001)(102836004)(53546011)(76176011)(5660300001)(106356001)(68736007)(66066001)(105586002)(2906002)(2900100001)(14454004)(110136005)(186003)(11346002)(446003)(476003)(25786009)(81166006)(81156014)(6636002)(97736004)(3280700002)(8936002)(7736002)(8676002)(478600001)(6116002)(305945005)(4326008)(33656002)(6436002)(9686003)(6246003)(53936002)(99286004)(55016002)(5250100002)(3846002)(3660700001)(229853002); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR0501MB2139; H:HE1PR0501MB2314.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: itPnaVsZ+/c6fOII+kTezHz8TbeLuWbRr5JI5Y4XoMLzslU/xX4PCGmZHvBCNezMEaBJeU06D620ZM8134gpw9yrFJUnV8jO1x464KNnahUoeSWOMvYXB5ssl1aFAHIjuMl/ak/NZILlNb/8p9hLHHVNMqAm9H5WcPSAIILdbQWqVBcOURxacZTOiNyHwbAM spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 7ec27a79-4f5f-4d80-efa4-08d5bc1e28b2 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7ec27a79-4f5f-4d80-efa4-08d5bc1e28b2 X-MS-Exchange-CrossTenant-originalarrivaltime: 17 May 2018 17:46:45.9248 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0501MB2139 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: Thu, 17 May 2018 17:46:48 -0000 Hi Adrien, I like the idea of having one conversion function from dpdk to verbs and vi= ce 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 >=20 > 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. >=20 > This patch refactors them as a single two-way function and gets rid of > redundant helper macros. >=20 > 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 tr= ade-off > given this function is not involved in data plane operations. >=20 > 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(-) >=20 > 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 =3D > - mlx4_ibv_to_rss_types(priv->hw_rss_sup); > + info->flow_type_rss_offloads =3D mlx4_conv_rss_types(priv, 0, 1); Calling with parameters (priv, 0, 1) parameters in not as intuitive as call= ing with (priv, DEFAULT_RSS, DPDK_TO_VERBS) Would you consider using definitions for the 0, 1 parameters to increase re= adability? > } >=20 > /** > 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" >=20 > -/** 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 { > }; >=20 > /** > - * 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 struc= t > + * 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[] =3D { > + enum { > + INNER, IPV4, IPV6, TCP, UDP, > + IPV4_TCP, IPV4_UDP, IPV6_TCP, IPV6_UDP, > + }; > + static const uint64_t dpdk[] =3D { > + [INNER] =3D 0, > [IPV4] =3D (ETH_RSS_IPV4 | > ETH_RSS_FRAG_IPV4 | > - ETH_RSS_NONFRAG_IPV4_TCP | > - ETH_RSS_NONFRAG_IPV4_UDP | > ETH_RSS_NONFRAG_IPV4_OTHER), > [IPV6] =3D (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] =3D (ETH_RSS_NONFRAG_IPV4_TCP | > - ETH_RSS_NONFRAG_IPV6_TCP | > - ETH_RSS_IPV6_TCP_EX), > - [UDP] =3D (ETH_RSS_NONFRAG_IPV4_UDP | > - ETH_RSS_NONFRAG_IPV6_UDP | > - ETH_RSS_IPV6_UDP_EX), > + ETH_RSS_IPV6_EX), > + [TCP] =3D 0, > + [UDP] =3D 0, > + [IPV4_TCP] =3D ETH_RSS_NONFRAG_IPV4_TCP, > + [IPV4_UDP] =3D ETH_RSS_NONFRAG_IPV4_UDP, > + [IPV6_TCP] =3D (ETH_RSS_NONFRAG_IPV6_TCP | > + ETH_RSS_IPV6_TCP_EX), > + [IPV6_UDP] =3D (ETH_RSS_NONFRAG_IPV6_UDP | > + ETH_RSS_IPV6_UDP_EX), > }; > - static const uint64_t out[RTE_DIM(in)] =3D { > + const uint64_t verbs[RTE_DIM(dpdk)] =3D { > + [INNER] =3D IBV_RX_HASH_INNER, > [IPV4] =3D IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4, > [IPV6] =3D IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6, > [TCP] =3D IBV_RX_HASH_SRC_PORT_TCP | > IBV_RX_HASH_DST_PORT_TCP, > [UDP] =3D IBV_RX_HASH_SRC_PORT_UDP | > IBV_RX_HASH_DST_PORT_UDP, > + [IPV4_TCP] =3D verbs[IPV4] | verbs[TCP], > + [IPV4_UDP] =3D verbs[IPV4] | verbs[UDP], > + [IPV6_TCP] =3D verbs[IPV6] | verbs[TCP], > + [IPV6_UDP] =3D verbs[IPV6] | verbs[UDP], > }; > + const uint64_t *in =3D verbs_to_dpdk ? verbs : dpdk; > + const uint64_t *out =3D verbs_to_dpdk ? dpdk : verbs; > uint64_t seen =3D 0; > uint64_t conv =3D 0; > unsigned int i; >=20 > - if (!types) > - return priv->hw_rss_sup; > - for (i =3D 0; i !=3D RTE_DIM(in); ++i) > + if (!types) { > + if (!verbs_to_dpdk) > + return priv->hw_rss_sup; > + types =3D priv->hw_rss_sup; > + } > + for (i =3D 0; i !=3D RTE_DIM(dpdk); ++i) > if (types & in[i]) { > seen |=3D types & in[i]; > conv |=3D out[i]; > } > - if ((conv & priv->hw_rss_sup) =3D=3D conv && !(types & ~seen)) > + if ((verbs_to_dpdk || (conv & priv->hw_rss_sup) =3D=3D conv) && > + !(types & ~seen)) > return conv; > rte_errno =3D ENOTSUP; > return (uint64_t)-1; > } >=20 1. I wonder if [INNER] case is correct. Assuming we are given IBV_RX_HASH_I= NNER 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 eff= ort" RSS HF. 2. Assuming we are given =3D IBV_RX_HASH_SRC_IPV4 alone (without IBV_RX_HAS= H_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 [IPV= 4_UDP]. Can you please explain this logic? The same question repeats for all other "_DST_" only or "_SRC_" only alone = constants. 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? As mentioned I like the idea of one function to transform either direction = between verbs and dpdk but please explain the current implementation follow= ing the questions above. > /** > - * Convert verbs RSS types to their DPDK equivalents. > - * > - * This function returns a group of RSS DPDK types given their equivalen= t > 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 e= xisted > - * 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[] =3D { > - [IPV4] =3D MLX4_IBV_IPV4_HF, > - [IPV6] =3D MLX4_IBV_IPV6_HF, > - [IPV4_TCP] =3D MLX4_IBV_IPV4_HF | MLX4_IBV_TCP_HF, > - [IPV6_TCP] =3D MLX4_IBV_IPV6_HF | MLX4_IBV_TCP_HF, > - [IPV4_UDP] =3D MLX4_IBV_IPV4_HF | MLX4_IBV_UDP_HF, > - [IPV6_UDP] =3D MLX4_IBV_IPV6_HF | MLX4_IBV_UDP_HF, > - }; > - static const uint64_t out[RTE_DIM(in)] =3D { > - [IPV4] =3D MLX4_RSS_IPV4_HF, > - [IPV6] =3D MLX4_RSS_IPV6_HF, > - [IPV4_TCP] =3D MLX4_RSS_IPV4_HF | > MLX4_RSS_IPV4_TCP_HF, > - [IPV6_TCP] =3D MLX4_RSS_IPV6_HF | > MLX4_RSS_IPV6_TCP_HF, > - [IPV4_UDP] =3D MLX4_RSS_IPV4_HF | > MLX4_RSS_IPV4_UDP_HF, > - [IPV6_UDP] =3D MLX4_RSS_IPV6_HF | > MLX4_RSS_IPV6_UDP_HF, > - }; > - uint64_t conv =3D 0; > - unsigned int i; > - > - for (i =3D 0; i !=3D RTE_DIM(in); ++i) > - if ((types & in[i]) =3D=3D in[i]) > - conv |=3D 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 =3D 0; > - fields =3D mlx4_conv_rss_types(priv, rss->types); > + fields =3D mlx4_conv_rss_types(priv, rss->types, 0); > if (fields =3D=3D (uint64_t)-1 && rte_errno) { > msg =3D "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 { >=20 > /* mlx4_flow.c */ >=20 > -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); vo= id > mlx4_flow_clean(struct priv *priv); int mlx4_filter_ctrl(struct rte_eth_= dev > *dev, > -- > 2.11.0