From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 354F3201 for ; Thu, 8 Nov 2018 07:33:08 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 8F4239C0059; Thu, 8 Nov 2018 06:33:06 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Thu, 8 Nov 2018 06:32:59 +0000 To: Adrien Mazarguil , Ophir Munk CC: Ferruh Yigit , "dev@dpdk.org" , Thomas Monjalon , Asaf Penso , Shahaf Shuler , Olga Shern 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> From: Andrew Rybchenko Message-ID: Date: Thu, 8 Nov 2018 09:32:55 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20181107164118.GM4638@6wind.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24210.003 X-TM-AS-Result: No-40.692600-8.000000-10 X-TMASE-MatchedRID: CxmI61mtwh8OwH4pD14DsPHkpkyUphL97k+C9WYaWv8iFs20Vxq/wnZE Dt3CljaDj65lZwStukNRfJLIyxbVcQ7WYEeNmTZnsyw+ZJnFumRG/JUd7BvSQmd6vNuG6Cqy+/F OCNPU6HmAtLwzDtz37BMOGeiAFYuCxEEtnnH5KRcLPVZHwod7gGf6wD367Vgtw01zN1c0miJDJi enRpkiMsJchn5RV4mr60JQlkdiEaFFhnuoIz0wobdQIb8hCnY+N/BTU5ZfZRKTMTaQzhvoeoVj9 z8S2T4dIr+DGVJo+Utv9iB0ayGvniC92D94UBz+Epbidxuv63OsKrCNemuWXhS11FlOYRoh7nk3 H8Ig1meA7e3G96La8uZGFB8ObxvJoayr11+eKrkXKqR+w9a7UECrr/LkAQ46UgSB7/aVPwDCLIt v4B1yR1rhjlZC171p8D58O9NhiXUTp1RhAuRgyoxVBvj1jbcj+IfriO3cV8Rb8pv4L0h+ItzRLT WypO1xs8tZrwRBjQXdShOiSVWedAMWCgdCzp+aE0Q83A2vD+sK3n1SHen81fWorfObDYJAa8n1V KhiQ3/KEio955kBs1QyCdfGvKVzPD57pIba+wGwHK2BMXhNNLqGBW9J0YqjbaZMa0fgRtbPC/7R IQaUoDa+tbo9aS+q6SP72+b+xIblW1X3S7HG17MjW/sniEQKf6/Md8Lb2l83Z3efQH+wj2sBAc2 1K1EY/pUg0ca/WV+0NItx1A9XzDblc6Gei4nlIj0zFI5DoJJeCrB32KOS0KMLUT/MIQivCvE68u 8U9k8R4d+Uvhl0qv1VprTjLOsR3lmiQx+Vqg2eAiCmPx4NwLTrdaH1ZWqC1B0Hk1Q1KyIc4WL5J jd88LkblkrgCLv44kYXbobxJbKl/MtrTwS4UHOw0TLHV6Q7uZYcOxT2io7oMu9MJDCtXjN+YY9x Av2HgnnGNRRa0Uk= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--40.692600-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24210.003 X-MDID: 1541658787-F38mFxlNMIkc Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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 06:33:08 -0000 On 11/7/18 7:41 PM, Adrien Mazarguil wrote: > On Wed, Nov 07, 2018 at 03:13:07PM +0000, Ophir Munk wrote: >>> -----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 >>> >>> 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 >>>>> >>>>> 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. >>>>>> >>>>>> 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. >>>>>> >>>>>> Signed-off-by: Ophir Munk >>>>>> --- >>>>>> lib/librte_ethdev/rte_flow.h | 9 +++++++-- >>>>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>>>> >>>>>> 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; >>>>> I'd suggest to document that on key_len instead. If key_len is >>>>> nonzero, key cannot be NULL anyway. >>>> The decision if a key/len combination is valid is done in the PMD action >>> validation API. >>>> For example, in MLX5 key==NULL and key_len==40 is accepted. >>>> The combination key==NULL and key_len==0 should always succeeds, >>> however the "must" parameter for RSS default is key==NULL and not >>> key_len==0. >>> >>> 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 == 0 as the trigger for a default >>> RSS hash key than key == NULL. >>> >>> 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! :) >>> >> Please let's stay backward compatible and consistent with previous dpdk releases where >> key==NULL is used in struct rte_eth_rss_conf (see code snippet in [1]). > And I thought I wouldn't hear again from that structure after ac8d22de2394 > ("ethdev: flatten RSS configuration in flow API") got rid of it in rte_flow > :) > > There is no need to be backward compatible with it particularly if doing so > improves consistency (assuming you agree it does). +1 > Take "rss_hf" versus "types" fields for instance [2]. Unsetting the former > 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). > > 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 trigger > default RSS, it doesn't rely on a NULL pointer. > > Generally speaking, I'm pushing for zero values in action objects to result > 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.: > > memcpy(target_device_rss_stuff, rss_conf->key, rss_conf->key_len); +1 > What's not to like? > > [2] "Re: [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API" > https://mails.dpdk.org/archives/dev/2018-April/096235.html > > [3] "Re: [dpdk-dev] [PATCH v6 07/16] ethdev: flatten RSS configuration in > flow API" > https://mails.dpdk.org/archives/dev/2018-May/100128.html > >> We should avoid confusing applications with setting key==NULL with legacy RSS and key_len==0 with rte_flows. >> >> With regard to trends APIs - I was thinking about free() where a NULL pointer is acceptable :) > 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. > >> [1] >> File lib/librte_ethdev/rte_ethdev.h: >> >> /** >> * 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. */ >> };