[PATCH] ethdev: force offloading API rules

[PATCH] ethdev: force offloading API rules

[Ferruh Yigit]

The error path was disabled in previous release to let apps to be more
flexible.

But this release they are enabled, applications have to obey offload API
rules otherwise they will get errors from following APIs:
rte_eth_dev_configure
rte_eth_rx_queue_setup
rte_eth_tx_queue_setup

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Shahaf Shuler <shahafs@mellanox.com>
Cc: Wei Dai <wei.dai@intel.com>
Cc: Qi Zhang <qi.z.zhang@intel.com>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index cd4bfd3c6..66e311676 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1171,7 +1171,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 				local_conf.rxmode.offloads,
 				dev_info.rx_offload_capa,
 				__func__);
-		/* Will return -EINVAL in the next release */
+		return -EINVAL;
 	}
 	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
 	     local_conf.txmode.offloads) {
@@ -1182,7 +1182,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 				local_conf.txmode.offloads,
 				dev_info.tx_offload_capa,
 				__func__);
-		/* Will return -EINVAL in the next release */
+		return -EINVAL;
 	}
 
 	/* Check that device supports requested rss hash functions. */
@@ -1580,7 +1580,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 				local_conf.offloads,
 				dev_info.rx_queue_offload_capa,
 				__func__);
-		/* Will return -EINVAL in the next release */
+		return -EINVAL;
 	}
 
 	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
@@ -1745,7 +1745,7 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 				local_conf.offloads,
 				dev_info.tx_queue_offload_capa,
 				__func__);
-		/* Will return -EINVAL in the next release */
+		return -EINVAL;
 	}
 
 	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
-- 
2.14.3

Re: [PATCH] ethdev: force offloading API rules

[Ferruh Yigit]

On 5/31/2018 1:44 PM, Ferruh Yigit wrote:
> The error path was disabled in previous release to let apps to be more
> flexible.
> 
> But this release they are enabled, applications have to obey offload API
> rules otherwise they will get errors from following APIs:
> rte_eth_dev_configure
> rte_eth_rx_queue_setup
> rte_eth_tx_queue_setup
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Shahaf Shuler <shahafs@mellanox.com>
> Cc: Wei Dai <wei.dai@intel.com>
> Cc: Qi Zhang <qi.z.zhang@intel.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>

Any objection to this patch?
I really would like to get this early to catch any possible issue.

Re: [PATCH] ethdev: force offloading API rules

[Thomas Monjalon]

08/06/2018 21:51, Ferruh Yigit:
> On 5/31/2018 1:44 PM, Ferruh Yigit wrote:
> > The error path was disabled in previous release to let apps to be more
> > flexible.
> > 
> > But this release they are enabled, applications have to obey offload API
> > rules otherwise they will get errors from following APIs:
> > rte_eth_dev_configure
> > rte_eth_rx_queue_setup
> > rte_eth_tx_queue_setup
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> > Cc: Shahaf Shuler <shahafs@mellanox.com>
> > Cc: Wei Dai <wei.dai@intel.com>
> > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> Any objection to this patch?
> I really would like to get this early to catch any possible issue.

Yes you're right.
Let's merge it.

Re: [PATCH] ethdev: force offloading API rules

[Stephen Hemminger]

On Thu, 31 May 2018 13:44:30 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> The error path was disabled in previous release to let apps to be more
> flexible.
> 
> But this release they are enabled, applications have to obey offload API
> rules otherwise they will get errors from following APIs:
> rte_eth_dev_configure
> rte_eth_rx_queue_setup
> rte_eth_tx_queue_setup
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Rather than always returning -EINVAL, why not propagate error code that the driver
returns. This would allow driver to give a more detailed error return.

The problem with that is you probably need to do a review of each drivers
configure and setup routines to see what they return on error (if any).

Re: [PATCH] ethdev: force offloading API rules

[Andrew Rybchenko]

On 06/08/2018 10:51 PM, Ferruh Yigit wrote:
> On 5/31/2018 1:44 PM, Ferruh Yigit wrote:
>> The error path was disabled in previous release to let apps to be more
>> flexible.
>>
>> But this release they are enabled, applications have to obey offload API
>> rules otherwise they will get errors from following APIs:
>> rte_eth_dev_configure
>> rte_eth_rx_queue_setup
>> rte_eth_tx_queue_setup
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> Cc: Shahaf Shuler <shahafs@mellanox.com>
>> Cc: Wei Dai <wei.dai@intel.com>
>> Cc: Qi Zhang <qi.z.zhang@intel.com>
>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> Any objection to this patch?
> I really would like to get this early to catch any possible issue.

I think it should be pushed just after old offload API removal since
if it is pushed before, it should take old offload API into account in
Tx queue setup function.

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [PATCH] ethdev: force offloading API rules

[Shahaf Shuler]

Friday, June 8, 2018 11:02 PM, Stephen Hemminger:
> Subject: Re: [dpdk-dev] [PATCH] ethdev: force offloading API rules
> 
> On Thu, 31 May 2018 13:44:30 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > The error path was disabled in previous release to let apps to be more
> > flexible.
> >
> > But this release they are enabled, applications have to obey offload
> > API rules otherwise they will get errors from following APIs:
> > rte_eth_dev_configure
> > rte_eth_rx_queue_setup
> > rte_eth_tx_queue_setup
> >
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

I am OK with returning EINVAL in case of configuration error.

> 
> Rather than always returning -EINVAL, why not propagate error code that
> the driver returns. This would allow driver to give a more detailed error
> return.
> 
> The problem with that is you probably need to do a review of each drivers
> configure and setup routines to see what they return on error (if any).

Per my understanding the error is in-depended on driver specific implementation, it is rather to enforce the offloading API rules of ethdev layer, which are:
1. PMD reports offloads capabilities
2. application sets it's offloads based on the supported capabilities.   

When the configuration fail, no need to dig into driver, just to look on the error log and see which unsupported offloads were set. 

Re: [PATCH] ethdev: force offloading API rules

[Thomas Monjalon]

08/06/2018 22:01, Thomas Monjalon:
> 08/06/2018 21:51, Ferruh Yigit:
> > On 5/31/2018 1:44 PM, Ferruh Yigit wrote:
> > > The error path was disabled in previous release to let apps to be more
> > > flexible.
> > > 
> > > But this release they are enabled, applications have to obey offload API
> > > rules otherwise they will get errors from following APIs:
> > > rte_eth_dev_configure
> > > rte_eth_rx_queue_setup
> > > rte_eth_tx_queue_setup
> > > 
> > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > ---
> > > Cc: Shahaf Shuler <shahafs@mellanox.com>
> > > Cc: Wei Dai <wei.dai@intel.com>
> > > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > > Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> > 
> > Any objection to this patch?
> > I really would like to get this early to catch any possible issue.
> 
> Yes you're right.
> Let's merge it.

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

Re: [PATCH] ethdev: force offloading API rules

[Ferruh Yigit]

On 6/13/2018 4:16 PM, Thomas Monjalon wrote:
> 08/06/2018 22:01, Thomas Monjalon:
>> 08/06/2018 21:51, Ferruh Yigit:
>>> On 5/31/2018 1:44 PM, Ferruh Yigit wrote:
>>>> The error path was disabled in previous release to let apps to be more
>>>> flexible.
>>>>
>>>> But this release they are enabled, applications have to obey offload API
>>>> rules otherwise they will get errors from following APIs:
>>>> rte_eth_dev_configure
>>>> rte_eth_rx_queue_setup
>>>> rte_eth_tx_queue_setup
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>>> Cc: Shahaf Shuler <shahafs@mellanox.com>
>>>> Cc: Wei Dai <wei.dai@intel.com>
>>>> Cc: Qi Zhang <qi.z.zhang@intel.com>
>>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>>>
>>> Any objection to this patch?
>>> I really would like to get this early to catch any possible issue.
>>
>> Yes you're right.
>> Let's merge it.
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

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

Re: [PATCH] ethdev: force offloading API rules

[Ferruh Yigit]

On 6/27/2018 9:31 AM, Ido Goshen wrote:
> I guess the error below relates to f52f1a6 ethdev: force offloading API rules

Hi Ido,

Yes it is related.

This specific error is related to the requested DEV_RX_OFFLOAD_CRC_STRIP, and
following patch should fix it because it adds to CRC strip capability to virtual
drivers:
https://patches.dpdk.org/patch/41359/

But perhaps we should also add some checks in examples as done in RSS check:
https://patches.dpdk.org/patch/41584/

There are a few pieces are moving, making it hard to manage, all these errors
should be detected and fixed by end of this week. Changes:
- We are removing old offload API and structus
- We are making ethdev more strict about what application request about offloads
and rss_hf
- We are in the process of deprecating CRC_STRIP flag


> 
>  
> 
> cgs@ubuntu:~/dpdk-next-net$ sudo examples/l2fwd/build/l2fwd -c 3 -n1 --no-huge
> --vdev=eth_pcap0,iface=dummy0 --vdev=eth_pcap1,iface=dummy1  -- -p 3 -T 1
> 
> EAL: Detected 4 lcore(s)
> 
> EAL: Detected 1 NUMA nodes
> 
> EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> 
> EAL: Probing VFIO support...
> 
> EAL: Started without hugepages support, physical addresses not available
> 
> EAL: PCI device 0000:02:01.0 on NUMA socket -1
> 
> EAL:   Invalid NUMA socket, default to 0
> 
> EAL:   probe driver: 8086:100f net_e1000_em
> 
> EAL: PCI device 0000:02:06.0 on NUMA socket -1
> 
> EAL:   Invalid NUMA socket, default to 0
> 
> EAL:   probe driver: 8086:100f net_e1000_em
> 
> MAC updating enabled
> 
> Lcore 0: RX port 0
> 
> Lcore 1: RX port 1
> 
> Initializing port 0... ethdev port_id=0 requested Rx offloads 0x1000 doesn't
> match Rx offloads capabilities 0x0 in rte_eth_dev_configure()
> 
>  
> 
> EAL: Error - exiting with code: 1
> 
>   Cause: Cannot configure device: err=-22, port=0
> 
>  
> 
> Please advice
> 

[PATCH] ethdev: force offloading API rules

[Ido Goshen]

I guess the error below relates to f52f1a6 ethdev: force offloading API rules

cgs@ubuntu:~/dpdk-next-net$ sudo examples/l2fwd/build/l2fwd -c 3 -n1 --no-huge --vdev=eth_pcap0,iface=dummy0 --vdev=eth_pcap1,iface=dummy1  -- -p 3 -T 1
EAL: Detected 4 lcore(s)
EAL: Detected 1 NUMA nodes
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Probing VFIO support...
EAL: Started without hugepages support, physical addresses not available
EAL: PCI device 0000:02:01.0 on NUMA socket -1
EAL:   Invalid NUMA socket, default to 0
EAL:   probe driver: 8086:100f net_e1000_em
EAL: PCI device 0000:02:06.0 on NUMA socket -1
EAL:   Invalid NUMA socket, default to 0
EAL:   probe driver: 8086:100f net_e1000_em
MAC updating enabled
Lcore 0: RX port 0
Lcore 1: RX port 1
Initializing port 0... ethdev port_id=0 requested Rx offloads 0x1000 doesn't match Rx offloads capabilities 0x0 in rte_eth_dev_configure()

EAL: Error - exiting with code: 1
  Cause: Cannot configure device: err=-22, port=0

Please advice