From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0065.outbound.protection.outlook.com [104.47.2.65]) by dpdk.org (Postfix) with ESMTP id 8E5912E81 for ; Fri, 2 Nov 2018 18:54:45 +0100 (CET) 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=VeOySJH8fc7D0xmWu4iA+YBQHZwCtSHbigrDyluM1yY=; b=yrUJovoEnlX4re3Vo7TSOrWfSu913OAjYgkPDy4sJq0IBE7o8QPOT2nu5OUY66xa5TgU+I/7vIIJx2Hrrvc3nBi57PJuEnqCZOjKKBNH7Nj8yqMKAeimgLBGAgNaIk/oMXZ6g+M0Zto941g6WGPMc3n7+Pw3qCh1cnVQ/t+tI20= Received: from VI1PR0502MB3743.eurprd05.prod.outlook.com (52.134.8.154) by VI1PR0502MB3726.eurprd05.prod.outlook.com (52.134.8.149) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1273.25; Fri, 2 Nov 2018 17:54:43 +0000 Received: from VI1PR0502MB3743.eurprd05.prod.outlook.com ([fe80::84d3:8812:d45c:98e4]) by VI1PR0502MB3743.eurprd05.prod.outlook.com ([fe80::84d3:8812:d45c:98e4%4]) with mapi id 15.20.1294.024; Fri, 2 Nov 2018 17:54:43 +0000 From: Ophir Munk To: "dev@dpdk.org" , Yongseok Koh CC: Thomas Monjalon , Asaf Penso , Olga Shern , Ophir Munk Thread-Topic: [PATCH v2] net/mlx5: set RSS key to NULL to indicate default RSS Thread-Index: AQHUccEnLoFSrTYmp0WASea6Fnjxl6U68w6AgAAOR7CAAcT0UA== Date: Fri, 2 Nov 2018 17:54:42 +0000 Message-ID: References: <1538588246-21436-1-git-send-email-ophirmu@mellanox.com> <1541062744-25491-1-git-send-email-ophirmu@mellanox.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: [109.186.62.238] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; VI1PR0502MB3726; 6:OtdaYydgD/DjkVHhonpz66nci85QOjMexECPcwXBYgbQFRxzvFTOpPy+RsfbOX15f+v3BqD1TntpnJBQpOu3+EC1oiFcSHEz4RklnhSMPJ+6+NHBVURKh8cJBr4AkR61M9v8dweXSe+c5radpk1ubNWWoEvSoz50pSwT4ylpnjWbZMkrfLCfhfpw2s2rumJ36HCcAxMNhENalJD0nLh/njy369xfnLf0NXRObA1447+BJh4AkyFWjQlJHa40i4kmwyBzPNa69qX7/zJP7M9qYJ+cz8/4uAaXkgsrgLn5Z6u05X1Bz+lviuSwV8XuLPVGMwqLvizu4qmO57EhbUog1BV1nXs7eXOaidhJ4iJnVGvEzjVjdGIp4UqbdEXNflQknvBpdGpeIqJwQivvCDuM0CcxIDkF7D6dBNpQadtuTlQ+y/UXXNmed5IM3SWphxLkZjgeX3UBYN5pUPx7Z3k3uQ==; 5:vegmFG2hzyumOIw3XsCK3LWJXkwPw8abrZz1t3nln0+8+d+Sufl/angCBZMpxfL1CdjonBZgp/dhfi1ewDoZITuDmHJHkx6eb4QVzsYKwH6U4KeSCZH/ycKGCtVcOa0guwe9WFKwSDgre1ta4ahVYZ0CqamLBqh6ErSMj7Bu9yg=; 7:MNjhUuywc1ZU+LokaBoISyigyMXcPE75JtvD6tLXkZHDoeLLr3IOy4er/fbANFIwmh+EczsANab7OdI0CyxTAl4b4r3D40k5ADxEttkukkJ4WIo7K0QKyI1B898Us2mgbnhUvGRAM/zUnegtRswDgQ== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 5d5fe542-810d-4780-6bac-08d640ec44db x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:VI1PR0502MB3726; x-ms-traffictypediagnostic: VI1PR0502MB3726: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(278428928389397)(17755550239193); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3231382)(944501410)(52105095)(93006095)(93001095)(3002001)(10201501046)(6055026)(148016)(149066)(150057)(6041310)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123558120)(20161123564045)(20161123560045)(20161123562045)(201708071742011)(7699051)(76991095); SRVR:VI1PR0502MB3726; BCL:0; PCL:0; RULEID:; SRVR:VI1PR0502MB3726; x-forefront-prvs: 08444C7C87 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(366004)(346002)(376002)(396003)(136003)(199004)(13464003)(189003)(2906002)(81156014)(3846002)(102836004)(81166006)(54906003)(53546011)(76176011)(26005)(74316002)(66066001)(186003)(8936002)(6506007)(7696005)(6116002)(6636002)(8676002)(316002)(107886003)(4326008)(68736007)(53936002)(33656002)(97736004)(256004)(14444005)(229853002)(71190400001)(110136005)(478600001)(106356001)(105586002)(6436002)(7736002)(2501003)(305945005)(575784001)(486006)(446003)(25786009)(2473003)(99286004)(55016002)(86362001)(5660300001)(9686003)(2900100001)(476003)(71200400001)(14454004); DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR0502MB3726; H:VI1PR0502MB3743.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: wPgMWVp2lTRQo15ksurrqit+CpZGpse6htP/QPlXLCehzzmQ7uH6j1yHepBOLZN6xsvtWvQZh8xqS1eIYw8bvxaH9Kr2dEenukbn1Ckoqkaf4oGmvufKxNzfcfUbW6L8FULlqs61541Gy2/kucDAkZfizGJ8gWQiuDE18vpdBT6TU0MS560AyUiH1P5nJCl4R7dFDUCqBE3QdSid5VGS9D5ou+HbT7pmMB0YXR6ekd48dZTEBSsMBsNG4oJlthSZQHjBo327IZlxWKlMr/ynLGCNLUDBIQbJq9/H4QaJBLiwf4lWQWW0nv3cfB77RWiMXte2T+GHd5W63nVCdyBnrSfPYm+SPtlGQnbv2UBbj5M= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5d5fe542-810d-4780-6bac-08d640ec44db X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Nov 2018 17:54:42.8537 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0502MB3726 Subject: [dpdk-dev] FW: [PATCH v2] net/mlx5: set RSS key to NULL to indicate default RSS 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, 02 Nov 2018 17:54:45 -0000 > -----Original Message----- > From: Shahaf Shuler > Sent: Thursday, November 01, 2018 4:01 PM > To: Ophir Munk ; dev@dpdk.org; Yongseok Koh=20 > > Cc: Thomas Monjalon ; Olga Shern=20 > ; Asaf Penso > Subject: RE: [PATCH v2] net/mlx5: set RSS key to NULL to indicate=20 > default RSS >=20 > Hi Ophir, >=20 > Thursday, November 1, 2018 10:59 AM, Ophir Munk: > > Subject: [PATCH v2] net/mlx5: set RSS key to NULL to indicate=20 > > default RSS > > > > Applications which add RSS rules must supply an RSS key and length. > > If an application is only interested in default RSS operation it=20 > > should not care about the exact RSS key. > > By setting the key to NULL - the PMD will use the default RSS key. > > In addition if the application does not care about the RSS type it=20 > > can set it to 0 and the PMD will use the default type (ETH_RSS_IP). > > > > Signed-off-by: Ophir Munk > > --- > > doc/guides/nics/mlx5.rst | 1 + > > drivers/net/mlx5/mlx5_flow.c | 8 +++++++- > > drivers/net/mlx5/mlx5_flow_dv.c | 8 ++++++-- > > drivers/net/mlx5/mlx5_flow_verbs.c | 9 +++++++-- > > drivers/net/mlx5/mlx5_rxq.c | 14 ++++++-------- > > lib/librte_ethdev/rte_flow.h | 2 +- > > 6 files changed, 28 insertions(+), 14 deletions(-) > > > > diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst=20 > > index 6769628..de8f15b 100644 > > --- a/doc/guides/nics/mlx5.rst > > +++ b/doc/guides/nics/mlx5.rst > > @@ -54,6 +54,7 @@ Features > > - Support for scattered TX and RX frames. > > - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of > queues. > > - Several RSS hash keys, one for each flow type. > > +- NULL RSS key indicates default RSS key >=20 > I don't think it is a feature, it should be an API implementation. 1. There is no "API implementation" section in the mlx5.rst doc. Please ind= icate where exactly you think it should be added. 2. I think you can consider this a feature: applications can use default RS= S without knowing any specific RSS parameters. Setting key to NULL is just the mean to get it (maybe should not be mention= ed). How about: Default RSS operation with no hash key specification >=20 > > - Configurable RETA table. > > - Support for multiple MAC addresses. > > - VLAN filtering. > > diff --git a/drivers/net/mlx5/mlx5_flow.c=20 > > b/drivers/net/mlx5/mlx5_flow.c index 280af0a..5742989 100644 > > --- a/drivers/net/mlx5/mlx5_flow.c > > +++ b/drivers/net/mlx5/mlx5_flow.c > > @@ -912,7 +912,13 @@ uint32_t mlx5_flow_adjust_priority(struct=20 > > rte_eth_dev *dev, int32_t priority, > > > > RTE_FLOW_ERROR_TYPE_ACTION_CONF, > > &rss->level, > > "tunnel RSS is not supported"); > > - if (rss->key_len < MLX5_RSS_HASH_KEY_LEN) > > + /* allow RSS key_len 0 in case of NULL (default) RSS key */ >=20 > Missing period. Will be added in v3 >=20 > > + if (rss->key_len =3D=3D 0 && rss->key !=3D NULL) > > + return rte_flow_error_set(error, ENOTSUP, > > + > > RTE_FLOW_ERROR_TYPE_ACTION_CONF, > > + &rss->key_len, > > + "RSS hash key length 0"); > > + if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN) >=20 > Can't you simplify using a single condition: > If (rss->key && rss->key_len < MLX5_RSS_HASH_KEY_LEN) >=20 Do you mean that if key=3D=3DNULL we do not care about the key_len? (since if key=3D=3DNULL key_len can have any value?=20 Wil update in V3 > Same rss->key addition should be for the case the key_len is too large. >=20 > > return rte_flow_error_set(error, ENOTSUP, > > > > RTE_FLOW_ERROR_TYPE_ACTION_CONF, > > &rss->key_len, > > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c=20 > > b/drivers/net/mlx5/mlx5_flow_dv.c index 8f729f4..fd52bf4 100644 > > --- a/drivers/net/mlx5/mlx5_flow_dv.c > > +++ b/drivers/net/mlx5/mlx5_flow_dv.c > > @@ -1056,6 +1056,7 @@ > > { > > const struct rte_flow_action_queue *queue; > > const struct rte_flow_action_rss *rss; > > + uint8_t *rss_key; >=20 > Why no declare it inside the RSS case? It is consistent with the coding style in this file. 1. There is no variable declaration inside any case. 2. If rss_key is declared inside the RSS case - the same should be for rss = variable (for consistency) and there can be more cases to check. >=20 > > int actions_n =3D dev_flow->dv.actions_n; > > struct rte_flow *flow =3D dev_flow->flow; > > > > @@ -1094,8 +1095,11 @@ > > memcpy((*flow->queue), rss->queue, > > rss->queue_num * sizeof(uint16_t)); > > flow->rss.queue_num =3D rss->queue_num; > > - memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN); > > - flow->rss.types =3D rss->types; > > + /* NULL RSS key indicates default RSS key */ >=20 > Missing period. Will be added in v3 >=20 > > + rss_key =3D (!rss->key) ? rss_hash_default_key : rss->key; > > + memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN); >=20 > For consistency you should also set the key_len on the flow structure=20 > to MLX5_RSS_HASH_KEY_LEN. >=20 There is no key_len on the flow structure.=20 I guess the key_len field is missing since it is redundant (key_len is alwa= ys 40 bytes). > > + /* RSS type 0 indicates default RSS type ETH_RSS_IP */ >=20 > Period. Will be added in v3 >=20 > > + flow->rss.types =3D !rss->types ? ETH_RSS_IP : rss->types; > > flow->rss.level =3D rss->level; > > /* Added to array only in apply since we need the QP */ > > flow->actions |=3D MLX5_FLOW_ACTION_RSS; diff --git=20 > > a/drivers/net/mlx5/mlx5_flow_verbs.c > > b/drivers/net/mlx5/mlx5_flow_verbs.c > > index 81bc39f..ae9042f 100644 > > --- a/drivers/net/mlx5/mlx5_flow_verbs.c > > +++ b/drivers/net/mlx5/mlx5_flow_verbs.c > > @@ -925,14 +925,19 @@ > > struct mlx5_flow *dev_flow) > > { > > const struct rte_flow_action_rss *rss =3D action->conf; > > + const uint8_t *rss_key; > > struct rte_flow *flow =3D dev_flow->flow; > > > > if (flow->queue) > > memcpy((*flow->queue), rss->queue, > > rss->queue_num * sizeof(uint16_t)); > > flow->rss.queue_num =3D rss->queue_num; > > - memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN); > > - flow->rss.types =3D rss->types; > > + > > + /* NULL RSS key indicates default RSS key */ >=20 > period Will be added in v3 >=20 > > + rss_key =3D !rss->key ? rss_hash_default_key : rss->key; >=20 > For consistency you should also set the key_len on the flow structure=20 > to MLX5_RSS_HASH_KEY_LEN. >=20 There is no key_len on the flow structure. > > + memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN); > > + /* RSS type 0 indicates default RSS type (ETH_RSS_IP) */ >=20 > period Will be added in v3 >=20 > > + flow->rss.types =3D !rss->types ? ETH_RSS_IP : rss->types; > > flow->rss.level =3D rss->level; > > *action_flags |=3D MLX5_FLOW_ACTION_RSS; } diff --git=20 > > a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c index > > ed993ea..36fc202 100644 > > --- a/drivers/net/mlx5/mlx5_rxq.c > > +++ b/drivers/net/mlx5/mlx5_rxq.c > > @@ -1783,10 +1783,6 @@ struct mlx5_hrxq * > > rte_errno =3D ENOMEM; > > return NULL; > > } > > - if (!rss_key_len) { > > - rss_key_len =3D MLX5_RSS_HASH_KEY_LEN; > > - rss_key =3D rss_hash_default_key; > > - } > > #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT > > qp =3D mlx5_glue->dv_create_qp > > (priv->ctx, > > @@ -1798,8 +1794,7 @@ struct mlx5_hrxq * > > IBV_QP_INIT_ATTR_RX_HASH, > > .rx_hash_conf =3D (struct ibv_rx_hash_conf){ > > .rx_hash_function =3D > > IBV_RX_HASH_FUNC_TOEPLITZ, > > - .rx_hash_key_len =3D rss_key_len ? > > rss_key_len : > > - > MLX5_RSS_HASH_KEY_LEN, > > + .rx_hash_key_len =3D rss_key_len, > > .rx_hash_key =3D rss_key ? > > (void *)(uintptr_t)rss_key : > > rss_hash_default_key, > > @@ -1824,8 +1819,7 @@ struct mlx5_hrxq * > > IBV_QP_INIT_ATTR_RX_HASH, > > .rx_hash_conf =3D (struct ibv_rx_hash_conf){ > > .rx_hash_function =3D > > IBV_RX_HASH_FUNC_TOEPLITZ, > > - .rx_hash_key_len =3D rss_key_len ? > > rss_key_len : > > - > MLX5_RSS_HASH_KEY_LEN, > > + .rx_hash_key_len =3D rss_key_len, > > .rx_hash_key =3D rss_key ? > > (void *)(uintptr_t)rss_key : > > rss_hash_default_key, > > @@ -1888,8 +1882,12 @@ struct mlx5_hrxq * > > LIST_FOREACH(hrxq, &priv->hrxqs, next) { > > struct mlx5_ind_table_ibv *ind_tbl; > > > > + if (!rss_key_len) > > + rss_key_len =3D MLX5_RSS_HASH_KEY_LEN; >=20 > In the mlx5_hrxq_new above you trust the rss_key_len to be the correct=20 > one, and it is ok since you validate it on the rss validate function. > Why it is not the case here as well? >=20 Will be updated in v3. Remark: rss_key_len assignment can be dropped because calling mlx5_hrxq_ge= t is done with constant MLX5_RSS_HASH_KEY_LEN already assigned to rss_key_l= en=20 > > if (hrxq->rss_key_len !=3D rss_key_len) > > continue; > > + if (!rss_key) > > + rss_key =3D rss_hash_default_key; >=20 > Same question. You already copied to correct key in=20 > flow_verbs_translate_action_rss or on the flow_dv_create_action funciton. Will be dropped in v3 Will also drop in v3 another legacy code which does redundant rss_key=3D=3D= NULL check >=20 > > if (memcmp(hrxq->rss_key, rss_key, rss_key_len)) > > continue; > > if (hrxq->hash_fields !=3D hash_fields) diff --git=20 > > a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h index=20 > > c0fe879..6cc72ab 100644 > > --- a/lib/librte_ethdev/rte_flow.h > > +++ b/lib/librte_ethdev/rte_flow.h > > @@ -1785,7 +1785,7 @@ struct rte_flow_action_rss { > > uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*). */ > > uint32_t key_len; /**< Hash key length in bytes. */ > > uint32_t queue_num; /**< Number of entries in @p queue. */ > > - const uint8_t *key; /**< Hash key. */ > > + const uint8_t *key; /**< Hash key. NULL should indicate default=20 > > +key >=20 > This change should be on a separate commit in a separate series. > It is better to put the doc update above the function and not inline=20 > on the field so that you can elaborate more. > Also you are missing the same doc update for the types field. >=20 Will issue a sepearte commit in a separate series > Note: your patch for mlx5 can be accepted also w/o it, but it is=20 > better to align all vendors and I think it is aligned w/ the spirit of=20 > the rte_flow maintainer. >=20 > > */ > > const uint16_t *queue; /**< Queue indices to use. */ }; > > > > -- > > 1.8.3.1