From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0079.outbound.protection.outlook.com [104.47.0.79]) by dpdk.org (Postfix) with ESMTP id A8931201 for ; Fri, 9 Nov 2018 00:07:26 +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=+jDCguOeeNv4gekYopEIaZKKCsrWX1MiSCEAl5s+rcs=; b=KxKCs9S1Cs3exT5ZlkcmgDNtuPuJV8ma4H+aM9c7cV7TRP4jUNu81ERzofhCiYfJX+24RpFijWDN50sZrezAN0+WbfoRsSEeT6b4Qs9dHJ8LP+BHSpx9CyqoyjcL+wcSZqnPYCYwyhmWnhTQyshZVKWtf2N+W+/OX/L0gxvbfKM= Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) by DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1294.27; Thu, 8 Nov 2018 23:07:23 +0000 Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::58e7:97d8:f9c1:4323]) by DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::58e7:97d8:f9c1:4323%3]) with mapi id 15.20.1294.034; Thu, 8 Nov 2018 23:07:23 +0000 From: Yongseok Koh To: Ophir Munk , Adrien Mazarguil , Andrew Rybchenko CC: Ferruh Yigit , "dev@dpdk.org" , Thomas Monjalon , Asaf Penso , Shahaf Shuler , Olga Shern Thread-Topic: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types Thread-Index: AQHUdnuYjfK7eXc6vU2uBox7+ptrWaVEDEOAgAA0mQCAABg3AIAAEryAgAAYpICAAQ60AIAA730A Date: Thu, 8 Nov 2018 23:07:23 +0000 Message-ID: References: <1541259953-4273-1-git-send-email-ophirmu@mellanox.com> <1541582611-1609-1-git-send-email-ophirmu@mellanox.com> <20181107093109.GG4638@6wind.com> <20181107140604.GL4638@6wind.com> <20181107164118.GM4638@6wind.com> In-Reply-To: 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=yskoh@mellanox.com; x-originating-ip: [69.181.245.183] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB3PR0502MB3980; 6:jzFK+HxPu7ATJOTrC5rwX4vn3e0FmjxLPVf2NIkq7a+AX+i9Dv8JG1WsquxPoqK1BUQ9fzCTRJWD326qfzEi/sU7owX16zx2xliNIR3Gk3ZoAB5/FX8EOu7FSJew0F+gFluzAytIH1uux0CRdw/0wFEVxuenvlEnaJkY5vXh+hknr7zWVuIaEhJpLv1r+mFh3Ha2vOVYcTFWoXn9Eaw1ew8x7B3q0nVnIFJFaaNpkSurJsgafQpRDoShfrPso+sRwQwVhXZ1EJ8GPQ9IHZKY2TyWuHkYiWW/QnRIBaqYE4m/D087X8RddysuUyRKKDWVUsDJiP2GiZy2w/a9gD/7supqH/O6kTLyGGBRTtmrQrHiQSFAb7wcEwOu8BL9q+L6WEkC10yj8VEv65QbaTgxkKlb78We4GPq+i1Lz+ZTMYE0wZtciyI5BjYu6hxVQiB+X5kZOBlCk0C0LOD2CfOWiQ==; 5:L3xwJKjQ1aMvw5QQcJdE7AV3feKgMacyi6EFL6rOGzHSH5BCqX0wppyUdXm85lSobV/rZ2zUKXQOQf0No7SGwCj/X72HZMmFuV0RSFWcqnqHcZj7/Pdoe+TfZjZBli3Vsu2Fb6ngPgKM24eYNoHOGM3j9rPQHwryJkmVgAmPEbM=; 7:hD57GeO2Z/xL4MLe7Q28YPRz84Okf3RX10JVfm8pzd0moQads1EFQk2Gyr26NyjssLJOM0WZFzuOlMPecR/54I3Ek4glRCrArfuc6l/40L2VHMGZmj5swqA1HxI3r3nOXK4xy6XiZPC2NKJIXg6IpQ== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: d44056ed-e21f-482e-b327-08d645cef187 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:DB3PR0502MB3980; x-ms-traffictypediagnostic: DB3PR0502MB3980: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(45079756050767)(189930954265078)(278428928389397)(228905959029699); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3231382)(944501410)(52105095)(10201501046)(93006095)(93001095)(3002001)(6055026)(148016)(149066)(150057)(6041310)(20161123564045)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123558120)(201708071742011)(7699051)(76991095); SRVR:DB3PR0502MB3980; BCL:0; PCL:0; RULEID:; SRVR:DB3PR0502MB3980; x-forefront-prvs: 0850800A29 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(346002)(376002)(39860400002)(396003)(136003)(366004)(13464003)(189003)(199004)(6486002)(86362001)(2906002)(53546011)(478600001)(575784001)(6506007)(6512007)(6306002)(99286004)(82746002)(45080400002)(97736004)(8936002)(2616005)(54906003)(11346002)(81156014)(26005)(71200400001)(81166006)(71190400001)(76176011)(102836004)(966005)(486006)(83716004)(229853002)(36756003)(8676002)(446003)(476003)(2900100001)(14454004)(186003)(68736007)(305945005)(316002)(6116002)(66066001)(5660300001)(4326008)(105586002)(33656002)(107886003)(93886005)(106356001)(25786009)(53936002)(6436002)(110136005)(14444005)(256004)(6246003)(7736002)(3846002); DIR:OUT; SFP:1101; SCL:1; SRVR:DB3PR0502MB3980; H:DB3PR0502MB3980.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: wPS8iiLrOwzyuZL4vuEs1mrdl7z6AcZ95/iyQnl77t49eOufvywCT31YEddaN40iwTmzWGSuixRz3L3+jlDPw3zli6lOg2RH2IhI0CCFizr7HA5unM0UcHg0zjwq1ZLTXFzlLcfYqXXq88XPd0Q/1/2ljsxOQmMQcKtY8OCAbVQFTGMKiFiN+Fafo8ykKKHzrhVaYZ8P0dxN4QApJiNKUsjxCQmlpldOTLKWO/IV859fHf3zgTc/AND2h2MzCt6uBY6h6yNt8lseLqJSl/+MFVTb0TMa6jsioLsId7kX90UTMxYk2D8ydxah0vPb/jq74MZJgHPJJRvd0/JyGDyWfhf/in22JXTrZUiy57yHTtg= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-ID: <5B71180494C90347AEBEE822A013615D@eurprd05.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: d44056ed-e21f-482e-b327-08d645cef187 X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Nov 2018 23:07:23.5879 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0502MB3980 Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types 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, 08 Nov 2018 23:07:27 -0000 > On Nov 8, 2018, at 12:50 AM, Ophir Munk wrote: >=20 >=20 >=20 >> -----Original Message----- >> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] >> Sent: Wednesday, November 07, 2018 6:41 PM >> To: Ophir Munk >> Cc: Ferruh Yigit ; Andrew Rybchenko >> ; dev@dpdk.org; Thomas Monjalon >> ; Asaf Penso ; Shahaf >> Shuler ; Olga Shern >> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and >> types >>=20 >> On Wed, Nov 07, 2018 at 03:13:07PM +0000, Ophir Munk wrote: >>>=20 >>>> -----Original Message----- >>>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] >>>> Sent: Wednesday, November 07, 2018 4:06 PM >>>> To: Ophir Munk >>>> Cc: Ferruh Yigit ; Andrew Rybchenko >>>> ; dev@dpdk.org; Thomas Monjalon >>>> ; Asaf Penso ; Shahaf >>>> Shuler ; Olga Shern >>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key >>>> and types >>>>=20 >>>> On Wed, Nov 07, 2018 at 12:39:24PM +0000, Ophir Munk wrote: >>>>>> -----Original Message----- >>>>>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] >>>>>> Sent: Wednesday, November 07, 2018 11:31 AM >>>>>> To: Ophir Munk >>>>>> Cc: Ferruh Yigit ; Andrew Rybchenko >>>>>> ; dev@dpdk.org; Thomas Monjalon >>>>>> ; Asaf Penso ; >> Shahaf >>>>>> Shuler ; Olga Shern >> >>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default >>>>>> key and types >>>>>>=20 >>>>>> On Wed, Nov 07, 2018 at 09:23:42AM +0000, Ophir Munk wrote: >>>>>>> struct rte_flow_action_rss include fields 'key' and 'types'. >>>>>>> Field 'key' is a pointer to bytes array (uint8_t *) which >>>>>>> contains the specific RSS hash key. >>>>>>> If an application is only interested in default RSS operation >>>>>>> it should not care about the specific hash key. The >>>>>>> application can set the hash key to NULL such that any PMD uses its >> default RSS key. >>>>>>>=20 >>>>>>> Field 'types' is a uint64_t bits flag used to specify a >>>>>>> specific RSS hash type such as ETH_RSS_IP (see ETH_RSS_*). >>>>>>> If an application does not care about the specific RSS type it >>>>>>> can set this field to 0 such that any PMD uses its default type. >>>>>>>=20 >>>>>>> Signed-off-by: Ophir Munk >>>>>>> --- >>>>>>> lib/librte_ethdev/rte_flow.h | 9 +++++++-- >>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>>>>>=20 >>>>>>> diff --git a/lib/librte_ethdev/rte_flow.h >>>>>>> b/lib/librte_ethdev/rte_flow.h index c0fe879..ca9e135 100644 >>>>>>> --- a/lib/librte_ethdev/rte_flow.h >>>>>>> +++ b/lib/librte_ethdev/rte_flow.h >>>>>>> @@ -1782,10 +1782,15 @@ struct rte_flow_action_rss { >>>>>>> * through. >>>>>>> */ >>>>>>> uint32_t level; >>>>>>> - uint64_t types; /**< Specific RSS hash types (see >> ETH_RSS_*). */ >>>>>>> + /** >>>>>>> + * Specific RSS hash types (see ETH_RSS_*), >>>>>>> + * or 0 for PMD specific default. >>>>>>> + */ >>>>>>> + uint64_t types; >>>>>>> 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. */ >>>>>>> + /** Hash key, or NULL for PMD specific default key. */ >>>>>>> + const uint8_t *key; >>>>>>=20 >>>>>> I'd suggest to document that on key_len instead. If key_len is >>>>>> nonzero, key cannot be NULL anyway. >>>>>=20 >>>>> The decision if a key/len combination is valid is done in the PMD >>>>> action >>>> validation API. >>>>> For example, in MLX5 key=3D=3DNULL and key_len=3D=3D40 is accepted. >>>>> The combination key=3D=3DNULL and key_len=3D=3D0 should always succee= ds, >>>> however the "must" parameter for RSS default is key=3D=3DNULL and not >>>> key_len=3D=3D0. >>>>=20 >>>> I understand this is how the mlx5 PMD implemented it, but my point >>>> is that it makes more sense API-wise to define key_len =3D=3D 0 as the >>>> trigger for a default RSS hash key than key =3D=3D NULL. >>>>=20 >>>> My suggestion is to follow the same trend as memcpy(), mmap(), >>>> snprintf() and other well-known functions that take a size when >>>> dealing with NULL/undefined pointers. Only size matters! :) >>>>=20 >>>=20 >>> Please let's stay backward compatible and consistent with previous >>> dpdk releases where key=3D=3DNULL is used in struct rte_eth_rss_conf (s= ee >> code snippet in [1]). >>=20 >> And I thought I wouldn't hear again from that structure after ac8d22de23= 94 >> ("ethdev: flatten RSS configuration in flow API") got rid of it in rte_f= low >> :) >>=20 >> There is no need to be backward compatible with it particularly if doing= so >> improves consistency (assuming you agree it does). >>=20 >> Take "rss_hf" versus "types" fields for instance [2]. Unsetting the form= er >> disables RSS completely while unsetting the latter provides default RSS >> settings for that flow rule, simply because a RSS action that doesn't do= RSS >> makes no sense (one could provide a single queue for that). >>=20 >> Regarding "key_len", have a look at this other message [3] in the same >> thread which clearly states that i40e expects a zero key length to trigg= er >> default RSS, it doesn't rely on a NULL pointer. >>=20 >> Generally speaking, I'm pushing for zero values in action objects to res= ult in a >> safe default behavior. A nonzero key_len with a NULL key may result in a >> crash, while the reverse is inherently safer since one doesn't even need= to >> check the size or pointer validity, e.g.: >>=20 >> memcpy(target_device_rss_stuff, rss_conf->key, rss_conf->key_len); >>=20 >> What's not to like? >>=20 >=20 > Some think key=3D=3DNULL && key_len!=3D0 is senseless while other think k= ey!=3DNULL && key_len=3D=3D0 is senseless. > Let's agree on both key=3D=3DNULL && key_len=3D=3D0. A bug's already been found by our QA team. The following command crashes. flow create 0 ingress pattern eth / ipv6 / udp / end actions rss queues 0= end level 2 types udp end key_len 40 / end due to null pointer access in the following code: lib/librte_ethdev/rte_flow.c @@ -464,7 +464,7 @@ rte_flow_conv_action_conf(void *buf, const size_t size, }), size > sizeof(*dst.rss) ? sizeof(*dst.rss) : siz= e); off =3D sizeof(*dst.rss); if (src.rss->key_len) { off =3D RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key)); tmp =3D sizeof(*src.rss->key) * src.rss->key_len; if (size >=3D off + tmp) dst.rss->key =3D rte_memcpy ((void *)((uintptr_t)dst.rss + off)= , src.rss->key, tmp); off +=3D tmp; } I wanted to submit a patch to add a sanity check, - if (src.rss->key_len) { + if (src.rss->key && src.rss->key_len) { but looks like we should conclude this thread first? Or, does the fix make any sense regardless of having key_len=3D0 or key=3Dn= ull for default key? Having more sanity check is no harm usually... Thanks, Yongseok >=20 >> [2] "Re: [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API" >>=20 >> https://emea01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fmai >> ls.dpdk.org%2Farchives%2Fdev%2F2018- >> April%2F096235.html&data=3D02%7C01%7Cophirmu%40mellanox.com% >> 7C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d14925 >> 6f461b%7C0%7C0%7C636772057013784575&sdata=3D6JsomMO68lJoiNd >> ehRaNdn2xG9BSAgqCjxwpJJJOP8A%3D&reserved=3D0 >>=20 >> [3] "Re: [dpdk-dev] [PATCH v6 07/16] ethdev: flatten RSS configuration i= n >> flow API" >>=20 >> https://emea01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fmai >> ls.dpdk.org%2Farchives%2Fdev%2F2018- >> May%2F100128.html&data=3D02%7C01%7Cophirmu%40mellanox.com%7 >> C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d149256 >> f461b%7C0%7C0%7C636772057013784575&sdata=3DTXDTKLho8qyKlH%2 >> FqBturGHWhvdfaACuMoqvl0eOjfcU%3D&reserved=3D0 >>=20 >>> We should avoid confusing applications with setting key=3D=3DNULL with >> legacy RSS and key_len=3D=3D0 with rte_flows. >>>=20 >>> With regard to trends APIs - I was thinking about free() where a NULL >>> pointer is acceptable :) >>=20 >> Yeah, though free() doesn't take a size. On the other hand the behavior = of >> realloc() is much more interesting when faced with a zero size :) That's >> probably one of the few exceptions so I guess my point still stands. >>=20 >>>=20 >>> [1] >>> File lib/librte_ethdev/rte_ethdev.h: >>>=20 >>> /** >>> * A structure used to configure the Receive Side Scaling (RSS) >>> feature >>> * of an Ethernet port. >>> * If not NULL, the *rss_key* pointer of the *rss_conf* structure >>> points >>> * to an array holding the RSS key to use for hashing specific header >>> * fields of received packets. The length of this array should be >>> indicated >>> * by *rss_key_len* below. Otherwise, a default random hash key is >>> used by >>> * the device driver. >>> * >>> * The *rss_key_len* field of the *rss_conf* structure indicates the >>> length >>> * in bytes of the array pointed by *rss_key*. To be compatible, this >>> length >>> * will be checked in i40e only. Others assume 40 bytes to be used as >> before. >>> * >>> * .......... >>> */ >>> struct rte_eth_rss_conf { >>> uint8_t *rss_key; /**< If not NULL, 40-byte hash key. */ >>> uint8_t rss_key_len; /**< hash key length in bytes. */ >>> uint64_t rss_hf; /**< Hash functions to apply - see below. */ >>> }; >>=20 >> -- >> Adrien Mazarguil >> 6WIND