[dpdk-dev,2/3] ethdev: fail if Tx queue offload is not supported at all

  Do not allow to request unsupported Tx offload since all checks
are removed from PMDs because of consistency check in ethdev.
Otherwise application may rely on offload which is not actually
supported and send traffic with, for example, wrong checksums,
truncated packets or packets with garbage.

Fixes: d04dd6d4ed67 ("ethdev: new Rx/Tx offloads API")

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
  

--Shahaf


> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Friday, May 11, 2018 7:26 PM
> To: dev@dpdk.org
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Shahaf Shuler <shahafs@mellanox.com>; Wei Dai
> <wei.dai@intel.com>
> Subject: [PATCH 2/3] ethdev: fail if Tx queue offload is not supported at all
> 
> Do not allow to request unsupported Tx offload since all checks are removed
> from PMDs because of consistency check in ethdev.
> Otherwise application may rely on offload which is not actually supported
> and send traffic with, for example, wrong checksums, truncated packets or
> packets with garbage.
> 
> Fixes: d04dd6d4ed67 ("ethdev: new Rx/Tx offloads API")
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index dd36e6270..60577efcf 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1744,6 +1744,16 @@ rte_eth_tx_queue_setup(uint16_t port_id,
> uint16_t tx_queue_id,
>  				local_conf.offloads,
>  				dev_info.tx_queue_offload_capa,
>  				__func__);
> +		/*
> +		 * Applications which are not converted yet to the new
> +		 * Tx offload API may request device level offloads on
> +		 * queue level (and nothing is requested on device level).
> +		 * However, if the offload is not supported at all Tx
> +		 * queue setup must fail.
> +		 */
> +		if ((local_conf.offloads & dev_info.tx_offload_capa) !=
> +		    local_conf.offloads)
> +			return -EINVAL;

Not converted application doesn't have a clue what are per-queue offloads, and this is the error they will get when the Tx queue configuration will fail.

How about using ETH_TXQ_FLAGS_IGNORE flag, which explicitly says "application converted to the new Tx offloads API". and have 2 different checks:
1. for converted application the already exist check[1] with the related error.
2. for not converted application your check with a related error. 


[1]
if ((local_conf.offloads & dev_info.tx_queue_offload_capa) !=
     local_conf.offloads) {                                  

>  	}
> 
>  	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
> --
> 2.17.0
  

On 05/13/2018 08:37 AM, Shahaf Shuler wrote:
>> Do not allow to request unsupported Tx offload since all checks are removed
>> from PMDs because of consistency check in ethdev.
>> Otherwise application may rely on offload which is not actually supported
>> and send traffic with, for example, wrong checksums, truncated packets or
>> packets with garbage.
>>
>> Fixes: d04dd6d4ed67 ("ethdev: new Rx/Tx offloads API")
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index dd36e6270..60577efcf 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -1744,6 +1744,16 @@ rte_eth_tx_queue_setup(uint16_t port_id,
>> uint16_t tx_queue_id,
>>   				local_conf.offloads,
>>   				dev_info.tx_queue_offload_capa,
>>   				__func__);
>> +		/*
>> +		 * Applications which are not converted yet to the new
>> +		 * Tx offload API may request device level offloads on
>> +		 * queue level (and nothing is requested on device level).
>> +		 * However, if the offload is not supported at all Tx
>> +		 * queue setup must fail.
>> +		 */
>> +		if ((local_conf.offloads & dev_info.tx_offload_capa) !=
>> +		    local_conf.offloads)
>> +			return -EINVAL;
> Not converted application doesn't have a clue what are per-queue offloads, and this is the error they will get when the Tx queue configuration will fail.
>
> How about using ETH_TXQ_FLAGS_IGNORE flag, which explicitly says "application converted to the new Tx offloads API". and have 2 different checks:
> 1. for converted application the already exist check[1] with the related error.
> 2. for not converted application your check with a related error.

Yes, I like the idea. Many thanks. I'll send v2 shortly.

> [1]
> if ((local_conf.offloads & dev_info.tx_queue_offload_capa) !=
>       local_conf.offloads) {
>
>>   	}
>>
>>   	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
>> --
>> 2.17.0