[dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Ferruh Yigit]

Many sample applications fail because of
dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()

The sample applications need to be fixed/updated before returning error
on rte_eth_dev_configure()

This patch keeps the error log but removes returning error.

Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
Cc: xuemingl@mellanox.com

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 59810dde8..5a67e6a7d 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1148,7 +1148,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 				    port_id,
 				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
 				    dev_info.flow_type_rss_offloads);
-		return -EINVAL;
 	}
 
 	/*
-- 
2.14.3

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Xueming(Steven) Li]

Looks good, thanks.

How do you think rss_hf, normally people think it best effort.

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, May 1, 2018 9:34 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@mellanox.com>
> Subject: [PATCH] ethdev: fix applications failure on configure
> 
> Many sample applications fail because of dev_info.flow_type_rss_offloads check in
> rte_eth_dev_configure()
> 
> The sample applications need to be fixed/updated before returning error on rte_eth_dev_configure()
> 
> This patch keeps the error log but removes returning error.
> 
> Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
> Cc: xuemingl@mellanox.com
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index
> 59810dde8..5a67e6a7d 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1148,7 +1148,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  				    port_id,
>  				    dev_conf->rx_adv_conf.rss_conf.rss_hf,
>  				    dev_info.flow_type_rss_offloads);
> -		return -EINVAL;
>  	}
> 
>  	/*
> --
> 2.14.3

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Thomas Monjalon]

01/05/2018 15:33, Ferruh Yigit:
> Many sample applications fail because of
> dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()

We need to define the API behaviour in doxygen.

> The sample applications need to be fixed/updated before returning error
> on rte_eth_dev_configure()
> 
> This patch keeps the error log but removes returning error.

If the doc is updated in 18.05, we can have a deprecation notice
to insert the error return in 18.08.

> Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
> Cc: xuemingl@mellanox.com
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Ferruh Yigit]

On 5/1/2018 3:01 PM, Thomas Monjalon wrote:
> 01/05/2018 15:33, Ferruh Yigit:
>> Many sample applications fail because of
>> dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()
> 
> We need to define the API behaviour in doxygen.
> 
>> The sample applications need to be fixed/updated before returning error
>> on rte_eth_dev_configure()
>>
>> This patch keeps the error log but removes returning error.
> 
> If the doc is updated in 18.05, we can have a deprecation notice
> to insert the error return in 18.08.

+1 to add a deprecation notice for this release and do the error return on 18.08.

Is the doc update you mentioned doxygen update?

Who can do doc update and deprecation notice patches for this release?

> 
>> Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
>> Cc: xuemingl@mellanox.com
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Thomas Monjalon]

01/05/2018 16:08, Ferruh Yigit:
> On 5/1/2018 3:01 PM, Thomas Monjalon wrote:
> > 01/05/2018 15:33, Ferruh Yigit:
> >> Many sample applications fail because of
> >> dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()
> > 
> > We need to define the API behaviour in doxygen.
> > 
> >> The sample applications need to be fixed/updated before returning error
> >> on rte_eth_dev_configure()
> >>
> >> This patch keeps the error log but removes returning error.
> > 
> > If the doc is updated in 18.05, we can have a deprecation notice
> > to insert the error return in 18.08.
> 
> +1 to add a deprecation notice for this release and do the error return on 18.08.
> 
> Is the doc update you mentioned doxygen update?

Yes, doxygen update.

> Who can do doc update and deprecation notice patches for this release?

Either you Ferruh, or Xueming and me together.
What do you prefer?

> >> Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
> >> Cc: xuemingl@mellanox.com
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Ferruh Yigit]

On 5/1/2018 3:12 PM, Thomas Monjalon wrote:
> 01/05/2018 16:08, Ferruh Yigit:
>> On 5/1/2018 3:01 PM, Thomas Monjalon wrote:
>>> 01/05/2018 15:33, Ferruh Yigit:
>>>> Many sample applications fail because of
>>>> dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()
>>>
>>> We need to define the API behaviour in doxygen.
>>>
>>>> The sample applications need to be fixed/updated before returning error
>>>> on rte_eth_dev_configure()
>>>>
>>>> This patch keeps the error log but removes returning error.
>>>
>>> If the doc is updated in 18.05, we can have a deprecation notice
>>> to insert the error return in 18.08.
>>
>> +1 to add a deprecation notice for this release and do the error return on 18.08.
>>
>> Is the doc update you mentioned doxygen update?
> 
> Yes, doxygen update.
> 
>> Who can do doc update and deprecation notice patches for this release?
> 
> Either you Ferruh, or Xueming and me together.
> What do you prefer?

Or as Xueming suggested, we can take rss_hf config as best effort and not return
error at all.

I think this forces PMDs to have up-to-date flow_type_rss_offloads values, is
there any other benefit?
What was the initial motivation to add error return on this check?

> 
>>>> Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
>>>> Cc: xuemingl@mellanox.com
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> 
> 

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Thomas Monjalon]

01/05/2018 15:33, Ferruh Yigit:
> Many sample applications fail because of
> dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()
> 
> The sample applications need to be fixed/updated before returning error
> on rte_eth_dev_configure()
> 
> This patch keeps the error log but removes returning error.
> 
> Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
> Cc: xuemingl@mellanox.com
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks

We'll decide later whether we need to return an error in 18.08 or not.

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Thomas Monjalon]

01/05/2018 17:30, Thomas Monjalon:
> 01/05/2018 15:33, Ferruh Yigit:
> > Many sample applications fail because of
> > dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()
> > 
> > The sample applications need to be fixed/updated before returning error
> > on rte_eth_dev_configure()
> > 
> > This patch keeps the error log but removes returning error.
> > 
> > Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
> > Cc: xuemingl@mellanox.com
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Applied, thanks

I've also removed the error return for rte_eth_dev_rss_hash_update
which was introduced by the same above patch.

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

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Xueming(Steven) Li]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, May 1, 2018 10:32 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Xueming(Steven) Li <xuemingl@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH] ethdev: fix applications failure on configure
> 
> On 5/1/2018 3:12 PM, Thomas Monjalon wrote:
> > 01/05/2018 16:08, Ferruh Yigit:
> >> On 5/1/2018 3:01 PM, Thomas Monjalon wrote:
> >>> 01/05/2018 15:33, Ferruh Yigit:
> >>>> Many sample applications fail because of
> >>>> dev_info.flow_type_rss_offloads check in rte_eth_dev_configure()
> >>>
> >>> We need to define the API behaviour in doxygen.
> >>>
> >>>> The sample applications need to be fixed/updated before returning
> >>>> error on rte_eth_dev_configure()
> >>>>
> >>>> This patch keeps the error log but removes returning error.
> >>>
> >>> If the doc is updated in 18.05, we can have a deprecation notice to
> >>> insert the error return in 18.08.
> >>
> >> +1 to add a deprecation notice for this release and do the error return on 18.08.
> >>
> >> Is the doc update you mentioned doxygen update?
> >
> > Yes, doxygen update.
> >
> >> Who can do doc update and deprecation notice patches for this release?
> >
> > Either you Ferruh, or Xueming and me together.
> > What do you prefer?
> 
> Or as Xueming suggested, we can take rss_hf config as best effort and not return error at all.
> 
> I think this forces PMDs to have up-to-date flow_type_rss_offloads values, is there any other benefit?
> What was the initial motivation to add error return on this check?

The original idea is to add rss_hf check on mlx5 PMD, while it looks more generic
to move the check to ethdev api from discussion[1].

[1] http://www.dpdk.org/ml/archives/dev/2018-April/095136.html

> 
> >
> >>>> Fixes: 8863a1fbfc66 ("ethdev: add supported hash function check")
> >>>> Cc: xuemingl@mellanox.com
> >>>>
> >>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >
> >
> >

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Thomas Monjalon]

02/05/2018 11:58, Xueming(Steven) Li:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > Or as Xueming suggested, we can take rss_hf config as best effort and not return error at all.
> > 
> > I think this forces PMDs to have up-to-date flow_type_rss_offloads values, is there any other benefit?
> > What was the initial motivation to add error return on this check?
> 
> The original idea is to add rss_hf check on mlx5 PMD, while it looks more generic
> to move the check to ethdev api from discussion[1].
> 
> [1] http://www.dpdk.org/ml/archives/dev/2018-April/095136.html

I think it is not correct to not return an error when the app
request an offload which is not supported.

Do we agree to work on PMDs and applications to fix this offload compliance?
And submit a deprecation notice to return an error in a later release?

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Shahaf Shuler]

Wednesday, May 2, 2018 1:06 PM, Thomas Monjalon:
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure
> 
> 02/05/2018 11:58, Xueming(Steven) Li:
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > Or as Xueming suggested, we can take rss_hf config as best effort and
> not return error at all.
> > >
> > > I think this forces PMDs to have up-to-date flow_type_rss_offloads
> values, is there any other benefit?
> > > What was the initial motivation to add error return on this check?
> >
> > The original idea is to add rss_hf check on mlx5 PMD, while it looks
> > more generic to move the check to ethdev api from discussion[1].
> >
> > [1]
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.
> > dpdk.org%2Fml%2Farchives%2Fdev%2F2018-
> April%2F095136.html&data=02%7C01
> >
> %7Cshahafs%40mellanox.com%7Cc773c15dcbda49d21da008d5b0145864%7C
> a652971
> >
> c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636608523825745897&sdata=jDU
> OyU0E%
> > 2FY5g3oSKBQxUsz8Ehr%2FN3ek5h8kotQBLZBU%3D&reserved=0
> 
> I think it is not correct to not return an error when the app request an offload
> which is not supported.
> 
> Do we agree to work on PMDs and applications to fix this offload
> compliance?
> And submit a deprecation notice to return an error in a later release?

I probably missed it but why deprecation notice is required? 
Per my understanding it is just a fix to enforce better the API which is:
1. application reads capabilities 
2. application sets offloads accordingly. 


> 

Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure

[Ferruh Yigit]

On 5/2/2018 6:09 PM, Shahaf Shuler wrote:
> Wednesday, May 2, 2018 1:06 PM, Thomas Monjalon:
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix applications failure on configure
>>
>> 02/05/2018 11:58, Xueming(Steven) Li:
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Or as Xueming suggested, we can take rss_hf config as best effort and
>> not return error at all.
>>>>
>>>> I think this forces PMDs to have up-to-date flow_type_rss_offloads
>> values, is there any other benefit?
>>>> What was the initial motivation to add error return on this check?
>>>
>>> The original idea is to add rss_hf check on mlx5 PMD, while it looks
>>> more generic to move the check to ethdev api from discussion[1].
>>>
>>> [1]
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
>> w.
>>> dpdk.org%2Fml%2Farchives%2Fdev%2F2018-
>> April%2F095136.html&data=02%7C01
>>>
>> %7Cshahafs%40mellanox.com%7Cc773c15dcbda49d21da008d5b0145864%7C
>> a652971
>>>
>> c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636608523825745897&sdata=jDU
>> OyU0E%
>>> 2FY5g3oSKBQxUsz8Ehr%2FN3ek5h8kotQBLZBU%3D&reserved=0
>>
>> I think it is not correct to not return an error when the app request an offload
>> which is not supported.
>>
>> Do we agree to work on PMDs and applications to fix this offload
>> compliance?
>> And submit a deprecation notice to return an error in a later release?
> 
> I probably missed it but why deprecation notice is required? 
> Per my understanding it is just a fix to enforce better the API which is:
> 1. application reads capabilities 
> 2. application sets offloads accordingly. 

The return error on some values is new, and this is a behavior change in API.

Some applications working fine will start throwing error, and for DPDK
application developers this may mean go and update their application.

For possible application update I believe the change should be announced in
deprecation notice in advance.