diff mbox

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

Message ID 20180420143023.117071-1-xuemingl@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Xueming Li April 20, 2018, 2:30 p.m. UTC 
  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(+)

Comments

Ferruh Yigit April 20, 2018, 2:35 p.m. UTC | #1 
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
Xueming Li April 20, 2018, 2:44 p.m. UTC | #2 
> -----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
Thomas Monjalon April 23, 2018, 3:20 p.m. UTC | #3 
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>
Ferruh Yigit April 23, 2018, 4:06 p.m. UTC | #4 
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?
Ferruh Yigit April 23, 2018, 4:07 p.m. UTC | #5 
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.
Thomas Monjalon April 23, 2018, 6:14 p.m. UTC | #6 
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?
Ferruh Yigit May 1, 2018, 11:04 a.m. UTC | #7 
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")
Thomas Monjalon May 1, 2018, 2:04 p.m. UTC | #8 
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
diff mbox

Patch

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