[dpdk-dev] [PATCH v1] ethdev: document RSS default key and types

[dpdk-dev] [PATCH v1] ethdev: document RSS default key and types

[Ophir Munk]

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 <ophirmu@mellanox.com>
---
v1: Initial version

 lib/librte_ethdev/rte_flow.h | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index c0fe879..fb3e9a8 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -1782,13 +1782,29 @@ struct rte_flow_action_rss {
 	 * through.
 	 */
 	uint32_t level;
-	uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*). */
+	uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*) or 0 */
+	/**
+	 * Applications should supply specific RSS hash types (e.g.
+	 * ETH_RSS_IP). If the application does not care about the specific RSS
+	 * types it can set them to 0 and let any PMD use its default 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. */
+	const uint8_t *key; /**< Hash key (can be NULL). */
+	/**
+	 * The key is a pointer to hash bytes array.
+	 *
+	 * Applications should supply a specific RSS hash key.
+	 * If an application is only interested in default RSS operation it
+	 * should not care about the specific hash key. By setting the hash
+	 * key to NULL - any PMD will use its default RSS key.
+	 */
 	const uint16_t *queue; /**< Queue indices to use. */
 };
 
+
+
+
 /**
  * RTE_FLOW_ACTION_TYPE_VF
  *
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v1] ethdev: document RSS default key and types

[Ferruh Yigit]

On 11/3/2018 3:46 PM, 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 <ophirmu@mellanox.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v1] ethdev: document RSS default key and types

[Thomas Monjalon]

This patch is in next-net but may be improved before reaching mainline.
The comments may be shorter and avoid being split in 2 parts.
The second part of the doxygen comments, starting with /**,
will apply wrongly to the next field.


03/11/2018 16:46, Ophir Munk:
> 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 <ophirmu@mellanox.com>
> ---
> v1: Initial version
> 
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1782,13 +1782,29 @@ struct rte_flow_action_rss {
>  	 * through.
>  	 */
>  	uint32_t level;
> -	uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*). */
> +	uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*) or 0 */
> +	/**
> +	 * Applications should supply specific RSS hash types (e.g.
> +	 * ETH_RSS_IP). If the application does not care about the specific RSS
> +	 * types it can set them to 0 and let any PMD use its default 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. */
> +	const uint8_t *key; /**< Hash key (can be NULL). */
> +	/**
> +	 * The key is a pointer to hash bytes array.
> +	 *
> +	 * Applications should supply a specific RSS hash key.
> +	 * If an application is only interested in default RSS operation it
> +	 * should not care about the specific hash key. By setting the hash
> +	 * key to NULL - any PMD will use its default RSS key.
> +	 */
>  	const uint16_t *queue; /**< Queue indices to use. */
>  };
>  
> +
> +
> +
>  /**
>   * RTE_FLOW_ACTION_TYPE_VF
>   *

[dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Ophir Munk]

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 <ophirmu@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;
 	const uint16_t *queue; /**< Queue indices to use. */
 };
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Adrien Mazarguil]

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 <ophirmu@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.

>  	const uint16_t *queue; /**< Queue indices to use. */
>  };
>  
> -- 
> 1.8.3.1
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Ophir Munk]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, November 07, 2018 11:31 AM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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 <ophirmu@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.

> 
> >  	const uint16_t *queue; /**< Queue indices to use. */  };
> >
> > --
> > 1.8.3.1
> >
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Adrien Mazarguil]

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 <ophirmu@mellanox.com>
> > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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 <ophirmu@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! :)

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Ophir Munk]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, November 07, 2018 4:06 PM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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.mazarguil@6wind.com]
> > > Sent: Wednesday, November 07, 2018 11:31 AM
> > > To: Ophir Munk <ophirmu@mellanox.com>
> > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > > <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > > Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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 <ophirmu@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]).
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 :)

[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. */
};

> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Adrien Mazarguil]

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 <ophirmu@mellanox.com>
> > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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.mazarguil@6wind.com]
> > > > Sent: Wednesday, November 07, 2018 11:31 AM
> > > > To: Ophir Munk <ophirmu@mellanox.com>
> > > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > > > <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > > > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > > > Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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 <ophirmu@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).

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);

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. */
> };

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Andrew Rybchenko]

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 <ophirmu@mellanox.com>
>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
>>> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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.mazarguil@6wind.com]
>>>>> Sent: Wednesday, November 07, 2018 11:31 AM
>>>>> To: Ophir Munk <ophirmu@mellanox.com>
>>>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>>>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
>>>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
>>>>> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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 <ophirmu@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. */
>> };

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Ophir Munk]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, November 07, 2018 6:41 PM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> types
> 
> 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 <ophirmu@mellanox.com>
> > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > > <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > > Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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.mazarguil@6wind.com]
> > > > > Sent: Wednesday, November 07, 2018 11:31 AM
> > > > > To: Ophir Munk <ophirmu@mellanox.com>
> > > > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > > > > <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > > > > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>;
> Shahaf
> > > > > Shuler <shahafs@mellanox.com>; Olga Shern
> <olgas@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 <ophirmu@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).
> 
> 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);
> 
> What's not to like?
> 

Some think key==NULL && key_len!=0 is senseless while other think key!=NULL && key_len==0 is senseless.
Let's agree on both key==NULL && key_len==0.

> [2] "Re: [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API"
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmai
> ls.dpdk.org%2Farchives%2Fdev%2F2018-
> April%2F096235.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%
> 7C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d14925
> 6f461b%7C0%7C0%7C636772057013784575&amp;sdata=6JsomMO68lJoiNd
> ehRaNdn2xG9BSAgqCjxwpJJJOP8A%3D&amp;reserved=0
> 
> [3] "Re: [dpdk-dev] [PATCH v6 07/16] ethdev: flatten RSS configuration in
>     flow API"
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmai
> ls.dpdk.org%2Farchives%2Fdev%2F2018-
> May%2F100128.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%7
> C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d149256
> f461b%7C0%7C0%7C636772057013784575&amp;sdata=TXDTKLho8qyKlH%2
> FqBturGHWhvdfaACuMoqvl0eOjfcU%3D&amp;reserved=0
> 
> > 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. */
> > };
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Yongseok Koh]

> On Nov 8, 2018, at 12:50 AM, Ophir Munk <ophirmu@mellanox.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
>> Sent: Wednesday, November 07, 2018 6:41 PM
>> To: Ophir Munk <ophirmu@mellanox.com>
>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
>> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
>> types
>> 
>> 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 <ophirmu@mellanox.com>
>>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
>>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
>>>> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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.mazarguil@6wind.com]
>>>>>> Sent: Wednesday, November 07, 2018 11:31 AM
>>>>>> To: Ophir Munk <ophirmu@mellanox.com>
>>>>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>>>>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
>>>>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>;
>> Shahaf
>>>>>> Shuler <shahafs@mellanox.com>; Olga Shern
>> <olgas@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 <ophirmu@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).
>> 
>> 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);
>> 
>> What's not to like?
>> 
> 
> Some think key==NULL && key_len!=0 is senseless while other think key!=NULL && key_len==0 is senseless.
> Let's agree on both key==NULL && key_len==0.

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) : size);
                off = sizeof(*dst.rss);
                if (src.rss->key_len) {
                        off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
                        tmp = sizeof(*src.rss->key) * src.rss->key_len;
                        if (size >= off + tmp)
                                dst.rss->key = rte_memcpy
                                        ((void *)((uintptr_t)dst.rss + off),
                                         src.rss->key, tmp);
                        off += 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=0 or key=null for default key?
Having more sanity check is no harm usually...


Thanks,
Yongseok


> 
>> [2] "Re: [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API"
>> 
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmai
>> ls.dpdk.org%2Farchives%2Fdev%2F2018-
>> April%2F096235.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%
>> 7C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d14925
>> 6f461b%7C0%7C0%7C636772057013784575&amp;sdata=6JsomMO68lJoiNd
>> ehRaNdn2xG9BSAgqCjxwpJJJOP8A%3D&amp;reserved=0
>> 
>> [3] "Re: [dpdk-dev] [PATCH v6 07/16] ethdev: flatten RSS configuration in
>>    flow API"
>> 
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmai
>> ls.dpdk.org%2Farchives%2Fdev%2F2018-
>> May%2F100128.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%7
>> C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d149256
>> f461b%7C0%7C0%7C636772057013784575&amp;sdata=TXDTKLho8qyKlH%2
>> FqBturGHWhvdfaACuMoqvl0eOjfcU%3D&amp;reserved=0
>> 
>>> 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. */
>>> };
>> 
>> --
>> Adrien Mazarguil
>> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Ophir Munk]

> -----Original Message-----
> From: Yongseok Koh
> Sent: Friday, November 09, 2018 1:07 AM
> To: Ophir Munk <ophirmu@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Thomas Monjalon
> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> types
> 
> 
> > On Nov 8, 2018, at 12:50 AM, Ophir Munk <ophirmu@mellanox.com>
> wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> >> Sent: Wednesday, November 07, 2018 6:41 PM
> >> To: Ophir Munk <ophirmu@mellanox.com>
> >> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> >> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> >> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> Shuler
> >> <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> >> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key
> >> and types
> >>
> >> 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 <ophirmu@mellanox.com>
> >>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> >>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> >>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> >>>> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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.mazarguil@6wind.com]
> >>>>>> Sent: Wednesday, November 07, 2018 11:31 AM
> >>>>>> To: Ophir Munk <ophirmu@mellanox.com>
> >>>>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> >>>>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> >>>>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>;
> >> Shahaf
> >>>>>> Shuler <shahafs@mellanox.com>; Olga Shern
> >> <olgas@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 <ophirmu@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).
> >>
> >> 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);
> >>
> >> What's not to like?
> >>
> >
> > Some think key==NULL && key_len!=0 is senseless while other think
> key!=NULL && key_len==0 is senseless.
> > Let's agree on both key==NULL && key_len==0.
> 
> 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) : size);
>                 off = sizeof(*dst.rss);
>                 if (src.rss->key_len) {
>                         off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
>                         tmp = sizeof(*src.rss->key) * src.rss->key_len;
>                         if (size >= off + tmp)
>                                 dst.rss->key = rte_memcpy
>                                         ((void *)((uintptr_t)dst.rss + off),
>                                          src.rss->key, tmp);
>                         off += 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=0 or key=null
> for default key?
> Having more sanity check is no harm usually...
> 
> 
> Thanks,
> Yongseok
> 

The setfault is the result of commit a4391f8bae ("app/testpmd: set default RSS key as null").
Reverting this commit should fix the segfault but it also means there is no way to set default key (key=NULL) with testpmd.
Need to check if this is only a testpmd limitation and not all applications limitation.

We should decide how an application can set default RSS without knowing anything about keys.

> 
> >
> >> [2] "Re: [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API"
> >>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> >> i
> >> ls.dpdk.org%2Farchives%2Fdev%2F2018-
> >>
> April%2F096235.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%
> >>
> 7C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d14925
> >>
> 6f461b%7C0%7C0%7C636772057013784575&amp;sdata=6JsomMO68lJoiNd
> >> ehRaNdn2xG9BSAgqCjxwpJJJOP8A%3D&amp;reserved=0
> >>
> >> [3] "Re: [dpdk-dev] [PATCH v6 07/16] ethdev: flatten RSS configuration in
> >>    flow API"
> >>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> >> i
> >> ls.dpdk.org%2Farchives%2Fdev%2F2018-
> >>
> May%2F100128.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%7
> >>
> C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d149256
> >>
> f461b%7C0%7C0%7C636772057013784575&amp;sdata=TXDTKLho8qyKlH%2
> >> FqBturGHWhvdfaACuMoqvl0eOjfcU%3D&amp;reserved=0
> >>
> >>> 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. */
> >>> };
> >>
> >> --
> >> Adrien Mazarguil
> >> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Ori Kam]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ophir Munk
> Sent: Friday, November 9, 2018 10:14 AM
> To: Yongseok Koh <yskoh@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Thomas Monjalon
> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types
> 
> 
> 
> > -----Original Message-----
> > From: Yongseok Koh
> > Sent: Friday, November 09, 2018 1:07 AM
> > To: Ophir Munk <ophirmu@mellanox.com>; Adrien Mazarguil
> > <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>
> > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Thomas Monjalon
> > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > Shuler <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> > types
> >
> >
> > > On Nov 8, 2018, at 12:50 AM, Ophir Munk <ophirmu@mellanox.com>
> > wrote:
> > >
> > >
> > >
> > >> -----Original Message-----
> > >> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > >> Sent: Wednesday, November 07, 2018 6:41 PM
> > >> To: Ophir Munk <ophirmu@mellanox.com>
> > >> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > >> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > >> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > Shuler
> > >> <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> > >> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key
> > >> and types
> > >>
> > >> 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 <ophirmu@mellanox.com>
> > >>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > >>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > >>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > >>>> Shuler <shahafs@mellanox.com>; Olga Shern <olgas@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.mazarguil@6wind.com]
> > >>>>>> Sent: Wednesday, November 07, 2018 11:31 AM
> > >>>>>> To: Ophir Munk <ophirmu@mellanox.com>
> > >>>>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> > >>>>>> <arybchenko@solarflare.com>; dev@dpdk.org; Thomas Monjalon
> > >>>>>> <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>;
> > >> Shahaf
> > >>>>>> Shuler <shahafs@mellanox.com>; Olga Shern
> > >> <olgas@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 <ophirmu@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).
> > >>
> > >> 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);
> > >>
> > >> What's not to like?
> > >>
> > >
> > > Some think key==NULL && key_len!=0 is senseless while other think
> > key!=NULL && key_len==0 is senseless.
> > > Let's agree on both key==NULL && key_len==0.
> >
> > 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) : size);
> >                 off = sizeof(*dst.rss);
> >                 if (src.rss->key_len) {
> >                         off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
> >                         tmp = sizeof(*src.rss->key) * src.rss->key_len;
> >                         if (size >= off + tmp)
> >                                 dst.rss->key = rte_memcpy
> >                                         ((void *)((uintptr_t)dst.rss + off),
> >                                          src.rss->key, tmp);
> >                         off += 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=0 or key=null
> > for default key?
> > Having more sanity check is no harm usually...
> >
> >
> > Thanks,
> > Yongseok
> >
> 
> The setfault is the result of commit a4391f8bae ("app/testpmd: set default RSS
> key as null").
> Reverting this commit should fix the segfault but it also means there is no way
> to set default key (key=NULL) with testpmd.
> Need to check if this is only a testpmd limitation and not all applications
> limitation.
> 
> We should decide how an application can set default RSS without knowing
> anything about keys.
> 

I agree with Adrian that the main criteria should be the length.
Maybe the set default RSS in testpmd should get new parameter.

> >
> > >
> > >> [2] "Re: [PATCH v2 07/15] ethdev: flatten RSS configuration in flow API"
> > >>
> > >>
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> > >> i
> > >> ls.dpdk.org%2Farchives%2Fdev%2F2018-
> > >>
> > April%2F096235.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%
> > >>
> > 7C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d14925
> > >>
> > 6f461b%7C0%7C0%7C636772057013784575&amp;sdata=6JsomMO68lJoiNd
> > >> ehRaNdn2xG9BSAgqCjxwpJJJOP8A%3D&amp;reserved=0
> > >>
> > >> [3] "Re: [dpdk-dev] [PATCH v6 07/16] ethdev: flatten RSS configuration in
> > >>    flow API"
> > >>
> > >>
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> > >> i
> > >> ls.dpdk.org%2Farchives%2Fdev%2F2018-
> > >>
> > May%2F100128.html&amp;data=02%7C01%7Cophirmu%40mellanox.com%7
> > >>
> > C4fdd2e5a838047c152c408d644cfe439%7Ca652971c7d2e4d9ba6a4d149256
> > >>
> > f461b%7C0%7C0%7C636772057013784575&amp;sdata=TXDTKLho8qyKlH%2
> > >> FqBturGHWhvdfaACuMoqvl0eOjfcU%3D&amp;reserved=0
> > >>
> > >>> 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. */
> > >>> };
> > >>
> > >> --
> > >> Adrien Mazarguil
> > >> 6WIND

Best,
Ori

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Adrien Mazarguil]

Again a bit late to the party, please see below.

On Sun, Nov 11, 2018 at 09:35:22AM +0000, Ori Kam wrote:
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Ophir Munk
> > Sent: Friday, November 9, 2018 10:14 AM
> > To: Yongseok Koh <yskoh@mellanox.com>; Adrien Mazarguil
> > <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>
> > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Thomas Monjalon
> > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types
> > 
> > > -----Original Message-----
> > > From: Yongseok Koh
> > > Sent: Friday, November 09, 2018 1:07 AM
> > > To: Ophir Munk <ophirmu@mellanox.com>; Adrien Mazarguil
> > > <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> > > <arybchenko@solarflare.com>
> > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Thomas Monjalon
> > > <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>; Shahaf
> > > Shuler <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> > > types
> > >
<snip>
> > >
> > > -               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=0 or key=null
> > > for default key?
> > > Having more sanity check is no harm usually...
> > >
> > >
> > > Thanks,
> > > Yongseok
> > >
> > 
> > The setfault is the result of commit a4391f8bae ("app/testpmd: set default RSS
> > key as null").
> > Reverting this commit should fix the segfault but it also means there is no way
> > to set default key (key=NULL) with testpmd.
> > Need to check if this is only a testpmd limitation and not all applications
> > limitation.
> > 
> > We should decide how an application can set default RSS without knowing
> > anything about keys.
> > 
> 
> I agree with Adrian that the main criteria should be the length.
> Maybe the set default RSS in testpmd should get new parameter.

Since [1] was reverted and we seem to agree that a zero key_len should
trigger a PMD-specific default key, this can already be requested with
testpmd by overriding key_len, e.g.:

 flow create 1 pattern eth / end actions rss key_len 0 / end 

Using an empty string as the key would yield the same result but cannot be
expressed on the command line yet. Note that specifying a key automatically
overrides key_len, so key_len must be forced to 0 last to get PMD defaults:

 flow create 1 pattern eth / end actions rss key foo key_len 0 / end

Here key_len is set to testpmd's default size when parsing "rss", updated to
3 when parsing "key foo" and updated once again when parsing "key_len 0".

Lastly, while it would make sense for testpmd to use 0 as the default value,
doing so yields inconsistent balancing results between vendors/devices as
they all come with a different key. Same reason as initializing the RSS
types field to the global rss_hf instead of 0.

[1] "app/testpmd: revert setting default RSS"
    https://mails.dpdk.org/archives/dev/2018-November/118786.html

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Ophir Munk]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, November 13, 2018 7:15 PM
> To: Ori Kam <orika@mellanox.com>
> Cc: Ophir Munk <ophirmu@mellanox.com>; Yongseok Koh
> <yskoh@mellanox.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Asaf Penso
> <asafp@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Olga
> Shern <olgas@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> types
> 
> Again a bit late to the party, please see below.
> 
> On Sun, Nov 11, 2018 at 09:35:22AM +0000, Ori Kam wrote:
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Ophir Munk
> > > Sent: Friday, November 9, 2018 10:14 AM
> > > To: Yongseok Koh <yskoh@mellanox.com>; Adrien Mazarguil
> > > <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> > > <arybchenko@solarflare.com>
> > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Thomas
> > > Monjalon <thomas@monjalon.net>; Asaf Penso <asafp@mellanox.com>;
> > > Shahaf Shuler <shahafs@mellanox.com>; Olga Shern
> > > <olgas@mellanox.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key
> > > and types
> > >
> > > > -----Original Message-----
> > > > From: Yongseok Koh
> > > > Sent: Friday, November 09, 2018 1:07 AM
> > > > To: Ophir Munk <ophirmu@mellanox.com>; Adrien Mazarguil
> > > > <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> > > > <arybchenko@solarflare.com>
> > > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Thomas
> > > > Monjalon <thomas@monjalon.net>; Asaf Penso
> <asafp@mellanox.com>;
> > > > Shahaf Shuler <shahafs@mellanox.com>; Olga Shern
> > > > <olgas@mellanox.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default
> > > > key and types
> > > >
> <snip>
> > > >
> > > > -               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=0 or
> > > > key=null for default key?
> > > > Having more sanity check is no harm usually...
> > > >
> > > >
> > > > Thanks,
> > > > Yongseok
> > > >
> > >
> > > The setfault is the result of commit a4391f8bae ("app/testpmd: set
> > > default RSS key as null").
> > > Reverting this commit should fix the segfault but it also means
> > > there is no way to set default key (key=NULL) with testpmd.
> > > Need to check if this is only a testpmd limitation and not all
> > > applications limitation.
> > >
> > > We should decide how an application can set default RSS without
> > > knowing anything about keys.
> > >
> >
> > I agree with Adrian that the main criteria should be the length.
> > Maybe the set default RSS in testpmd should get new parameter.
> 
> Since [1] was reverted and we seem to agree that a zero key_len should
> trigger a PMD-specific default key, this can already be requested with
> testpmd by overriding key_len, e.g.:
> 
>  flow create 1 pattern eth / end actions rss key_len 0 / end
> 

Not all agree that zero key_len should trigger default key :)
The suggestion is to require applications to set both key=NULL and key_len==0.
Individual PMDs will have the option to base default RSS on key_len, key or both.

> Using an empty string as the key would yield the same result but cannot be
> expressed on the command line yet. Note that specifying a key automatically
> overrides key_len, so key_len must be forced to 0 last to get PMD defaults:
> 
>  flow create 1 pattern eth / end actions rss key foo key_len 0 / end
> 
> Here key_len is set to testpmd's default size when parsing "rss", updated to
> 3 when parsing "key foo" and updated once again when parsing "key_len 0".
> 
> Lastly, while it would make sense for testpmd to use 0 as the default value,
> doing so yields inconsistent balancing results between vendors/devices as
> they all come with a different key. Same reason as initializing the RSS types
> field to the global rss_hf instead of 0.
> 
> [1] "app/testpmd: revert setting default RSS"
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmai
> ls.dpdk.org%2Farchives%2Fdev%2F2018-
> November%2F118786.html&amp;data=02%7C01%7Cophirmu%40mellanox.c
> om%7Ce765a4cb2c8b4bb1547808d6498ba2a5%7Ca652971c7d2e4d9ba6a4d
> 149256f461b%7C0%7C0%7C636777261426945159&amp;sdata=k1NXLzaQeK
> fp3yptZQsJd6w0NA%2FUVRb8rctLdkDPIv8%3D&amp;reserved=0
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Shahaf Shuler]

Hi Adrien, 

Tuesday, November 13, 2018 7:15 PM, Adrien Mazarguil:
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> types
> 
> Again a bit late to the party, please see below.
> 
> On Sun, Nov 11, 2018 at 09:35:22AM +0000, Ori Kam wrote:

[...]

> > > The setfault is the result of commit a4391f8bae ("app/testpmd: set
> > > default RSS key as null").
> > > Reverting this commit should fix the segfault but it also means
> > > there is no way to set default key (key=NULL) with testpmd.
> > > Need to check if this is only a testpmd limitation and not all
> > > applications limitation.
> > >
> > > We should decide how an application can set default RSS without
> > > knowing anything about keys.
> > >
> >
> > I agree with Adrian that the main criteria should be the length.
> > Maybe the set default RSS in testpmd should get new parameter.
> 
> Since [1] was reverted and we seem to agree that a zero key_len should
> trigger a PMD-specific default key, this can already be requested with
> testpmd by overriding key_len, e.g.:
> 
>  flow create 1 pattern eth / end actions rss key_len 0 / end
> 
> Using an empty string as the key would yield the same result but cannot be
> expressed on the command line yet. Note that specifying a key automatically
> overrides key_len, so key_len must be forced to 0 last to get PMD defaults:
> 
>  flow create 1 pattern eth / end actions rss key foo key_len 0 / end

I don't understand why we are backing up API claims with "how testpmd is implemented". The APIs should be correct, regardless of how testpmd is using them. 

To this doc issue, 
I don't understand on what cases it makes sense for application to have rss_key_len = 0 while rss_key != NULL. This is obviously in-consist input, and of course all PMD will just ignore the key.
I think enforcing rss_key and rss_key_len to be NULL is a fair requirement from application, and it makes no confusion in the API inputs.

> 
> Here key_len is set to testpmd's default size when parsing "rss", updated to
> 3 when parsing "key foo" and updated once again when parsing "key_len 0".
> 
> Lastly, while it would make sense for testpmd to use 0 as the default value,
> doing so yields inconsistent balancing results between vendors/devices as
> they all come with a different key. Same reason as initializing the RSS types
> field to the global rss_hf instead of 0.



> 
> [1] "app/testpmd: revert setting default RSS"
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> ils.dpdk.org%2Farchives%2Fdev%2F2018-
> November%2F118786.html&data=02%7C01%7Cshahafs%40mellanox.co
> m%7C0eecf3e9af4b4b6bc53108d6498ba2a8%7Ca652971c7d2e4d9ba6a4d1492
> 56f461b%7C0%7C0%7C636777261425388073&sdata=Hu0iGr2xS%2FI%2FI
> s5PtzCylMMft5w5TBmtd3GYppEKKcA%3D&reserved=0
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Adrien Mazarguil]

Hi Shahaf,

On Tue, Nov 13, 2018 at 06:39:04PM +0000, Shahaf Shuler wrote:
> Hi Adrien, 
> 
> Tuesday, November 13, 2018 7:15 PM, Adrien Mazarguil:
> > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> > types
> > 
> > Again a bit late to the party, please see below.
> > 
> > On Sun, Nov 11, 2018 at 09:35:22AM +0000, Ori Kam wrote:
> 
> [...]
> 
> > > > The setfault is the result of commit a4391f8bae ("app/testpmd: set
> > > > default RSS key as null").
> > > > Reverting this commit should fix the segfault but it also means
> > > > there is no way to set default key (key=NULL) with testpmd.
> > > > Need to check if this is only a testpmd limitation and not all
> > > > applications limitation.
> > > >
> > > > We should decide how an application can set default RSS without
> > > > knowing anything about keys.
> > > >
> > >
> > > I agree with Adrian that the main criteria should be the length.
> > > Maybe the set default RSS in testpmd should get new parameter.
> > 
> > Since [1] was reverted and we seem to agree that a zero key_len should
> > trigger a PMD-specific default key, this can already be requested with
> > testpmd by overriding key_len, e.g.:
> > 
> >  flow create 1 pattern eth / end actions rss key_len 0 / end
> > 
> > Using an empty string as the key would yield the same result but cannot be
> > expressed on the command line yet. Note that specifying a key automatically
> > overrides key_len, so key_len must be forced to 0 last to get PMD defaults:
> > 
> >  flow create 1 pattern eth / end actions rss key foo key_len 0 / end
> 
> I don't understand why we are backing up API claims with "how testpmd is implemented". The APIs should be correct, regardless of how testpmd is using them. 

This wasn't the intent, I mean, currently one cannot input something like
that to get a zero key length:

 flow create 1 pattern eth / end actions rss key "" / end

Because "" is interpreted literally. So the only way to request a zero key
length is by explicitly setting it through "key_len 0".

The API remains clear: a zero key length requests default behavior from the
PMD regardless of the key pointer, which doesn't *have* to be NULL, merely
undefined. Testpmd does exactly that.

> To this doc issue, 
> I don't understand on what cases it makes sense for application to have rss_key_len = 0 while rss_key != NULL. This is obviously in-consist input, and of course all PMD will just ignore the key.
> I think enforcing rss_key and rss_key_len to be NULL is a fair requirement from application, and it makes no confusion in the API inputs.

Then you need to define what happens when key_len != 0 and key == NULL, also
when key_len == 0 and key != NULL, none of which make sense currently.

There's no reason for the PMD to even look at the key pointer if key_len is
0. Only if nonzero, it *can* check for its validity however there's no
reason to, it's a programming error in the application if not the case.
assert() is more appropriate for such situations.

I agree there's a lack of documentation which must be addressed, my point is
that key_len is the only guarantee a PMD needs from the application.

> > Here key_len is set to testpmd's default size when parsing "rss", updated to
> > 3 when parsing "key foo" and updated once again when parsing "key_len 0".
> > 
> > Lastly, while it would make sense for testpmd to use 0 as the default value,
> > doing so yields inconsistent balancing results between vendors/devices as
> > they all come with a different key. Same reason as initializing the RSS types
> > field to the global rss_hf instead of 0.
> 
> 
> 
> > 
> > [1] "app/testpmd: revert setting default RSS"
> > 
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> > ils.dpdk.org%2Farchives%2Fdev%2F2018-
> > November%2F118786.html&data=02%7C01%7Cshahafs%40mellanox.co
> > m%7C0eecf3e9af4b4b6bc53108d6498ba2a8%7Ca652971c7d2e4d9ba6a4d1492
> > 56f461b%7C0%7C0%7C636777261425388073&sdata=Hu0iGr2xS%2FI%2FI
> > s5PtzCylMMft5w5TBmtd3GYppEKKcA%3D&reserved=0

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Shahaf Shuler]

Wednesday, November 14, 2018 11:41 AM, Adrien Mazarguil:
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> types
> 
> Hi Shahaf,
> 
> On Tue, Nov 13, 2018 at 06:39:04PM +0000, Shahaf Shuler wrote:
> > Hi Adrien,
> >
> > Tuesday, November 13, 2018 7:15 PM, Adrien Mazarguil:
> > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key
> > > and types
> > >
> > > Again a bit late to the party, please see below.
> > >
> > > On Sun, Nov 11, 2018 at 09:35:22AM +0000, Ori Kam wrote:
> >
> > [...]
> >
> > > > > The setfault is the result of commit a4391f8bae ("app/testpmd:
> > > > > set default RSS key as null").
> > > > > Reverting this commit should fix the segfault but it also means
> > > > > there is no way to set default key (key=NULL) with testpmd.
> > > > > Need to check if this is only a testpmd limitation and not all
> > > > > applications limitation.
> > > > >
> > > > > We should decide how an application can set default RSS without
> > > > > knowing anything about keys.
> > > > >
> > > >
> > > > I agree with Adrian that the main criteria should be the length.
> > > > Maybe the set default RSS in testpmd should get new parameter.
> > >
> > > Since [1] was reverted and we seem to agree that a zero key_len
> > > should trigger a PMD-specific default key, this can already be
> > > requested with testpmd by overriding key_len, e.g.:
> > >
> > >  flow create 1 pattern eth / end actions rss key_len 0 / end
> > >
> > > Using an empty string as the key would yield the same result but
> > > cannot be expressed on the command line yet. Note that specifying a
> > > key automatically overrides key_len, so key_len must be forced to 0 last
> to get PMD defaults:
> > >
> > >  flow create 1 pattern eth / end actions rss key foo key_len 0 / end
> >
> > I don't understand why we are backing up API claims with "how testpmd is
> implemented". The APIs should be correct, regardless of how testpmd is
> using them.
> 
> This wasn't the intent, I mean, currently one cannot input something like that
> to get a zero key length:
> 
>  flow create 1 pattern eth / end actions rss key "" / end
> 
> Because "" is interpreted literally. So the only way to request a zero key
> length is by explicitly setting it through "key_len 0".
> 
> The API remains clear: a zero key length requests default behavior from the
> PMD regardless of the key pointer, which doesn't *have* to be NULL, merely
> undefined. Testpmd does exactly that.

IMO, it will make it more clear if the key will *have* to be null, because there is no single good reason to have it otherwise. 

However it looks like an endless debate between strict and relaxed API. there are points to both sides, yet we are failing to converge.
Mlx5 already implements according to the rss_key_len and rss_key approach. What are other PMD doing?

Assuming there is a consensus among the PMDs, maybe we can follow it in order to avoid the extra work.
is it that critical for you to enforce only the key_len w/o the rss_key? 

> 
> > To this doc issue,
> > I don't understand on what cases it makes sense for application to have
> rss_key_len = 0 while rss_key != NULL. This is obviously in-consist input, and
> of course all PMD will just ignore the key.
> > I think enforcing rss_key and rss_key_len to be NULL is a fair requirement
> from application, and it makes no confusion in the API inputs.
> 
> Then you need to define what happens when key_len != 0 and key == NULL,
> also when key_len == 0 and key != NULL, none of which make sense
> currently.

Of course all are in-consist and the PMD is free to reject such rules. This is not related though to how we define the default RSS right? 

> 
> There's no reason for the PMD to even look at the key pointer if key_len is 0.
> Only if nonzero, it *can* check for its validity however there's no reason to,
> it's a programming error in the application if not the case.
> assert() is more appropriate for such situations.
> 
> I agree there's a lack of documentation which must be addressed, my point is
> that key_len is the only guarantee a PMD needs from the application.
> 
> > > Here key_len is set to testpmd's default size when parsing "rss",
> > > updated to
> > > 3 when parsing "key foo" and updated once again when parsing "key_len
> 0".
> > >
> > > Lastly, while it would make sense for testpmd to use 0 as the
> > > default value, doing so yields inconsistent balancing results
> > > between vendors/devices as they all come with a different key. Same
> > > reason as initializing the RSS types field to the global rss_hf instead of 0.
> >
> >
> >
> > >
> > > [1] "app/testpmd: revert setting default RSS"
> > >
> > >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fm
> > > a
> > > ils.dpdk.org%2Farchives%2Fdev%2F2018-
> > >
> November%2F118786.html&data=02%7C01%7Cshahafs%40mellanox.co
> > >
> m%7C0eecf3e9af4b4b6bc53108d6498ba2a8%7Ca652971c7d2e4d9ba6a4d1492
> > >
> 56f461b%7C0%7C0%7C636777261425388073&sdata=Hu0iGr2xS%2FI%2FI
> > > s5PtzCylMMft5w5TBmtd3GYppEKKcA%3D&reserved=0
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Adrien Mazarguil]

On Wed, Nov 14, 2018 at 01:51:19PM +0000, Shahaf Shuler wrote:
<snip>
> IMO, it will make it more clear if the key will *have* to be null, because there is no single good reason to have it otherwise. 
> 
> However it looks like an endless debate between strict and relaxed API. there are points to both sides, yet we are failing to converge.
> Mlx5 already implements according to the rss_key_len and rss_key approach. What are other PMD doing?
> 
> Assuming there is a consensus among the PMDs, maybe we can follow it in order to avoid the extra work.
> is it that critical for you to enforce only the key_len w/o the rss_key? 

Not at all, I don't mind extra checks in PMDs actually.

To be clear, here's a list of what I consider valid PMD checks:

- if (!key_len) use_default_key();

- if (key_len) { assert(key); use_app_key(); }

- if (key_len) { if (!key) complain(); else use_app_key(); }

- if (key_len) { if (!key) { complain(); use_default_key(); } else use_app_key(); }

- if (key && key_len) use_app_key(); else use_default_key(); /* it's OK
  since the alternative is a crash */

- if (!key_len || !key) use_default_key(); /* ditto */

While those are invalid:

- if (!key_len && !key) use_default_key(); /* err, else what? */

- if (!key_len) { assert(!key); use_default_key(); } /* unless you hate
  users */

- if (!key_len) { if (key) complain(); use_default_key(); } /* extra noise
  can be annoying */

What I'm most concerned with is rte_flow API documentation, that is, what we
ask users to do in order to achieve something. A default behavior for a zero
value is currently documented on "level" and "types" fields. "key_len" and
"queue_num" are currently lacking this information.

Just like "key", requesting RSS without providing a list of queues should
default to all configured Rx queues for convenience. In that case the
"queue" pointer can be undefined, not necessarily NULL because the PMD won't
look for queues if that list anyway, its value doesn't matter.

So documentation will describe that if "key_len" or "queue_num" are nonzero,
non-default behavior is requested and the related "key"/"queue" fields must
be set and valid. Otherwise they can remain undefined.

> > > To this doc issue,
> > > I don't understand on what cases it makes sense for application to have
> > rss_key_len = 0 while rss_key != NULL. This is obviously in-consist input, and
> > of course all PMD will just ignore the key.
> > > I think enforcing rss_key and rss_key_len to be NULL is a fair requirement
> > from application, and it makes no confusion in the API inputs.
> > 
> > Then you need to define what happens when key_len != 0 and key == NULL,
> > also when key_len == 0 and key != NULL, none of which make sense
> > currently.
> 
> Of course all are in-consist and the PMD is free to reject such rules. This is not related though to how we define the default RSS right? 

If we force users to set both key_len = 0 *and* key = NULL like you suggest
in order to get the default behavior and key_len is not enough, I think it
makes sense to also describe these two cases for consistency. Users are
going to wonder why is the damn PMD reading key at all if its length is
zero, so we have to explain that no, the PMD doesn't do that but the pointer
still has to be NULL anyway.

Likewise users shouldn't be tempted to enter a nonzero key length without a
valid key. It also has to be described should we choose this approach.

-- 
Adrien Mazarguil
6WIND