[dpdk-dev] [PATCH] net/mlx5: add supported hash function check

[dpdk-dev] [PATCH] net/mlx5: add supported hash function check

[Xueming Li]

Add supported RSS hash function check in device configuration to
have better error verbosity for application developers.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 drivers/net/mlx5/mlx5_ethdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index b73cb53..175a1ff 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -346,6 +346,14 @@ struct ethtool_link_settings {
 		      rx_offloads, supp_rx_offloads);
 		return ENOTSUP;
 	}
+	if (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &
+	    MLX5_RSS_HF_MASK) {
+		ERROR("Some RSS hash function not supported "
+		      "requested 0x%" PRIx64 " supported 0x%" PRIx64,
+		      dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf,
+		      (uint64_t)(~MLX5_RSS_HF_MASK));
+		return ENOTSUP;
+	}
 	if (use_app_rss_key &&
 	    (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len !=
 	     rss_hash_default_key_len)) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: add supported hash function check

[Nélio Laranjeiro]

On Sun, Mar 18, 2018 at 03:37:20PM +0800, Xueming Li wrote:
> Add supported RSS hash function check in device configuration to
> have better error verbosity for application developers.
>
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_ethdev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index b73cb53..175a1ff 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -346,6 +346,14 @@ struct ethtool_link_settings {
>  		      rx_offloads, supp_rx_offloads);
>  		return ENOTSUP;
>  	}
> +	if (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &
> +	    MLX5_RSS_HF_MASK) {
> +		ERROR("Some RSS hash function not supported "
> +		      "requested 0x%" PRIx64 " supported 0x%" PRIx64,
> +		      dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf,
> +		      (uint64_t)(~MLX5_RSS_HF_MASK));
> +		return ENOTSUP;
> +	}
>  	if (use_app_rss_key &&
>  	    (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len !=
>  	     rss_hash_default_key_len)) {
> -- 
> 1.8.3.1
> 

I would answer than an application should not try to configure something
not advertise by the device. 
This information is present in struct rte_eth_dev_info returned by
mlx5_dev_infos_get() and thus the devops of the device.

Seems rte_eth_dev_configure() should be fixed to avoid configuring wrong
values.

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: add supported hash function check

[Xueming(Steven) Li]

Just remind, denying unsupported hash function in rte_eth_dev_configure() might
impact some user app using PMD that simply ignoring them silently.

Testpmd command "port config <port> rss all" should be updated as well 
to 'all' supported values from rte_eth_dev_info, I'll include this change in 
next version.

> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Monday, March 19, 2018 4:30 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH] net/mlx5: add supported hash function check
> 
> On Sun, Mar 18, 2018 at 03:37:20PM +0800, Xueming Li wrote:
> > Add supported RSS hash function check in device configuration to have
> > better error verbosity for application developers.
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5_ethdev.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > b/drivers/net/mlx5/mlx5_ethdev.c index b73cb53..175a1ff 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -346,6 +346,14 @@ struct ethtool_link_settings {
> >  		      rx_offloads, supp_rx_offloads);
> >  		return ENOTSUP;
> >  	}
> > +	if (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &
> > +	    MLX5_RSS_HF_MASK) {
> > +		ERROR("Some RSS hash function not supported "
> > +		      "requested 0x%" PRIx64 " supported 0x%" PRIx64,
> > +		      dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf,
> > +		      (uint64_t)(~MLX5_RSS_HF_MASK));
> > +		return ENOTSUP;
> > +	}
> >  	if (use_app_rss_key &&
> >  	    (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len !=
> >  	     rss_hash_default_key_len)) {
> > --
> > 1.8.3.1
> >
> 
> I would answer than an application should not try to configure something
> not advertise by the device.
> This information is present in struct rte_eth_dev_info returned by
> mlx5_dev_infos_get() and thus the devops of the device.
> 
> Seems rte_eth_dev_configure() should be fixed to avoid configuring wrong
> values.
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: add supported hash function check

[Nélio Laranjeiro]

On Thu, Mar 22, 2018 at 10:42:44AM +0000, Xueming(Steven) Li wrote:
> Just remind, denying unsupported hash function in rte_eth_dev_configure() might
> impact some user app using PMD that simply ignoring them silently.

If the default behavior from other devices is to use only possible
values, this device should to the same instead of refusing it.

> Testpmd command "port config <port> rss all" should be updated as well 
> to 'all' supported values from rte_eth_dev_info, I'll include this change in 
> next version.
>
> > -----Original Message-----
> > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > Sent: Monday, March 19, 2018 4:30 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; dev@dpdk.org
> > Subject: Re: [PATCH] net/mlx5: add supported hash function check
> > 
> > On Sun, Mar 18, 2018 at 03:37:20PM +0800, Xueming Li wrote:
> > > Add supported RSS hash function check in device configuration to have
> > > better error verbosity for application developers.
> > >
> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5_ethdev.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > > b/drivers/net/mlx5/mlx5_ethdev.c index b73cb53..175a1ff 100644
> > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > @@ -346,6 +346,14 @@ struct ethtool_link_settings {
> > >  		      rx_offloads, supp_rx_offloads);
> > >  		return ENOTSUP;
> > >  	}
> > > +	if (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &
> > > +	    MLX5_RSS_HF_MASK) {
> > > +		ERROR("Some RSS hash function not supported "
> > > +		      "requested 0x%" PRIx64 " supported 0x%" PRIx64,
> > > +		      dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf,
> > > +		      (uint64_t)(~MLX5_RSS_HF_MASK));
> > > +		return ENOTSUP;
> > > +	}
> > >  	if (use_app_rss_key &&
> > >  	    (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len !=
> > >  	     rss_hash_default_key_len)) {
> > > --
> > > 1.8.3.1
> > >
> > 
> > I would answer than an application should not try to configure something
> > not advertise by the device.
> > This information is present in struct rte_eth_dev_info returned by
> > mlx5_dev_infos_get() and thus the devops of the device.
> > 
> > Seems rte_eth_dev_configure() should be fixed to avoid configuring wrong
> > values.
> > 
> > Regards,
> > 
> > --
> > Nélio Laranjeiro
> > 6WIND

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: add supported hash function check

[Xueming(Steven) Li]

> this device should to the same instead of refusing it

Just double confirm, is it "do the same"? are you suggesting not denying
unsupported hash function in rte_eth_dev_configure()? 

From quick view of ixgbe code, kind of try best, would like to hear from 
other PMDs on how this interpreted. 

> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Monday, March 26, 2018 7:40 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH] net/mlx5: add supported hash function check
> 
> On Thu, Mar 22, 2018 at 10:42:44AM +0000, Xueming(Steven) Li wrote:
> > Just remind, denying unsupported hash function in
> > rte_eth_dev_configure() might impact some user app using PMD that simply
> ignoring them silently.
> 
> If the default behavior from other devices is to use only possible values,
> this device should to the same instead of refusing it.
> 
> > Testpmd command "port config <port> rss all" should be updated as well
> > to 'all' supported values from rte_eth_dev_info, I'll include this
> > change in next version.
> >
> > > -----Original Message-----
> > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > > Sent: Monday, March 19, 2018 4:30 PM
> > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> > > <shahafs@mellanox.com>; dev@dpdk.org
> > > Subject: Re: [PATCH] net/mlx5: add supported hash function check
> > >
> > > On Sun, Mar 18, 2018 at 03:37:20PM +0800, Xueming Li wrote:
> > > > Add supported RSS hash function check in device configuration to
> > > > have better error verbosity for application developers.
> > > >
> > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > > ---
> > > >  drivers/net/mlx5/mlx5_ethdev.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > > > b/drivers/net/mlx5/mlx5_ethdev.c index b73cb53..175a1ff 100644
> > > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > > @@ -346,6 +346,14 @@ struct ethtool_link_settings {
> > > >  		      rx_offloads, supp_rx_offloads);
> > > >  		return ENOTSUP;
> > > >  	}
> > > > +	if (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &
> > > > +	    MLX5_RSS_HF_MASK) {
> > > > +		ERROR("Some RSS hash function not supported "
> > > > +		      "requested 0x%" PRIx64 " supported 0x%" PRIx64,
> > > > +		      dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf,
> > > > +		      (uint64_t)(~MLX5_RSS_HF_MASK));
> > > > +		return ENOTSUP;
> > > > +	}
> > > >  	if (use_app_rss_key &&
> > > >  	    (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len !=
> > > >  	     rss_hash_default_key_len)) {
> > > > --
> > > > 1.8.3.1
> > > >
> > >
> > > I would answer than an application should not try to configure
> > > something not advertise by the device.
> > > This information is present in struct rte_eth_dev_info returned by
> > > mlx5_dev_infos_get() and thus the devops of the device.
> > >
> > > Seems rte_eth_dev_configure() should be fixed to avoid configuring
> > > wrong values.
> > >
> > > Regards,
> > >
> > > --
> > > Nélio Laranjeiro
> > > 6WIND
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: add supported hash function check

[Nélio Laranjeiro]

On Mon, Apr 02, 2018 at 12:41:33PM +0000, Xueming(Steven) Li wrote:
> > this device should to the same instead of refusing it
> 
> Just double confirm, is it "do the same"? are you suggesting not denying
> unsupported hash function in rte_eth_dev_configure()?

I mean, if others PMD are ignoring non supported values, but only using
what they support, this PMD should do the same.

> From quick view of ixgbe code, kind of try best, would like to hear from 
> other PMDs on how this interpreted.

Don't expect other maintainers to answer you if you don't ask them
directly.

> > -----Original Message-----
> > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > Sent: Monday, March 26, 2018 7:40 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; dev@dpdk.org
> > Subject: Re: [PATCH] net/mlx5: add supported hash function check
> > 
> > On Thu, Mar 22, 2018 at 10:42:44AM +0000, Xueming(Steven) Li wrote:
> > > Just remind, denying unsupported hash function in
> > > rte_eth_dev_configure() might impact some user app using PMD that simply
> > ignoring them silently.
> > 
> > If the default behavior from other devices is to use only possible values,
> > this device should to the same instead of refusing it.
> > 
> > > Testpmd command "port config <port> rss all" should be updated as well
> > > to 'all' supported values from rte_eth_dev_info, I'll include this
> > > change in next version.
> > >
> > > > -----Original Message-----
> > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > > > Sent: Monday, March 19, 2018 4:30 PM
> > > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> > > > <shahafs@mellanox.com>; dev@dpdk.org
> > > > Subject: Re: [PATCH] net/mlx5: add supported hash function check
> > > >
> > > > On Sun, Mar 18, 2018 at 03:37:20PM +0800, Xueming Li wrote:
> > > > > Add supported RSS hash function check in device configuration to
> > > > > have better error verbosity for application developers.
> > > > >
> > > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > > > ---
> > > > >  drivers/net/mlx5/mlx5_ethdev.c | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > > > > b/drivers/net/mlx5/mlx5_ethdev.c index b73cb53..175a1ff 100644
> > > > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > > > @@ -346,6 +346,14 @@ struct ethtool_link_settings {
> > > > >  		      rx_offloads, supp_rx_offloads);
> > > > >  		return ENOTSUP;
> > > > >  	}
> > > > > +	if (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &
> > > > > +	    MLX5_RSS_HF_MASK) {
> > > > > +		ERROR("Some RSS hash function not supported "
> > > > > +		      "requested 0x%" PRIx64 " supported 0x%" PRIx64,
> > > > > +		      dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf,
> > > > > +		      (uint64_t)(~MLX5_RSS_HF_MASK));
> > > > > +		return ENOTSUP;
> > > > > +	}
> > > > >  	if (use_app_rss_key &&
> > > > >  	    (dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len !=
> > > > >  	     rss_hash_default_key_len)) {
> > > > > --
> > > > > 1.8.3.1
> > > > >
> > > >
> > > > I would answer than an application should not try to configure
> > > > something not advertise by the device.
> > > > This information is present in struct rte_eth_dev_info returned by
> > > > mlx5_dev_infos_get() and thus the devops of the device.
> > > >
> > > > Seems rte_eth_dev_configure() should be fixed to avoid configuring
> > > > wrong values.
> > > >
> > > > Regards,
> > > >
> > > > --
> > > > Nélio Laranjeiro
> > > > 6WIND
> > 
> > --
> > Nélio Laranjeiro
> > 6WIND

-- 
Nélio Laranjeiro
6WIND

[dpdk-dev] [PATCH v1 1/2] ethdev: add supported hash function check

[Xueming Li]

Add supported RSS hash function check in device configuration to
have better error verbosity for application developers.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 209796d46..a8e122781 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1179,6 +1179,17 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 							ETHER_MAX_LEN;
 	}
 
+	/* Check that device supports requested rss hash functions. */
+	if ((dev_info.flow_type_rss_offloads |
+	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: 0x%lx, valid value: 0x%lx\n",
+				    port_id,
+				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -2832,9 +2843,19 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
 			    struct rte_eth_rss_conf *rss_conf)
 {
 	struct rte_eth_dev *dev;
+	struct rte_eth_dev_info dev_info = {0};
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
+	rte_eth_dev_info_get(port_id, &dev_info);
+	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: 0x%lx, valid value: 0x%lx\n",
+				    port_id,
+				    rss_conf->rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_update, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
 								 rss_conf));
-- 
2.13.3

Re: [dpdk-dev] [PATCH v1 1/2] ethdev: add supported hash function check

[Thomas Monjalon]

09/04/2018 14:10, Xueming Li:
> Add supported RSS hash function check in device configuration to
> have better error verbosity for application developers.

In other words, you check whether the requested RSS type is supported.

I don't see any reason to not accept it.
Any other comment?

[dpdk-dev] [PATCH v2 1/2] ethdev: add supported hash function check

[Xueming Li]

Add supported RSS hash function check in device configuration to
have better error verbosity for application developers.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 209796d46..a8e122781 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1179,6 +1179,17 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 							ETHER_MAX_LEN;
 	}
 
+	/* Check that device supports requested rss hash functions. */
+	if ((dev_info.flow_type_rss_offloads |
+	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: 0x%lx, valid value: 0x%lx\n",
+				    port_id,
+				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -2832,9 +2843,19 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
 			    struct rte_eth_rss_conf *rss_conf)
 {
 	struct rte_eth_dev *dev;
+	struct rte_eth_dev_info dev_info = {0};
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
+	rte_eth_dev_info_get(port_id, &dev_info);
+	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: 0x%lx, valid value: 0x%lx\n",
+				    port_id,
+				    rss_conf->rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_update, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
 								 rss_conf));
-- 
2.13.3

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add supported hash function check

[Thomas Monjalon]

17/04/2018 16:24, Xueming Li:
> +	/* Check that device supports requested rss hash functions. */
> +	if ((dev_info.flow_type_rss_offloads |
> +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> +	    dev_info.flow_type_rss_offloads) {
> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: 0x%lx, valid value: 0x%lx\n",
> +				    port_id,
> +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
> +				    dev_info.flow_type_rss_offloads);
> +		return -EINVAL;
> +	}

Please use PRIx64 and test 32-bit compilation.

Reminder from this recent post:
        http://dpdk.org/ml/archives/dev/2018-February/090882.html
"
Most of the times, using %l is wrong (except when printing a long).
So next time you write %l, please think "I am probably wrong".
"

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add supported hash function check

[Xueming(Steven) Li]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, April 18, 2018 6:01 AM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>;
> Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add supported hash function check
> 
> 17/04/2018 16:24, Xueming Li:
> > +	/* Check that device supports requested rss hash functions. */
> > +	if ((dev_info.flow_type_rss_offloads |
> > +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> > +	    dev_info.flow_type_rss_offloads) {
> > +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: 0x%lx, valid value: 0x%lx\n",
> > +				    port_id,
> > +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
> > +				    dev_info.flow_type_rss_offloads);
> > +		return -EINVAL;
> > +	}
> 
> Please use PRIx64 and test 32-bit compilation.
> 
> Reminder from this recent post:
> 
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpdk.org%2Fml%2Farchives%2Fdev%2F201
> 8-
> February%2F090882.html&data=02%7C01%7Cxuemingl%40mellanox.com%7C5508bc716aac41b932fb08d5a4aeb241%7Ca65
> 2971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636595992591300475&sdata=1hirZ1j7VqCMnrzngZFAuQj42OACfxFUgYgzy
> VQyw%2F4%3D&reserved=0
> "
> Most of the times, using %l is wrong (except when printing a long).
> So next time you write %l, please think "I am probably wrong".
> "

Thanks, got following warning from checkpatch when applying this rule to my other patchset, is it normal?
  CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
  #49: FILE: drivers/net/mlx5/mlx5_flow.c:2083:
  +               " hash:%" PRIx64 "/%u specs:%hhu(%hu), priority:%hu, type:%d,"

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add supported hash function check

[Thomas Monjalon]

18/04/2018 13:55, Xueming(Steven) Li:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 17/04/2018 16:24, Xueming Li:
> > > +	/* Check that device supports requested rss hash functions. */
> > > +	if ((dev_info.flow_type_rss_offloads |
> > > +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> > > +	    dev_info.flow_type_rss_offloads) {
> > > +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: 0x%lx, valid value: 0x%lx\n",
> > > +				    port_id,
> > > +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
> > > +				    dev_info.flow_type_rss_offloads);
> > > +		return -EINVAL;
> > > +	}
> > 
> > Please use PRIx64 and test 32-bit compilation.
> > 
> > Reminder from this recent post:
> > 
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpdk.org%2Fml%2Farchives%2Fdev%2F201
> > 8-
> > February%2F090882.html&data=02%7C01%7Cxuemingl%40mellanox.com%7C5508bc716aac41b932fb08d5a4aeb241%7Ca65
> > 2971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636595992591300475&sdata=1hirZ1j7VqCMnrzngZFAuQj42OACfxFUgYgzy
> > VQyw%2F4%3D&reserved=0
> > "
> > Most of the times, using %l is wrong (except when printing a long).
> > So next time you write %l, please think "I am probably wrong".
> > "
> 
> Thanks, got following warning from checkpatch when applying this rule to my other patchset, is it normal?
>   CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
>   #49: FILE: drivers/net/mlx5/mlx5_flow.c:2083:
>   +               " hash:%" PRIx64 "/%u specs:%hhu(%hu), priority:%hu, type:%d,"
> 

Yes it is "normal".
Something we should fix in checkpatch.

[dpdk-dev] [PATCH v2 2/2] app/testpmd: only config supported RSS hash types

[Xueming Li]

"port config all rss all" command will fail on PMD that not support any
of hard coding RSS hash types. This patch changed hard coding hash types
to supported types retrieved from device info.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 app/test-pmd/cmdline.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad7e..ece9be08d 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1879,6 +1879,7 @@ cmd_config_rss_parsed(void *parsed_result,
 {
 	struct cmd_config_rss *res = parsed_result;
 	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
+	struct rte_eth_dev_info dev_info = {0};
 	int diag;
 	uint8_t i;
 
@@ -1915,6 +1916,12 @@ cmd_config_rss_parsed(void *parsed_result,
 	}
 	rss_conf.rss_key = NULL;
 	for (i = 0; i < rte_eth_dev_count(); i++) {
+		if (!strcmp(res->value, "all")) {
+			rte_eth_dev_info_get(i, &dev_info);
+			if (dev_info.flow_type_rss_offloads)
+				rss_conf.rss_hf =
+					dev_info.flow_type_rss_offloads;
+		}
 		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
 		if (diag < 0)
 			printf("Configuration of RSS hash at ethernet port %d "
-- 
2.13.3

[dpdk-dev] [PATCH v3 1/2] ethdev: add supported hash function check

[Xueming Li]

Add supported RSS hash function check in device configuration to
have better error verbosity for application developers.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 3c049ef43..a6212f146 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1179,6 +1179,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 							ETHER_MAX_LEN;
 	}
 
+	/* Check that device supports requested rss hash functions. */
+	if ((dev_info.flow_type_rss_offloads |
+	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -2835,9 +2847,20 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
 			    struct rte_eth_rss_conf *rss_conf)
 {
 	struct rte_eth_dev *dev;
+	struct rte_eth_dev_info dev_info = {0};
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
+	rte_eth_dev_info_get(port_id, &dev_info);
+	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    rss_conf->rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_update, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
 								 rss_conf));
-- 
2.13.3

[dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Xueming Li]

"port config all rss all" command will fail on PMD that not support any
of hard coding RSS hash types. This patch changed hard coding hash types
to supported types retrieved from device info.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 app/test-pmd/cmdline.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 512e3b55e..d357de7e6 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,
 {
 	struct cmd_config_rss *res = parsed_result;
 	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
+	struct rte_eth_dev_info dev_info = {0};
 	int diag;
 	uint8_t i;
 
@@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
 	}
 	rss_conf.rss_key = NULL;
 	for (i = 0; i < rte_eth_dev_count(); i++) {
+		if (!strcmp(res->value, "all")) {
+			rte_eth_dev_info_get(i, &dev_info);
+			if (dev_info.flow_type_rss_offloads)
+				rss_conf.rss_hf =
+					dev_info.flow_type_rss_offloads;
+		}
 		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
 		if (diag < 0)
 			printf("Configuration of RSS hash at ethernet port %d "
-- 
2.13.3

[dpdk-dev] [PATCH v3 1/2] ethdev: add supported hash function check

[Xueming Li]

Add supported RSS hash function check in device configuration to
have better error verbosity for application developers.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 3c049ef43..4ac0fadae 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1179,6 +1179,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 							ETHER_MAX_LEN;
 	}
 
+	/* Check that device supports requested rss hash functions. */
+	if ((dev_info.flow_type_rss_offloads |
+	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -2835,9 +2847,20 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
 			    struct rte_eth_rss_conf *rss_conf)
 {
 	struct rte_eth_dev *dev;
+	struct rte_eth_dev_info dev_info = {0};
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
+	rte_eth_dev_info_get(port_id, &dev_info);
+	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    rss_conf->rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_update, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
 								 rss_conf));
-- 
2.13.3

[dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Xueming Li]

"port config all rss all" command will fail on PMD that not support any
of hard coding RSS hash types. This patch changed hard coding hash types
to supported types retrieved from device info.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 app/test-pmd/cmdline.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 512e3b55e..d357de7e6 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,
 {
 	struct cmd_config_rss *res = parsed_result;
 	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
+	struct rte_eth_dev_info dev_info = {0};
 	int diag;
 	uint8_t i;
 
@@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
 	}
 	rss_conf.rss_key = NULL;
 	for (i = 0; i < rte_eth_dev_count(); i++) {
+		if (!strcmp(res->value, "all")) {
+			rte_eth_dev_info_get(i, &dev_info);
+			if (dev_info.flow_type_rss_offloads)
+				rss_conf.rss_hf =
+					dev_info.flow_type_rss_offloads;
+		}
 		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
 		if (diag < 0)
 			printf("Configuration of RSS hash at ethernet port %d "
-- 
2.13.3

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Adrien Mazarguil]

Hi Xueming,

On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> "port config all rss all" command will fail on PMD that not support any
> of hard coding RSS hash types. This patch changed hard coding hash types
> to supported types retrieved from device info.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>

Problem is that this patch removes the ability to request "all" RSS types
regardless of PMD support.

Users will expect "all" to behave as documented, however doing so will only
result in the limited set of types reported by PMDs. For instance at least
mlx4 doesn't update the flow_type_rss_offloads field since it has never
implemented configuration support for the legacy RSS API.

You should add an argument with a different name (e.g. "supported" or
"default") for that and keep the original meaning for "all".

Testpmd documentation has to be updated accordingly.

One more nit below.

> ---
>  app/test-pmd/cmdline.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 512e3b55e..d357de7e6 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,
>  {
>  	struct cmd_config_rss *res = parsed_result;
>  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> +	struct rte_eth_dev_info dev_info = {0};

This could be declared in the new block where it's used. Also note that
"{0}" is nonstandard syntax, use memset() or initialize a field like
rss_key_len above.

>  	int diag;
>  	uint8_t i;
>  
> @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
>  	}
>  	rss_conf.rss_key = NULL;
>  	for (i = 0; i < rte_eth_dev_count(); i++) {
> +		if (!strcmp(res->value, "all")) {
> +			rte_eth_dev_info_get(i, &dev_info);
> +			if (dev_info.flow_type_rss_offloads)
> +				rss_conf.rss_hf =
> +					dev_info.flow_type_rss_offloads;
> +		}
>  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
>  		if (diag < 0)
>  			printf("Configuration of RSS hash at ethernet port %d "
> -- 
> 2.13.3
> 


-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Xueming(Steven) Li]

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 9:26 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> 
> Hi Xueming,
> 
> On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> > "port config all rss all" command will fail on PMD that not support
> > any of hard coding RSS hash types. This patch changed hard coding hash
> > types to supported types retrieved from device info.
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> 
> Problem is that this patch removes the ability to request "all" RSS types regardless of PMD support.
> 
> Users will expect "all" to behave as documented, however doing so will only result in the limited set
> of types reported by PMDs. For instance at least
> mlx4 doesn't update the flow_type_rss_offloads field since it has never implemented configuration
> support for the legacy RSS API.
> 
> You should add an argument with a different name (e.g. "supported" or
> "default") for that and keep the original meaning for "all".
> 
> Testpmd documentation has to be updated accordingly.

You must want to have a look at first part of this patchset:
http://www.dpdk.org/ml/archives/dev/2018-April/097849.html

> 
> One more nit below.
> 
> > ---
> >  app/test-pmd/cmdline.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 512e3b55e..d357de7e6 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,  {
> >  	struct cmd_config_rss *res = parsed_result;
> >  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> > +	struct rte_eth_dev_info dev_info = {0};
> 
> This could be declared in the new block where it's used. Also note that "{0}" is nonstandard syntax,
> use memset() or initialize a field like rss_key_len above.
> 
> >  	int diag;
> >  	uint8_t i;
> >
> > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
> >  	}
> >  	rss_conf.rss_key = NULL;
> >  	for (i = 0; i < rte_eth_dev_count(); i++) {
> > +		if (!strcmp(res->value, "all")) {
> > +			rte_eth_dev_info_get(i, &dev_info);
> > +			if (dev_info.flow_type_rss_offloads)
> > +				rss_conf.rss_hf =
> > +					dev_info.flow_type_rss_offloads;
> > +		}
> >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> >  		if (diag < 0)
> >  			printf("Configuration of RSS hash at ethernet port %d "
> > --
> > 2.13.3
> >
> 
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Adrien Mazarguil]

On Wed, Apr 18, 2018 at 01:54:20PM +0000, Xueming(Steven) Li wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Wednesday, April 18, 2018 9:26 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> > <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> > dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> > 
> > Hi Xueming,
> > 
> > On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> > > "port config all rss all" command will fail on PMD that not support
> > > any of hard coding RSS hash types. This patch changed hard coding hash
> > > types to supported types retrieved from device info.
> > >
> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > 
> > Problem is that this patch removes the ability to request "all" RSS types regardless of PMD support.
> > 
> > Users will expect "all" to behave as documented, however doing so will only result in the limited set
> > of types reported by PMDs. For instance at least
> > mlx4 doesn't update the flow_type_rss_offloads field since it has never implemented configuration
> > support for the legacy RSS API.
> > 
> > You should add an argument with a different name (e.g. "supported" or
> > "default") for that and keep the original meaning for "all".
> > 
> > Testpmd documentation has to be updated accordingly.
> 
> You must want to have a look at first part of this patchset:
> http://www.dpdk.org/ml/archives/dev/2018-April/097849.html

I did, however this is does not address my previous comment.

In this patch you're preventing testpmd users from requesting something. If
they want to enable "all" RSS types and some of them happen to be
unsupported on some ports, it means they want to get errors. Otherwise they
might think their request succeeded while it was in fact interpreted in an
undocumented manner.

The point is if users want to enable only "supported" RSS types, they
wouldn't write "all" on the command line right? A new parameter is needed
for that.

> 
> > 
> > One more nit below.
> > 
> > > ---
> > >  app/test-pmd/cmdline.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > 512e3b55e..d357de7e6 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,  {
> > >  	struct cmd_config_rss *res = parsed_result;
> > >  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> > > +	struct rte_eth_dev_info dev_info = {0};
> > 
> > This could be declared in the new block where it's used. Also note that "{0}" is nonstandard syntax,
> > use memset() or initialize a field like rss_key_len above.
> > 
> > >  	int diag;
> > >  	uint8_t i;
> > >
> > > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
> > >  	}
> > >  	rss_conf.rss_key = NULL;
> > >  	for (i = 0; i < rte_eth_dev_count(); i++) {
> > > +		if (!strcmp(res->value, "all")) {
> > > +			rte_eth_dev_info_get(i, &dev_info);
> > > +			if (dev_info.flow_type_rss_offloads)
> > > +				rss_conf.rss_hf =
> > > +					dev_info.flow_type_rss_offloads;
> > > +		}
> > >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> > >  		if (diag < 0)
> > >  			printf("Configuration of RSS hash at ethernet port %d "
> > > --
> > > 2.13.3
> > >
> > 
> > 
> > --
> > Adrien Mazarguil
> > 6WIND

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Xueming(Steven) Li]

+Olivier

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 10:17 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> 
> On Wed, Apr 18, 2018 at 01:54:20PM +0000, Xueming(Steven) Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Sent: Wednesday, April 18, 2018 9:26 PM
> > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> > > <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> > > Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config
> > > supported RSS hash types
> > >
> > > Hi Xueming,
> > >
> > > On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> > > > "port config all rss all" command will fail on PMD that not
> > > > support any of hard coding RSS hash types. This patch changed hard
> > > > coding hash types to supported types retrieved from device info.
> > > >
> > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > >
> > > Problem is that this patch removes the ability to request "all" RSS types regardless of PMD
> support.
> > >
> > > Users will expect "all" to behave as documented, however doing so
> > > will only result in the limited set of types reported by PMDs. For
> > > instance at least
> > > mlx4 doesn't update the flow_type_rss_offloads field since it has
> > > never implemented configuration support for the legacy RSS API.
> > >
> > > You should add an argument with a different name (e.g. "supported"
> > > or
> > > "default") for that and keep the original meaning for "all".
> > >
> > > Testpmd documentation has to be updated accordingly.
> >
> > You must want to have a look at first part of this patchset:
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> > dpdk.org%2Fml%2Farchives%2Fdev%2F2018-April%2F097849.html&data=02%7C01
> > %7Cxuemingl%40mellanox.com%7C497583093d324a6861f708d5a5370a8f%7Ca65297
> > 1c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636596578186582002&sdata=uNVw2egf
> > 0rcTHazCuoMelNrY44lZVfd6Ado1bJfzg8s%3D&reserved=0
> 
> I did, however this is does not address my previous comment.

In first patch, if flow_type_rss_offloads not set, any RSS will not be allowed.
We need to clarify definition of empty flow_type_rss_offloads, not set or no rss support.

> 
> In this patch you're preventing testpmd users from requesting something. If they want to enable "all"
> RSS types and some of them happen to be unsupported on some ports, it means they want to get errors.
> Otherwise they might think their request succeeded while it was in fact interpreted in an undocumented
> manner.
> 
> The point is if users want to enable only "supported" RSS types, they wouldn't write "all" on the
> command line right? A new parameter is needed for that.
> 
> >
> > >
> > > One more nit below.
> > >
> > > > ---
> > > >  app/test-pmd/cmdline.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > 512e3b55e..d357de7e6 100644
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,  {
> > > >  	struct cmd_config_rss *res = parsed_result;
> > > >  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> > > > +	struct rte_eth_dev_info dev_info = {0};
> > >
> > > This could be declared in the new block where it's used. Also note
> > > that "{0}" is nonstandard syntax, use memset() or initialize a field like rss_key_len above.
> > >
> > > >  	int diag;
> > > >  	uint8_t i;
> > > >
> > > > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
> > > >  	}
> > > >  	rss_conf.rss_key = NULL;
> > > >  	for (i = 0; i < rte_eth_dev_count(); i++) {
> > > > +		if (!strcmp(res->value, "all")) {
> > > > +			rte_eth_dev_info_get(i, &dev_info);
> > > > +			if (dev_info.flow_type_rss_offloads)
> > > > +				rss_conf.rss_hf =
> > > > +					dev_info.flow_type_rss_offloads;
> > > > +		}
> > > >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> > > >  		if (diag < 0)
> > > >  			printf("Configuration of RSS hash at ethernet port %d "
> > > > --
> > > > 2.13.3
> > > >
> > >
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Adrien Mazarguil]

On Wed, Apr 18, 2018 at 02:26:30PM +0000, Xueming(Steven) Li wrote:
> +Olivier
> 
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Wednesday, April 18, 2018 10:17 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> > <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> > dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> > 
> > On Wed, Apr 18, 2018 at 01:54:20PM +0000, Xueming(Steven) Li wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > > Sent: Wednesday, April 18, 2018 9:26 PM
> > > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> > > > <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> > > > Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config
> > > > supported RSS hash types
> > > >
> > > > Hi Xueming,
> > > >
> > > > On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> > > > > "port config all rss all" command will fail on PMD that not
> > > > > support any of hard coding RSS hash types. This patch changed hard
> > > > > coding hash types to supported types retrieved from device info.
> > > > >
> > > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > >
> > > > Problem is that this patch removes the ability to request "all" RSS types regardless of PMD
> > support.
> > > >
> > > > Users will expect "all" to behave as documented, however doing so
> > > > will only result in the limited set of types reported by PMDs. For
> > > > instance at least
> > > > mlx4 doesn't update the flow_type_rss_offloads field since it has
> > > > never implemented configuration support for the legacy RSS API.
> > > >
> > > > You should add an argument with a different name (e.g. "supported"
> > > > or
> > > > "default") for that and keep the original meaning for "all".
> > > >
> > > > Testpmd documentation has to be updated accordingly.
> > >
> > > You must want to have a look at first part of this patchset:
> > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> > > dpdk.org%2Fml%2Farchives%2Fdev%2F2018-April%2F097849.html&data=02%7C01
> > > %7Cxuemingl%40mellanox.com%7C497583093d324a6861f708d5a5370a8f%7Ca65297
> > > 1c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636596578186582002&sdata=uNVw2egf
> > > 0rcTHazCuoMelNrY44lZVfd6Ado1bJfzg8s%3D&reserved=0
> > 
> > I did, however this is does not address my previous comment.
> 
> In first patch, if flow_type_rss_offloads not set, any RSS will not be allowed.
> We need to clarify definition of empty flow_type_rss_offloads, not set or no rss support.

It's fine, mlx4 does not implement rte_eth_dev_rss_hash_update() either
(legacy RSS configuration is not implemented at all).

With mlx4, the command:

 port config all rss (all|anything else)

Always returns an error. It's a good thing because it's not implemented and
users must be made aware of that fact. No need to lie about it.

> > In this patch you're preventing testpmd users from requesting something. If they want to enable "all"
> > RSS types and some of them happen to be unsupported on some ports, it means they want to get errors.
> > Otherwise they might think their request succeeded while it was in fact interpreted in an undocumented
> > manner.
> > 
> > The point is if users want to enable only "supported" RSS types, they wouldn't write "all" on the
> > command line right? A new parameter is needed for that.
> > 
> > >
> > > >
> > > > One more nit below.
> > > >
> > > > > ---
> > > > >  app/test-pmd/cmdline.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > > 512e3b55e..d357de7e6 100644
> > > > > --- a/app/test-pmd/cmdline.c
> > > > > +++ b/app/test-pmd/cmdline.c
> > > > > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,  {
> > > > >  	struct cmd_config_rss *res = parsed_result;
> > > > >  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> > > > > +	struct rte_eth_dev_info dev_info = {0};
> > > >
> > > > This could be declared in the new block where it's used. Also note
> > > > that "{0}" is nonstandard syntax, use memset() or initialize a field like rss_key_len above.
> > > >
> > > > >  	int diag;
> > > > >  	uint8_t i;
> > > > >
> > > > > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
> > > > >  	}
> > > > >  	rss_conf.rss_key = NULL;
> > > > >  	for (i = 0; i < rte_eth_dev_count(); i++) {
> > > > > +		if (!strcmp(res->value, "all")) {
> > > > > +			rte_eth_dev_info_get(i, &dev_info);
> > > > > +			if (dev_info.flow_type_rss_offloads)
> > > > > +				rss_conf.rss_hf =
> > > > > +					dev_info.flow_type_rss_offloads;
> > > > > +		}
> > > > >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> > > > >  		if (diag < 0)
> > > > >  			printf("Configuration of RSS hash at ethernet port %d "
> > > > > --
> > > > > 2.13.3
> > > > >
> > > >
> > > >
> > > > --
> > > > Adrien Mazarguil
> > > > 6WIND
> > 
> > --
> > Adrien Mazarguil
> > 6WIND

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Xueming(Steven) Li]

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 9:26 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> 
> Hi Xueming,
> 
> On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> > "port config all rss all" command will fail on PMD that not support
> > any of hard coding RSS hash types. This patch changed hard coding hash
> > types to supported types retrieved from device info.
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> 
> Problem is that this patch removes the ability to request "all" RSS types regardless of PMD support.
> 
> Users will expect "all" to behave as documented, however doing so will only result in the limited set
> of types reported by PMDs. For instance at least
> mlx4 doesn't update the flow_type_rss_offloads field since it has never implemented configuration
> support for the legacy RSS API.

Keeping it "all" as it is will result in error for PMDs not completely support "all" RSS types and this 
always confuse people.
How about adding a check, if flow_type_rss_offloads not set, ignore and set default RSS "all" types?

> 
> You should add an argument with a different name (e.g. "supported" or
> "default") for that and keep the original meaning for "all".
> 
> Testpmd documentation has to be updated accordingly.
> 
> One more nit below.
> 
> > ---
> >  app/test-pmd/cmdline.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 512e3b55e..d357de7e6 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,  {
> >  	struct cmd_config_rss *res = parsed_result;
> >  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> > +	struct rte_eth_dev_info dev_info = {0};
> 
> This could be declared in the new block where it's used. Also note that "{0}" is nonstandard syntax,
> use memset() or initialize a field like rss_key_len above.
> 
> >  	int diag;
> >  	uint8_t i;
> >
> > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
> >  	}
> >  	rss_conf.rss_key = NULL;
> >  	for (i = 0; i < rte_eth_dev_count(); i++) {
> > +		if (!strcmp(res->value, "all")) {
> > +			rte_eth_dev_info_get(i, &dev_info);
> > +			if (dev_info.flow_type_rss_offloads)
> > +				rss_conf.rss_hf =
> > +					dev_info.flow_type_rss_offloads;
> > +		}
> >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> >  		if (diag < 0)
> >  			printf("Configuration of RSS hash at ethernet port %d "
> > --
> > 2.13.3
> >
> 
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Adrien Mazarguil]

On Wed, Apr 18, 2018 at 02:10:45PM +0000, Xueming(Steven) Li wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Wednesday, April 18, 2018 9:26 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> > <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> > dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> > 
> > Hi Xueming,
> > 
> > On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> > > "port config all rss all" command will fail on PMD that not support
> > > any of hard coding RSS hash types. This patch changed hard coding hash
> > > types to supported types retrieved from device info.
> > >
> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > 
> > Problem is that this patch removes the ability to request "all" RSS types regardless of PMD support.
> > 
> > Users will expect "all" to behave as documented, however doing so will only result in the limited set
> > of types reported by PMDs. For instance at least
> > mlx4 doesn't update the flow_type_rss_offloads field since it has never implemented configuration
> > support for the legacy RSS API.
> 
> Keeping it "all" as it is will result in error for PMDs not completely support "all" RSS types and this 
> always confuse people.

You're redefining the commonly accepted meaning of "all". The first instance
of "all" in the "port config all rss all" command means "all ports", not
"only ports that happen to implement the RSS configuration callback". Same
for all other testpmd commands accepting "all" as a parameter anywhere.

Those that don't will return errors, it's fine.

> How about adding a check, if flow_type_rss_offloads not set, ignore and set default RSS "all" types?

Results would be even less predictable. Keep it simple. Testpmd is a testing
tool, if PMD developers want to check error-spitting abilities of their
PMDs, they must be able to. Adding a dedicated parameter is one way to
satisfy everyone, dropping this patch is another.

> > You should add an argument with a different name (e.g. "supported" or
> > "default") for that and keep the original meaning for "all".
> > 
> > Testpmd documentation has to be updated accordingly.
> > 
> > One more nit below.
> > 
> > > ---
> > >  app/test-pmd/cmdline.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > 512e3b55e..d357de7e6 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,  {
> > >  	struct cmd_config_rss *res = parsed_result;
> > >  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> > > +	struct rte_eth_dev_info dev_info = {0};
> > 
> > This could be declared in the new block where it's used. Also note that "{0}" is nonstandard syntax,
> > use memset() or initialize a field like rss_key_len above.
> > 
> > >  	int diag;
> > >  	uint8_t i;
> > >
> > > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
> > >  	}
> > >  	rss_conf.rss_key = NULL;
> > >  	for (i = 0; i < rte_eth_dev_count(); i++) {
> > > +		if (!strcmp(res->value, "all")) {
> > > +			rte_eth_dev_info_get(i, &dev_info);
> > > +			if (dev_info.flow_type_rss_offloads)
> > > +				rss_conf.rss_hf =
> > > +					dev_info.flow_type_rss_offloads;
> > > +		}
> > >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> > >  		if (diag < 0)
> > >  			printf("Configuration of RSS hash at ethernet port %d "
> > > --
> > > 2.13.3
> > >
> > 
> > 
> > --
> > Adrien Mazarguil
> > 6WIND

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types

[Xueming(Steven) Li]

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 10:30 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> 
> On Wed, Apr 18, 2018 at 02:10:45PM +0000, Xueming(Steven) Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Sent: Wednesday, April 18, 2018 9:26 PM
> > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> > > <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> > > Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config
> > > supported RSS hash types
> > >
> > > Hi Xueming,
> > >
> > > On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote:
> > > > "port config all rss all" command will fail on PMD that not
> > > > support any of hard coding RSS hash types. This patch changed hard
> > > > coding hash types to supported types retrieved from device info.
> > > >
> > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > >
> > > Problem is that this patch removes the ability to request "all" RSS types regardless of PMD
> support.
> > >
> > > Users will expect "all" to behave as documented, however doing so
> > > will only result in the limited set of types reported by PMDs. For
> > > instance at least
> > > mlx4 doesn't update the flow_type_rss_offloads field since it has
> > > never implemented configuration support for the legacy RSS API.
> >
> > Keeping it "all" as it is will result in error for PMDs not completely
> > support "all" RSS types and this always confuse people.
> 
> You're redefining the commonly accepted meaning of "all". The first instance of "all" in the "port
> config all rss all" command means "all ports", not "only ports that happen to implement the RSS
> configuration callback". Same for all other testpmd commands accepting "all" as a parameter anywhere.
> 
> Those that don't will return errors, it's fine.
> 
> > How about adding a check, if flow_type_rss_offloads not set, ignore and set default RSS "all" types?
> 
> Results would be even less predictable. Keep it simple. Testpmd is a testing tool, if PMD developers
> want to check error-spitting abilities of their PMDs, they must be able to. Adding a dedicated
> parameter is one way to satisfy everyone, dropping this patch is another.

Okay, v4 version sent by adding "default" parameter, thanks.

> 
> > > You should add an argument with a different name (e.g. "supported"
> > > or
> > > "default") for that and keep the original meaning for "all".
> > >
> > > Testpmd documentation has to be updated accordingly.
> > >
> > > One more nit below.
> > >
> > > > ---
> > > >  app/test-pmd/cmdline.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > 512e3b55e..d357de7e6 100644
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,  {
> > > >  	struct cmd_config_rss *res = parsed_result;
> > > >  	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
> > > > +	struct rte_eth_dev_info dev_info = {0};
> > >
> > > This could be declared in the new block where it's used. Also note
> > > that "{0}" is nonstandard syntax, use memset() or initialize a field like rss_key_len above.
> > >
> > > >  	int diag;
> > > >  	uint8_t i;
> > > >
> > > > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result,
> > > >  	}
> > > >  	rss_conf.rss_key = NULL;
> > > >  	for (i = 0; i < rte_eth_dev_count(); i++) {
> > > > +		if (!strcmp(res->value, "all")) {
> > > > +			rte_eth_dev_info_get(i, &dev_info);
> > > > +			if (dev_info.flow_type_rss_offloads)
> > > > +				rss_conf.rss_hf =
> > > > +					dev_info.flow_type_rss_offloads;
> > > > +		}
> > > >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> > > >  		if (diag < 0)
> > > >  			printf("Configuration of RSS hash at ethernet port %d "
> > > > --
> > > > 2.13.3
> > > >
> > >
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> 
> --
> Adrien Mazarguil
> 6WIND

[dpdk-dev] [PATCH v4 1/2] ethdev: add supported hash function check

[Xueming Li]

Add supported RSS hash function check in device configuration to
have better error verbosity for application developers.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 3c049ef43..4ac0fadae 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1179,6 +1179,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 							ETHER_MAX_LEN;
 	}
 
+	/* Check that device supports requested rss hash functions. */
+	if ((dev_info.flow_type_rss_offloads |
+	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -2835,9 +2847,20 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
 			    struct rte_eth_rss_conf *rss_conf)
 {
 	struct rte_eth_dev *dev;
+	struct rte_eth_dev_info dev_info = {0};
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
+	rte_eth_dev_info_get(port_id, &dev_info);
+	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    rss_conf->rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_update, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
 								 rss_conf));
-- 
2.13.3

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add supported hash function check

[Adrien Mazarguil]

On Thu, Apr 19, 2018 at 11:48:39PM +0800, Xueming Li wrote:
> Add supported RSS hash function check in device configuration to
> have better error verbosity for application developers.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>

In general, I do not like API/wrappers generating log messages on their own,
as those can't be caught and interpreted by applications. I believe error
codes should be explicit enough. However in this specific instance,
rte_eth_dev_configure() already does it a lot, so I guess it's fine:

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

One nit below.

> ---
>  lib/librte_ether/rte_ethdev.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 3c049ef43..4ac0fadae 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1179,6 +1179,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  							ETHER_MAX_LEN;
>  	}
>  
> +	/* Check that device supports requested rss hash functions. */
> +	if ((dev_info.flow_type_rss_offloads |
> +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> +	    dev_info.flow_type_rss_offloads) {
> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
> +				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
> +				    port_id,
> +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
> +				    dev_info.flow_type_rss_offloads);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Setup new number of RX/TX queues and reconfigure device.
>  	 */
> @@ -2835,9 +2847,20 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
>  			    struct rte_eth_rss_conf *rss_conf)
>  {
>  	struct rte_eth_dev *dev;
> +	struct rte_eth_dev_info dev_info = {0};

A "{0}" initializer for structures is nonstandard and may trigger compiler
warnings. I encourage you to initialize one field C99-style instead:

 struct rte_eth_dev_info dev_info = { .device = NULL };

> 
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
> +	rte_eth_dev_info_get(port_id, &dev_info);
> +	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
> +	    dev_info.flow_type_rss_offloads) {
> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
> +				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
> +				    port_id,
> +				    rss_conf->rss_hf,
> +				    dev_info.flow_type_rss_offloads);
> +		return -EINVAL;
> +	}
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_update, -ENOTSUP);
>  	return eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
>  								 rss_conf));
> -- 
> 2.13.3
> 

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH v4 2/2] app/testpmd: new parameter for port config all rss command

[Xueming Li]

This patches add "default" parameter to "port config all rss" command.
"default" means all supported hash types reported by device info.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 app/test-pmd/cmdline.c                      | 13 +++++++++----
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 512e3b55e..77068129e 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -821,8 +821,8 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"    Set crc-strip/scatter/rx-checksum/hardware-vlan/drop_en"
 			" for ports.\n\n"
 
-			"port config all rss (all|ip|tcp|udp|sctp|ether|port|vxlan|"
-			"geneve|nvgre|none|<flowtype_id>)\n"
+			"port config all rss (all|default|ip|tcp|udp|sctp|"
+			"ether|port|vxlan|geneve|nvgre|none|<flowtype_id>)\n"
 			"    Set the RSS mode.\n\n"
 
 			"port config port-id rss reta (hash,queue)[,(hash,queue)]\n"
@@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,
 {
 	struct cmd_config_rss *res = parsed_result;
 	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
+	struct rte_eth_dev_info dev_info = { .flow_type_rss_offloads = 0, };
 	int diag;
 	uint8_t i;
 
@@ -2023,7 +2024,7 @@ cmd_config_rss_parsed(void *parsed_result,
 		rss_conf.rss_hf = ETH_RSS_GENEVE;
 	else if (!strcmp(res->value, "nvgre"))
 		rss_conf.rss_hf = ETH_RSS_NVGRE;
-	else if (!strcmp(res->value, "none"))
+	else if (!strcmp(res->value, "none") || !strcmp(res->value, "default"))
 		rss_conf.rss_hf = 0;
 	else if (isdigit(res->value[0]) && atoi(res->value) > 0 &&
 						atoi(res->value) < 64)
@@ -2034,6 +2035,10 @@ cmd_config_rss_parsed(void *parsed_result,
 	}
 	rss_conf.rss_key = NULL;
 	for (i = 0; i < rte_eth_dev_count(); i++) {
+		if (!strcmp(res->value, "default")) {
+			rte_eth_dev_info_get(i, &dev_info);
+			rss_conf.rss_hf = dev_info.flow_type_rss_offloads;
+		}
 		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
 		if (diag < 0)
 			printf("Configuration of RSS hash at ethernet port %d "
@@ -2057,7 +2062,7 @@ cmdline_parse_inst_t cmd_config_rss = {
 	.f = cmd_config_rss_parsed,
 	.data = NULL,
 	.help_str = "port config all rss "
-		"all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|<flowtype_id>",
+		"all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|<flowtype_id>",
 	.tokens = {
 		(void *)&cmd_config_rss_port,
 		(void *)&cmd_config_rss_keyword,
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index a766ac795..0e1719d9a 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1751,10 +1751,12 @@ port config - RSS
 
 Set the RSS (Receive Side Scaling) mode on or off::
 
-   testpmd> port config all rss (all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none)
+   testpmd> port config all rss (all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none)
 
 RSS is on by default.
 
+The ``all`` option is equivalent to ip|tcp|udp|sctp|ether.
+The ``default`` option enables all supported RSS types reported by device info.
 The ``none`` option is equivalent to the ``--disable-rss`` command-line option.
 
 port config - RSS Reta
-- 
2.13.3

Re: [dpdk-dev] [PATCH v4 2/2] app/testpmd: new parameter for port config all rss command

[Adrien Mazarguil]

On Thu, Apr 19, 2018 at 11:48:40PM +0800, Xueming Li wrote:
> This patches add "default" parameter to "port config all rss" command.
> "default" means all supported hash types reported by device info.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>

Looks good, thanks.

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO

[Xueming Li]

This patch introduce new TX offload flags for device that supports
IP or UDP tunneled packet L3/L4 checksum and TSO offload.
It will be used for non-standard tunnels.

The support from the device is for inner and outer checksums on
IPV4/TCP/UDP and TSO for *any packet with the following format*:

<some headers> / [optional IPv4/IPv6] / [optional TCP/UDP] / <some
headers> / [optional inner IPv4/IPv6] / [optional TCP/UDP]

For example the following packets can use this feature:

1. eth / ipv4 / udp / VXLAN / ip / tcp
2. eth / ipv4 / GRE / MPLS / ipv4 / udp

Please note that specific tunnel headers that contain payload length,
sequence id or checksum will not be updated.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ether/rte_ethdev.h | 12 ++++++++++++
 lib/librte_mbuf/rte_mbuf.c    |  6 ++++++
 lib/librte_mbuf/rte_mbuf.h    | 25 +++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 4f417f573..ef152dec6 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -980,6 +980,18 @@ struct rte_eth_conf {
  *   the same mempool and has refcnt = 1.
  */
 #define DEV_TX_OFFLOAD_SECURITY         0x00020000
+/**
+ * Device supports generic UDP tunneled packet TSO.
+ * Application must set PKT_TX_TUNNEL_UDP and other mbuf fields required
+ * for tunnel TSO.
+ */
+#define DEV_TX_OFFLOAD_UDP_TNL_TSO      0x00040000
+/**
+ * Device supports generic IP tunneled packet TSO.
+ * Application must set PKT_TX_TUNNEL_IP and other mbuf fields required
+ * for tunnel TSO.
+ */
+#define DEV_TX_OFFLOAD_IP_TNL_TSO       0x00080000
 
 /*
  * If new Tx offload capabilities are defined, they also must be
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 3f4c83305..64e960d4c 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -390,6 +390,8 @@ const char *rte_get_tx_ol_flag_name(uint64_t mask)
 	case PKT_TX_TUNNEL_IPIP: return "PKT_TX_TUNNEL_IPIP";
 	case PKT_TX_TUNNEL_GENEVE: return "PKT_TX_TUNNEL_GENEVE";
 	case PKT_TX_TUNNEL_MPLSINUDP: return "PKT_TX_TUNNEL_MPLSINUDP";
+	case PKT_TX_TUNNEL_IP: return "PKT_TX_TUNNEL_IP";
+	case PKT_TX_TUNNEL_UDP: return "PKT_TX_TUNNEL_UDP";
 	case PKT_TX_MACSEC: return "PKT_TX_MACSEC";
 	case PKT_TX_SEC_OFFLOAD: return "PKT_TX_SEC_OFFLOAD";
 	default: return NULL;
@@ -424,6 +426,10 @@ rte_get_tx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
 		  "PKT_TX_TUNNEL_NONE" },
 		{ PKT_TX_TUNNEL_MPLSINUDP, PKT_TX_TUNNEL_MASK,
 		  "PKT_TX_TUNNEL_NONE" },
+		{ PKT_TX_TUNNEL_IP, PKT_TX_TUNNEL_MASK,
+		  "PKT_TX_TUNNEL_NONE" },
+		{ PKT_TX_TUNNEL_UDP, PKT_TX_TUNNEL_MASK,
+		  "PKT_TX_TUNNEL_NONE" },
 		{ PKT_TX_MACSEC, PKT_TX_MACSEC, NULL },
 		{ PKT_TX_SEC_OFFLOAD, PKT_TX_SEC_OFFLOAD, NULL },
 	};
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 06eceba37..80595e6e2 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -210,6 +210,31 @@ extern "C" {
 #define PKT_TX_TUNNEL_GENEVE  (0x4ULL << 45)
 /**< TX packet with MPLS-in-UDP RFC 7510 header. */
 #define PKT_TX_TUNNEL_MPLSINUDP (0x5ULL << 45)
+/**
+ * Generic IP encapsulated tunnel type, used for TSO and checksum offload.
+ * It can be used for tunnels which are not standards or listed above.
+ * It is preferred to use specific tunnel flags like PKT_TX_TUNNEL_GRE
+ * or PKT_TX_TUNNEL_IPIP if possible.
+ * The ethdev must be configured with DEV_TX_OFFLOAD_IP_TNL_TSO.
+ * Outer and inner checksums are done according to the existing flags like
+ * PKT_TX_xxx_CKSUM.
+ * Specific tunnel headers that contain payload length, sequence id
+ * or checksum are not expected to be updated.
+ */
+#define PKT_TX_TUNNEL_IP (0xDULL << 45)
+/**
+ * Generic UDP encapsulated tunnel type, used for TSO and checksum offload.
+ * UDP tunnel type implies outer IP layer.
+ * It can be used for tunnels which are not standards or listed above.
+ * It is preferred to use specific tunnel flags like PKT_TX_TUNNEL_GRE
+ * or PKT_TX_TUNNEL_IPIP if possible.
+ * The ethdev must be configured with DEV_TX_OFFLOAD_UDP_TNL_TSO.
+ * Outer and inner checksums are done according to the existing flags like
+ * PKT_TX_xxx_CKSUM.
+ * Specific tunnel headers that contain payload length, sequence id
+ * or checksum are not expected to be updated.
+ */
+#define PKT_TX_TUNNEL_UDP (0xEULL << 45)
 /* add new TX TUNNEL type here */
 #define PKT_TX_TUNNEL_MASK    (0xFULL << 45)
 
-- 
2.13.3

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO

[Ferruh Yigit]

On 4/20/2018 2:06 PM, Xueming Li wrote:
> This patch introduce new TX offload flags for device that supports
> IP or UDP tunneled packet L3/L4 checksum and TSO offload.
> It will be used for non-standard tunnels.
> 
> The support from the device is for inner and outer checksums on
> IPV4/TCP/UDP and TSO for *any packet with the following format*:
> 
> <some headers> / [optional IPv4/IPv6] / [optional TCP/UDP] / <some
> headers> / [optional inner IPv4/IPv6] / [optional TCP/UDP]
> 
> For example the following packets can use this feature:
> 
> 1. eth / ipv4 / udp / VXLAN / ip / tcp
> 2. eth / ipv4 / GRE / MPLS / ipv4 / udp
> 
> Please note that specific tunnel headers that contain payload length,
> sequence id or checksum will not be updated.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

It is getting messier! [1]

Hi Thomas,

Any suggestion on how to manage these rte_flow patches, we are late and they
aren't settle down yet. There are some level of dependency and there are some
uncertainty in some of the dependent patches because of ABI/API process.

It would be great to get them incremental or have a plan to how to proceed.


[1]
Previous version in this thread is following patches:
[PATCH v4 1/2] ethdev: add supported hash function check
[PATCH v4 2/2] app/testpmd: new parameter for port config all rss command

And this set is:
[PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO
[PATCH v5 2/2] app/testpmd: testpmd support Tx generic tunnel offloads

But there is already v5 send for this set and in other thread there is v7 of it:
[PATCH v7 0/2] support Tx generic tunnel checksum and TSO
[PATCH v7 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO
[PATCH v7 2/2] app/testpmd: testpmd support Tx generic tunnel offloads

Most probably you have intended to send another patchset here.

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO

[Ferruh Yigit]

On 4/20/2018 2:48 PM, Ferruh Yigit wrote:
> On 4/20/2018 2:06 PM, Xueming Li wrote:
>> This patch introduce new TX offload flags for device that supports
>> IP or UDP tunneled packet L3/L4 checksum and TSO offload.
>> It will be used for non-standard tunnels.
>>
>> The support from the device is for inner and outer checksums on
>> IPV4/TCP/UDP and TSO for *any packet with the following format*:
>>
>> <some headers> / [optional IPv4/IPv6] / [optional TCP/UDP] / <some
>> headers> / [optional inner IPv4/IPv6] / [optional TCP/UDP]
>>
>> For example the following packets can use this feature:
>>
>> 1. eth / ipv4 / udp / VXLAN / ip / tcp
>> 2. eth / ipv4 / GRE / MPLS / ipv4 / udp
>>
>> Please note that specific tunnel headers that contain payload length,
>> sequence id or checksum will not be updated.
>>
>> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> It is getting messier! [1]
> 
> Hi Thomas,
> 
> Any suggestion on how to manage these rte_flow patches, we are late and they
> aren't settle down yet. There are some level of dependency and there are some
> uncertainty in some of the dependent patches because of ABI/API process.
> 
> It would be great to get them incremental or have a plan to how to proceed.

Involved parties looks like following:

Xueming: Mellanox offloads for tunnel protocols.
Adrien: rte_flow improvements for rss?
Qi: rte_flow more protocol support?
Declan/Awal: TEP, port representor, using rte_flow?
Andrew: sfc PMD updates on top of rte_flow changes.

Gaetan: devargs, previously devargs dependency mentioned for some of above, not
sure that is still the case.

I am not clear of latest status above patches and their dependency with
eachother, are they in sync or not, it can be nice who know clarifies more.

> 
> 
> [1]
> Previous version in this thread is following patches:
> [PATCH v4 1/2] ethdev: add supported hash function check
> [PATCH v4 2/2] app/testpmd: new parameter for port config all rss command
> 
> And this set is:
> [PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO
> [PATCH v5 2/2] app/testpmd: testpmd support Tx generic tunnel offloads
> 
> But there is already v5 send for this set and in other thread there is v7 of it:
> [PATCH v7 0/2] support Tx generic tunnel checksum and TSO
> [PATCH v7 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO
> [PATCH v7 2/2] app/testpmd: testpmd support Tx generic tunnel offloads
> 
> Most probably you have intended to send another patchset here.
> 

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO

[Xueming(Steven) Li]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, April 20, 2018 9:48 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>; Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO
> 
> On 4/20/2018 2:06 PM, Xueming Li wrote:
> > This patch introduce new TX offload flags for device that supports IP
> > or UDP tunneled packet L3/L4 checksum and TSO offload.
> > It will be used for non-standard tunnels.
> >
> > The support from the device is for inner and outer checksums on
> > IPV4/TCP/UDP and TSO for *any packet with the following format*:
> >
> > <some headers> / [optional IPv4/IPv6] / [optional TCP/UDP] / <some
> > headers> / [optional inner IPv4/IPv6] / [optional TCP/UDP]
> >
> > For example the following packets can use this feature:
> >
> > 1. eth / ipv4 / udp / VXLAN / ip / tcp 2. eth / ipv4 / GRE / MPLS /
> > ipv4 / udp
> >
> > Please note that specific tunnel headers that contain payload length,
> > sequence id or checksum will not be updated.
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> It is getting messier! [1]
> 
> Hi Thomas,
> 
> Any suggestion on how to manage these rte_flow patches, we are late and they aren't settle down yet.
> There are some level of dependency and there are some uncertainty in some of the dependent patches
> because of ABI/API process.
> 
> It would be great to get them incremental or have a plan to how to proceed.
> 
> 
> [1]
> Previous version in this thread is following patches:
> [PATCH v4 1/2] ethdev: add supported hash function check [PATCH v4 2/2] app/testpmd: new parameter for
> port config all rss command
> 
> And this set is:
> [PATCH v5 1/2] ethdev: introduce generic IP/UDP tunnel checksum and TSO [PATCH v5 2/2] app/testpmd:
> testpmd support Tx generic tunnel offloads

My bad, I was using wrong branch to format v5 of this thread, will resend, sorry for confusion.

> 
> But there is already v5 send for this set and in other thread there is v7 of it:
> [PATCH v7 0/2] support Tx generic tunnel checksum and TSO [PATCH v7 1/2] ethdev: introduce generic
> IP/UDP tunnel checksum and TSO [PATCH v7 2/2] app/testpmd: testpmd support Tx generic tunnel offloads
> 
> Most probably you have intended to send another patchset here.

Correct.

[dpdk-dev] [PATCH v5 2/2] app/testpmd: testpmd support Tx generic tunnel offloads

[Xueming Li]

"show port cap" and "csum parse tunnel" command support TX generic
tunnel offloads

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 app/test-pmd/cmdline.c | 14 ++++++++++++--
 app/test-pmd/config.c  | 17 +++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 512e3b55e..4ec3dab56 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4157,6 +4157,12 @@ check_tunnel_tso_nic_support(portid_t port_id)
 	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GENEVE_TNL_TSO))
 		printf("Warning: GENEVE TUNNEL TSO not supported therefore "
 		       "not enabled for port %d\n", port_id);
+	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IP_TNL_TSO))
+		printf("Warning: IP TUNNEL TSO not supported therefore "
+		       "not enabled for port %d\n", port_id);
+	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_UDP_TNL_TSO))
+		printf("Warning: UDP TUNNEL TSO not supported therefore "
+		       "not enabled for port %d\n", port_id);
 	return dev_info;
 }
 
@@ -4184,13 +4190,17 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 			~(DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
 			  DEV_TX_OFFLOAD_GRE_TNL_TSO |
 			  DEV_TX_OFFLOAD_IPIP_TNL_TSO |
-			  DEV_TX_OFFLOAD_GENEVE_TNL_TSO);
+			  DEV_TX_OFFLOAD_GENEVE_TNL_TSO |
+			  DEV_TX_OFFLOAD_IP_TNL_TSO |
+			  DEV_TX_OFFLOAD_UDP_TNL_TSO);
 		printf("TSO for tunneled packets is disabled\n");
 	} else {
 		uint64_t tso_offloads = (DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
 					 DEV_TX_OFFLOAD_GRE_TNL_TSO |
 					 DEV_TX_OFFLOAD_IPIP_TNL_TSO |
-					 DEV_TX_OFFLOAD_GENEVE_TNL_TSO);
+					 DEV_TX_OFFLOAD_GENEVE_TNL_TSO |
+					 DEV_TX_OFFLOAD_IP_TNL_TSO |
+					 DEV_TX_OFFLOAD_UDP_TNL_TSO);
 
 		ports[res->port_id].dev_conf.txmode.offloads |=
 			(tso_offloads & dev_info.tx_offload_capa);
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index dd051f5ca..f329a8810 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -722,6 +722,23 @@ port_offload_cap_display(portid_t port_id)
 			printf("off\n");
 	}
 
+	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IP_TNL_TSO) {
+		printf("IP tunnel TSO:  ");
+		if (ports[port_id].dev_conf.txmode.offloads &
+		    DEV_TX_OFFLOAD_IP_TNL_TSO)
+			printf("on\n");
+		else
+			printf("off\n");
+	}
+
+	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_UDP_TNL_TSO) {
+		printf("UDP tunnel TSO:  ");
+		if (ports[port_id].dev_conf.txmode.offloads &
+		    DEV_TX_OFFLOAD_UDP_TNL_TSO)
+			printf("on\n");
+		else
+			printf("off\n");
+	}
 }
 
 int
-- 
2.13.3

Re: [dpdk-dev] [PATCH v5 2/2] app/testpmd: testpmd support Tx generic tunnel offloads

[Xueming(Steven) Li]

Wrong patch, please ignore this one.

> -----Original Message-----
> From: Xueming Li <xuemingl@mellanox.com>
> Sent: Friday, April 20, 2018 9:07 PM
> To: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Xueming(Steven) Li <xuemingl@mellanox.com>; dev@dpdk.org
> Subject: [PATCH v5 2/2] app/testpmd: testpmd support Tx generic tunnel offloads
> 
> "show port cap" and "csum parse tunnel" command support TX generic tunnel offloads
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> ---
>  app/test-pmd/cmdline.c | 14 ++++++++++++--  app/test-pmd/config.c  | 17 +++++++++++++++++
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index 512e3b55e..4ec3dab56 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -4157,6 +4157,12 @@ check_tunnel_tso_nic_support(portid_t port_id)
>  	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GENEVE_TNL_TSO))
>  		printf("Warning: GENEVE TUNNEL TSO not supported therefore "
>  		       "not enabled for port %d\n", port_id);
> +	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IP_TNL_TSO))
> +		printf("Warning: IP TUNNEL TSO not supported therefore "
> +		       "not enabled for port %d\n", port_id);
> +	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_UDP_TNL_TSO))
> +		printf("Warning: UDP TUNNEL TSO not supported therefore "
> +		       "not enabled for port %d\n", port_id);
>  	return dev_info;
>  }
> 
> @@ -4184,13 +4190,17 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>  			~(DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
>  			  DEV_TX_OFFLOAD_GRE_TNL_TSO |
>  			  DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> -			  DEV_TX_OFFLOAD_GENEVE_TNL_TSO);
> +			  DEV_TX_OFFLOAD_GENEVE_TNL_TSO |
> +			  DEV_TX_OFFLOAD_IP_TNL_TSO |
> +			  DEV_TX_OFFLOAD_UDP_TNL_TSO);
>  		printf("TSO for tunneled packets is disabled\n");
>  	} else {
>  		uint64_t tso_offloads = (DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
>  					 DEV_TX_OFFLOAD_GRE_TNL_TSO |
>  					 DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> -					 DEV_TX_OFFLOAD_GENEVE_TNL_TSO);
> +					 DEV_TX_OFFLOAD_GENEVE_TNL_TSO |
> +					 DEV_TX_OFFLOAD_IP_TNL_TSO |
> +					 DEV_TX_OFFLOAD_UDP_TNL_TSO);
> 
>  		ports[res->port_id].dev_conf.txmode.offloads |=
>  			(tso_offloads & dev_info.tx_offload_capa); diff --git a/app/test-pmd/config.c
> b/app/test-pmd/config.c index dd051f5ca..f329a8810 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -722,6 +722,23 @@ port_offload_cap_display(portid_t port_id)
>  			printf("off\n");
>  	}
> 
> +	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IP_TNL_TSO) {
> +		printf("IP tunnel TSO:  ");
> +		if (ports[port_id].dev_conf.txmode.offloads &
> +		    DEV_TX_OFFLOAD_IP_TNL_TSO)
> +			printf("on\n");
> +		else
> +			printf("off\n");
> +	}
> +
> +	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_UDP_TNL_TSO) {
> +		printf("UDP tunnel TSO:  ");
> +		if (ports[port_id].dev_conf.txmode.offloads &
> +		    DEV_TX_OFFLOAD_UDP_TNL_TSO)
> +			printf("on\n");
> +		else
> +			printf("off\n");
> +	}
>  }
> 
>  int
> --
> 2.13.3

[dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Xueming Li]

Add supported RSS hash function check in device configuration to
have better error verbosity for application developers.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 3c049ef43..7b1dda926 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1179,6 +1179,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 							ETHER_MAX_LEN;
 	}
 
+	/* Check that device supports requested rss hash functions. */
+	if ((dev_info.flow_type_rss_offloads |
+	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -2835,9 +2847,20 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
 			    struct rte_eth_rss_conf *rss_conf)
 {
 	struct rte_eth_dev *dev;
+	struct rte_eth_dev_info dev_info = { .flow_type_rss_offloads = 0, };
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
+	rte_eth_dev_info_get(port_id, &dev_info);
+	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
+	    dev_info.flow_type_rss_offloads) {
+		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
+				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
+				    port_id,
+				    rss_conf->rss_hf,
+				    dev_info.flow_type_rss_offloads);
+		return -EINVAL;
+	}
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_update, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
 								 rss_conf));
-- 
2.13.3

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Ferruh Yigit]

On 4/20/2018 3:30 PM, Xueming Li wrote:
> Add supported RSS hash function check in device configuration to
> have better error verbosity for application developers.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Hi Xueming,

Can you please update the patchwork to only keep latest versions of your sets?

Thanks,
ferruh

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Xueming(Steven) Li]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, April 20, 2018 10:35 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>; Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check
> 
> On 4/20/2018 3:30 PM, Xueming Li wrote:
> > Add supported RSS hash function check in device configuration to have
> > better error verbosity for application developers.
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Hi Xueming,
> 
> Can you please update the patchwork to only keep latest versions of your sets?

Done.

> 
> Thanks,
> ferruh

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Thomas Monjalon]

20/04/2018 16:30, Xueming Li:
> Add supported RSS hash function check in device configuration to
> have better error verbosity for application developers.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Ferruh Yigit]

On 4/23/2018 4:20 PM, Thomas Monjalon wrote:
> 20/04/2018 16:30, Xueming Li:
>> Add supported RSS hash function check in device configuration to
>> have better error verbosity for application developers.
>>
>> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Series applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Ferruh Yigit]

On 4/20/2018 3:30 PM, Xueming Li wrote:
> Add supported RSS hash function check in device configuration to
> have better error verbosity for application developers.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 3c049ef43..7b1dda926 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1179,6 +1179,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  							ETHER_MAX_LEN;
>  	}
>  
> +	/* Check that device supports requested rss hash functions. */
> +	if ((dev_info.flow_type_rss_offloads |
> +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> +	    dev_info.flow_type_rss_offloads) {
> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
> +				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
> +				    port_id,
> +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
> +				    dev_info.flow_type_rss_offloads);
> +		return -EINVAL;
> +	}
> +

Hi Thomas,

This can break the PMDs that are not setting flow_type_rss_offloads properly.
How can we highlight this so that PMD owners can double check?

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Thomas Monjalon]

23/04/2018 18:06, Ferruh Yigit:
> On 4/20/2018 3:30 PM, Xueming Li wrote:
> > Add supported RSS hash function check in device configuration to
> > have better error verbosity for application developers.
> > 
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >  
> > +	/* Check that device supports requested rss hash functions. */
> > +	if ((dev_info.flow_type_rss_offloads |
> > +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> > +	    dev_info.flow_type_rss_offloads) {
> > +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
> > +				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
> > +				    port_id,
> > +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
> > +				    dev_info.flow_type_rss_offloads);
> > +		return -EINVAL;
> > +	}
> 
> Hi Thomas,
> 
> This can break the PMDs that are not setting flow_type_rss_offloads properly.
> How can we highlight this so that PMD owners can double check?

Can we have a check-list in the RC1 announce email?

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Ferruh Yigit]

On 4/23/2018 7:14 PM, Thomas Monjalon wrote:
> 23/04/2018 18:06, Ferruh Yigit:
>> On 4/20/2018 3:30 PM, Xueming Li wrote:
>>> Add supported RSS hash function check in device configuration to
>>> have better error verbosity for application developers.
>>>
>>> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
>>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>  
>>> +	/* Check that device supports requested rss hash functions. */
>>> +	if ((dev_info.flow_type_rss_offloads |
>>> +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
>>> +	    dev_info.flow_type_rss_offloads) {
>>> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
>>> +				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
>>> +				    port_id,
>>> +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
>>> +				    dev_info.flow_type_rss_offloads);
>>> +		return -EINVAL;
>>> +	}
>>
>> Hi Thomas,
>>
>> This can break the PMDs that are not setting flow_type_rss_offloads properly.
>> How can we highlight this so that PMD owners can double check?
> 
> Can we have a check-list in the RC1 announce email?

Hi Thomas, Xueming,

This change is breaking multiple sample applications, testpmd was also broken
but already fixed by Qi [1].

Indeed this patch should update sample applications and testpmd as well when
doing an ethdev API update, also should update release notes "API Changes" section.

We can fix sample applications for rc2, but same thing also can hit users.

Or for this release we can remote returning error, instead update log message to
error. Next release add the return and change log message back to debug.

What do you think?


[1]
Commit: 9089296206ce ("app/testpmd: fix config due to RSS offload check")

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add supported hash function check

[Thomas Monjalon]

01/05/2018 13:04, Ferruh Yigit:
> On 4/23/2018 7:14 PM, Thomas Monjalon wrote:
> > 23/04/2018 18:06, Ferruh Yigit:
> >> On 4/20/2018 3:30 PM, Xueming Li wrote:
> >>> Add supported RSS hash function check in device configuration to
> >>> have better error verbosity for application developers.
> >>>
> >>> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> >>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >>>  
> >>> +	/* Check that device supports requested rss hash functions. */
> >>> +	if ((dev_info.flow_type_rss_offloads |
> >>> +	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
> >>> +	    dev_info.flow_type_rss_offloads) {
> >>> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d invalid rss_hf: "
> >>> +				    "0x%"PRIx64", valid value: 0x%"PRIx64"\n",
> >>> +				    port_id,
> >>> +				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
> >>> +				    dev_info.flow_type_rss_offloads);
> >>> +		return -EINVAL;
> >>> +	}
> >>
> >> Hi Thomas,
> >>
> >> This can break the PMDs that are not setting flow_type_rss_offloads properly.
> >> How can we highlight this so that PMD owners can double check?
> > 
> > Can we have a check-list in the RC1 announce email?
> 
> Hi Thomas, Xueming,
> 
> This change is breaking multiple sample applications, testpmd was also broken
> but already fixed by Qi [1].
> 
> Indeed this patch should update sample applications and testpmd as well when
> doing an ethdev API update, also should update release notes "API Changes" section.
> 
> We can fix sample applications for rc2, but same thing also can hit users.
> 
> Or for this release we can remote returning error, instead update log message to
> error. Next release add the return and change log message back to debug.
> 
> What do you think?

Yes:
1/ update the API doc and sample apps in 18.05
2/ send a deprecation notice
3/ add error return in 18.08

I've  replied to your patch too:
	http://dpdk.org/ml/archives/dev/2018-May/099865.html

[dpdk-dev] [PATCH v5 2/2] app/testpmd: new parameter for port config all rss command

[Xueming Li]

This patches add "default" parameter to "port config all rss" command.
"default" means all supported hash types reported by device info.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 app/test-pmd/cmdline.c                      | 13 +++++++++----
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 512e3b55e..77068129e 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -821,8 +821,8 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"    Set crc-strip/scatter/rx-checksum/hardware-vlan/drop_en"
 			" for ports.\n\n"
 
-			"port config all rss (all|ip|tcp|udp|sctp|ether|port|vxlan|"
-			"geneve|nvgre|none|<flowtype_id>)\n"
+			"port config all rss (all|default|ip|tcp|udp|sctp|"
+			"ether|port|vxlan|geneve|nvgre|none|<flowtype_id>)\n"
 			"    Set the RSS mode.\n\n"
 
 			"port config port-id rss reta (hash,queue)[,(hash,queue)]\n"
@@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result,
 {
 	struct cmd_config_rss *res = parsed_result;
 	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
+	struct rte_eth_dev_info dev_info = { .flow_type_rss_offloads = 0, };
 	int diag;
 	uint8_t i;
 
@@ -2023,7 +2024,7 @@ cmd_config_rss_parsed(void *parsed_result,
 		rss_conf.rss_hf = ETH_RSS_GENEVE;
 	else if (!strcmp(res->value, "nvgre"))
 		rss_conf.rss_hf = ETH_RSS_NVGRE;
-	else if (!strcmp(res->value, "none"))
+	else if (!strcmp(res->value, "none") || !strcmp(res->value, "default"))
 		rss_conf.rss_hf = 0;
 	else if (isdigit(res->value[0]) && atoi(res->value) > 0 &&
 						atoi(res->value) < 64)
@@ -2034,6 +2035,10 @@ cmd_config_rss_parsed(void *parsed_result,
 	}
 	rss_conf.rss_key = NULL;
 	for (i = 0; i < rte_eth_dev_count(); i++) {
+		if (!strcmp(res->value, "default")) {
+			rte_eth_dev_info_get(i, &dev_info);
+			rss_conf.rss_hf = dev_info.flow_type_rss_offloads;
+		}
 		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
 		if (diag < 0)
 			printf("Configuration of RSS hash at ethernet port %d "
@@ -2057,7 +2062,7 @@ cmdline_parse_inst_t cmd_config_rss = {
 	.f = cmd_config_rss_parsed,
 	.data = NULL,
 	.help_str = "port config all rss "
-		"all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|<flowtype_id>",
+		"all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|<flowtype_id>",
 	.tokens = {
 		(void *)&cmd_config_rss_port,
 		(void *)&cmd_config_rss_keyword,
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index a766ac795..0e1719d9a 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1751,10 +1751,12 @@ port config - RSS
 
 Set the RSS (Receive Side Scaling) mode on or off::
 
-   testpmd> port config all rss (all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none)
+   testpmd> port config all rss (all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none)
 
 RSS is on by default.
 
+The ``all`` option is equivalent to ip|tcp|udp|sctp|ether.
+The ``default`` option enables all supported RSS types reported by device info.
 The ``none`` option is equivalent to the ``--disable-rss`` command-line option.
 
 port config - RSS Reta
-- 
2.13.3

[dpdk-dev] [PATCH v1 2/2] app/testpmd: config all supported RSS functions

[Xueming Li]

Only configure RSS hash functions supported by the device.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
 app/test-pmd/cmdline.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad7e..c41dd71ce 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1879,6 +1879,7 @@ cmd_config_rss_parsed(void *parsed_result,
 {
 	struct cmd_config_rss *res = parsed_result;
 	struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, };
+	struct rte_eth_dev_info dev_info = {0};
 	int diag;
 	uint8_t i;
 
@@ -1915,6 +1916,11 @@ cmd_config_rss_parsed(void *parsed_result,
 	}
 	rss_conf.rss_key = NULL;
 	for (i = 0; i < rte_eth_dev_count(); i++) {
+		if (!strcmp(res->value, "all")) {
+			rte_eth_dev_info_get(i, &dev_info);
+			if (dev_info.flow_type_rss_offloads)
+			    rss_conf.rss_hf = dev_info.flow_type_rss_offloads;
+		}
 		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
 		if (diag < 0)
 			printf("Configuration of RSS hash at ethernet port %d "
-- 
2.13.3

Re: [dpdk-dev] [PATCH v1 2/2] app/testpmd: config all supported RSS functions

[Thomas Monjalon]

09/04/2018 14:10, Xueming Li:
> Only configure RSS hash functions supported by the device.
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>

This commit message is too short.
Please explain what was the behaviour, and why you change it.