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.mazarg...@6wind.com]
Sent: Wednesday, November 07, 2018 4:06 PM
To: Ophir Munk <ophi...@mellanox.com>
Cc: Ferruh Yigit <ferruh.yi...@intel.com>; Andrew Rybchenko
<arybche...@solarflare.com>; dev@dpdk.org; Thomas Monjalon
<tho...@monjalon.net>; Asaf Penso <as...@mellanox.com>; Shahaf
Shuler <shah...@mellanox.com>; Olga Shern <ol...@mellanox.com>
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.mazarg...@6wind.com]
Sent: Wednesday, November 07, 2018 11:31 AM
To: Ophir Munk <ophi...@mellanox.com>
Cc: Ferruh Yigit <ferruh.yi...@intel.com>; Andrew Rybchenko
<arybche...@solarflare.com>; dev@dpdk.org; Thomas Monjalon
<tho...@monjalon.net>; Asaf Penso <as...@mellanox.com>; Shahaf
Shuler <shah...@mellanox.com>; Olga Shern <ol...@mellanox.com>
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 <ophi...@mellanox.com>
---
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. */
};
