[dpdk-dev,v5,1/2] ethdev: add supported hash function check
Checks
Commit Message
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
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
> -----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
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>
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?
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.
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?
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")
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
@@ -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));